Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scripting & documenting debugging one test without anything else in the loop. #7096

Merged
merged 16 commits into from May 11, 2024

Conversation

josh-ramer
Copy link
Contributor

@josh-ramer josh-ramer commented May 6, 2024

Here is a little document about how to debug one test without having anything else run to get a short feedback loop.

@mofosyne
Copy link
Collaborator

mofosyne commented May 6, 2024

For a guide aimed at developers who are already working with the codebase, you might consider naming it something like "USAGE.md", "GUIDE.md", or "GETTING_STARTED.md".

(As a counterpart, we may need to also create DEVELOPER.md to store specific quick-starts for maintainers who want to directly contribute to this project.)

As you have currently written, it reads more like a blogpost (which you should do btw on your personal blog anyway). With some cleanup to formalise it and be more direct and formal, maybe it can be a good user guide.

@josh-ramer
Copy link
Contributor Author

josh-ramer commented May 6, 2024

For a guide aimed at developers who are already working with the codebase, you might consider naming it something like "USAGE.md", "GUIDE.md", or "GETTING_STARTED.md".

(As a counterpart, we may need to also create DEVELOPER.md to store specific quick-starts for maintainers who want to directly contribute to this project.)

As you have currently written, it reads more like a blogpost (which you should do btw on your personal blog anyway). With some cleanup to formalise it and be more direct and formal, maybe it can be a good user guide.

Absolutely, you are correct. I decided to extract just the part about how to debug a specific test without anything else in the loop. This was the actual direct part. I also removed most of the personality to keep it pointed & useful instead of educational. I appreciate your feedback. @mofosyne

@josh-ramer josh-ramer changed the title Documenting some startup, debugging, & testing lessons learned during my first issue. Documenting debugging one test without anything else in the loop. May 7, 2024
@mofosyne mofosyne added documentation Improvements or additions to documentation review complexity : low Trivial changes to code that most beginner devs (or those who want a break) can tackle. e.g. UI fix labels May 9, 2024
@mofosyne
Copy link
Collaborator

mofosyne commented May 9, 2024

Attempted to replicate your steps based on the readme with some difficulties. I think i was getting a bit confused. Maybe it should be a step by step instruction...

Also attempted to see if I can grok your script and make it a bit more standalone, give this a shot. Maybe it might be worth making it into a callable bash script.

#!/bin/bash
test_suite="test-tokenizer-0"

# Step 1: Prepare the Build Environment
rm -rf build-ci-debug && mkdir build-ci-debug && cd build-ci-debug
cmake -DCMAKE_BUILD_TYPE=Debug -DLLAMA_CUDA=1 -DLLAMA_FATAL_WARNINGS=ON .. && cd ..

# Step 2: Build the Project
make -j

# Step 3: Navigate to Test Directory
# Tip: If you see a warning about missing "cache" during installation. Then install it to save immense amounts of time!
cd tests

# Step 4: Identify Test Command for Debugging
# The output of this command will give you the command & arguments needed to run GDB.
# -R test-tokenizer : looks for all the test files named test-tokenizer* (R=Regex)
# -N : "show-only" disables test execution & shows test commands that you can feed to GDB.
# -V : Verbose Mode
echo "Avaliable Tests"
ctest -R ${test_suite} -V -N | grep "Total Tests:"

# Step 5: Debug the Test
read -p "Enter the number of the test command you want to debug: " test_number
ctest -R ${test_suite} -V -N | grep "${test_number}: Test command:"
test_bin=ctest -R ${test_suite} -V -N | grep "${test_number}: Test command:" | awk '{print $4}'
gguf_path=$(ctest -R ${test_suite} -V -N | grep "${test_number}: Test command:" | awk '{print $5}')
gdb --args $test_bin $gguf_path

@josh-ramer
Copy link
Contributor Author

Thanks, I'll try it & I'll make the test file regex a parameter.

@josh-ramer
Copy link
Contributor Author

Ok @mofosyne, I took your good idea & turned it into a script that lets you select a test to debug from a list. I outlined how the scripts works & how to use it in the readme.

@mofosyne
Copy link
Collaborator

Awesome. I've rejigged the script a bit as I was having difficulties getting ctest to show any number. Still cannot, but added a few checks to make it more obvious. I don't think I want to hold this off too long as if it works for you and is possibly just my setup being the issue then we can include it in anyway and see how other developers find the script.

One minor thing you may want to see if you can also add (optional) is if you could optionally add

./scripts/debug-test.sh <test suite> <test number>
./scripts/debug-test.sh test-tokenizer-0 3

Below is the updated script I've adjusted to add more 'early exit' error checks

#!/bin/bash

# Function to select and debug a test
function select_test() {
    test_suite="$1"

    # Sanity Check If Tests Is Detected
    max_tests=($(ctest -R ${test_suite} -V -N | grep "Total Tests:" | cut -d':' -f2 | awk '{$1=$1};1'))
    if [ $max_tests -eq "0" ]
    then
        echo "No tests avaliable"
        exit 1
    fi

    # List out avaliable tests
    printf "\n\nGathering tests that fit the REGEX: ${test_suite} ...\n"
    printf "Which test would you like to debug?\n"
    id=0
    tests=($(ctest -R ${test_suite} -V -N | grep "Test.\ #*" | cut -d':' -f2 | awk '{$1=$1};1'))
    gdb_params=($(ctest -R test-tokenizer -V -N | grep "Test command" | cut -d':' -f3 | awk '{$1=$1};1'))
    for s in "${tests[@]}"
    do
        echo "Test# ${id}"
        echo "  $s"
        ((id++))
    done

    # Prompt user which test they wanted to run
    printf "\nRun test#? "
    read n

    # Start GDB with the requested test binary and test arguments
    printf "Debugging(GDB) test: ${tests[n]} ...\n\n"
    test=${gdb_params[n*2]}
    test_arg=$(echo ${gdb_params[n*2+1]} | sed -e 's/^.//' -e 's/.$//')
    gdb --args ${test} ${test_arg}
}

# Step 0: Check the args
if [ $# -ne 1 ] || [ "$1" = "help" ]
then
    echo "Supply one regex to the script, e.g. test-tokenizer would\n"
    echo "return all the tests in files that match with test-tokenizer."
    exit 1
fi

# Step 1: Prepare the Build Environment

## Sanity check that we are actually in a git repo
repo_root=$(git rev-parse --show-toplevel)
if [ ! -d "$repo_root" ]; then
    echo "Error: Not in a Git repository."
    exit 1
fi

## Build test binaries
pushd "$repo_root" || exit 1
rm -rf build-ci-debug && mkdir build-ci-debug && pushd build-ci-debug || exit 1
cmake -DCMAKE_BUILD_TYPE=Debug -DLLAMA_CUDA=1 -DLLAMA_FATAL_WARNINGS=ON .. && popd || exit 1
make -j || exit 1
pushd tests || exit 1

# Step 2: Debug the Test
select_test "$1"

# Step 3: Return to the directory from which the user ran the command.
popd || exit 1
popd || exit 1

@mofosyne mofosyne self-assigned this May 10, 2024
@mofosyne mofosyne self-requested a review May 10, 2024 04:37
@josh-ramer
Copy link
Contributor Author

Thanks for the error checking. I'll add that in & I'll do more testing with the other test suites today & update the script with anything I find. I appreciate your help.

@josh-ramer
Copy link
Contributor Author

josh-ramer commented May 10, 2024

Alright @mofosyne, now it's a lot more robust, I ran it on many different test suites & fixed regex problems that I found. I also made the invocation of gdb capable of taking many arguments in case any of the test suites are setup that way. It seems to work well now in all the cases I tried out.

A good way to see that it works across the suite of tests is to run scripts/debug-test.sh test which will output >20 different possible tests & run them when selected.

@josh-ramer josh-ramer changed the title Documenting debugging one test without anything else in the loop. Scripting & documenting debugging one test without anything else in the loop. May 10, 2024
@mofosyne
Copy link
Collaborator

mofosyne commented May 11, 2024

Sorry to do this to you. But I think i narrowed what went wrong when trying to replicate your script. Your script as making lots of assumption about the state of the repo. After studying the ci flow in github actions in this repo, I think you are actually meant to run ctest within the build folder you created.

Ultimately, this is what appears to work reliably on my setup after rejigging your work further. Also made it such that you can also provide the test number in the commandline as well if you already know what test number you are trying to test against in the test suite.

Also updated the help

$ ./scripts/debug-test.sh
Usage: ./scripts/debug-test.sh [test_regex] [test_number]
e.g., ./scripts/debug-test.sh test-tokenizer
      ./scripts/debug-test.sh test-tokenizer 3
Supply one regex to the script to filter tests,
and optionally a test number to run a specific test.

When you run this line, it will ask you for the test number (this is same as your existing script intended behavior)

./scripts/debug-test.sh test

But this is a new one, where if you add the number it will skip the number prompting behavior.

./scripts/debug-test.sh test 3

Here is the updated script so you can check against your setup and then commit it if it works well. I've tested this to work on my setup.

#!/bin/bash
build_dir="build-ci-debug"
test_suite=${1:-}
test_number=${2:-}

# Function to select and debug a test
function select_test() {
    test_suite=${1:-test}
    test_number=${2:-}

    # Sanity Check If Tests Is Detected
    printf "\n\nGathering tests that fit REGEX: ${test_suite} ...\n"
    tests=($(ctest -R ${test_suite} -V -N | grep -E " +Test +#[0-9]+*" | cut -d':' -f2 | awk '{$1=$1};1'))
    if [ ${#tests[@]} -eq 0 ]
    then
        echo "No tests avaliable... check your compliation process..."
        echo "Exiting."
        exit 1
    fi

    if [ -z $test_number ]
    then
        # List out avaliable tests
        printf "Which test would you like to debug?\n"
        id=0
        for s in "${tests[@]}"
        do
            echo "Test# ${id}"
            echo "  $s"
            ((id++))
        done

        # Prompt user which test they wanted to run
        printf "\nRun test#? "
        read test_number
    else
        printf "\nUser Already Requested #${test_number}"
    fi

    # Start GDB with the requested test binary and arguments
    printf "Debugging(GDB) test: ${tests[test_number]}\n"
    # Change IFS (Internal Field Separator)
    sIFS=$IFS
    IFS=$'\n'

    # Get test args
    gdb_args=($(ctest -R ${test_suite} -V -N | grep "Test command" | cut -d':' -f3 | awk '{$1=$1};1' ))
    IFS=$sIFS
    printf "Debug arguments: ${gdb_args[test_number]}\n\n"

    # Expand paths if needed
    args=()
    for x in $(echo ${gdb_args[test_number]} | sed -e 's/"\/\<//' -e 's/\>"//')
    do
        args+=($(echo $x | sed -e 's/.*\/..\//..\//'))
    done

    # Execute debugger
    echo "gdb args: ${args[@]}"
    gdb --args ${args[@]}
}

# Step 0: Check the args
if [ -z "$test_suite" ] || [ "$1" = "help" ]
then
    echo "Usage: $0 [test_regex] [test_number]"
    echo "e.g., $0 test-tokenizer"
    echo "      $0 test-tokenizer 3"
    echo "Supply one regex to the script to filter tests,"
    echo "and optionally a test number to run a specific test."
    exit 1
fi

# Step 1: Reset and Setup folder context
## Sanity check that we are actually in a git repo
repo_root=$(git rev-parse --show-toplevel)
if [ ! -d "$repo_root" ]; then
    echo "Error: Not in a Git repository."
    exit 1
fi

## Reset folder to root context of git repo
pushd "$repo_root" || exit 1

## Create and enter build directory
rm -rf "$build_dir" && mkdir "$build_dir" && pushd "$build_dir" || exit 1

# Step 2: Setup Build Environment and Compile Test Binaries
cmake -DCMAKE_BUILD_TYPE=Debug -DLLAMA_CUDA=1 -DLLAMA_FATAL_WARNINGS=ON .. && make -j || exit 1

# Step 3: Debug the Test
select_test "$test_suite" "$test_number"

# Step 4: Return to the directory from which the user ran the command.
popd || exit 1
popd || exit 1
popd || exit 1

@josh-ramer
Copy link
Contributor Author

josh-ramer commented May 11, 2024

Thanks, nice cleanup & additional argument.
Ok, I tried exactly that on an Ubuntu box in AWS with no previous repo setup. I performed these steps to make sure that I have exactly the structure that the repository assumes.

  1. git reset --hard cuda-infill-segfault-bug # reset to last commit on branch
  2. git clean -xfn # to clean out files not in version control
  3. scripts/debug-test.sh test

Exactly what you provided me fails because I think it relies on CMake having persisted information about where to output builds. I altered what you gave me, just lines 90-91, to do exactly the same thing it did but to use the -B flag specifically when calling CMake from the repository root. Then changing directory to the build-ci-debug directory to continue the commands as you have them. Hopefully this works for both of us.

Copy link
Collaborator

@mofosyne mofosyne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, thanks for your contribution! I've adjusted the help a bit and was able to confirm that you change still works on my side.

I've also taken extra time to resync the documentation to match the new behavior of the script that you pushed in.

I've approved the change as I'm now confident that this script is portable (at least between us two). Also thanks for your suggestion on cleaning up my repo, to make the portability check more robust.

Anyway feel free to push in any more minor changes to the script (or minor/major change to the markdown documentation) if you feel so and merge anytime you are ready.

@josh-ramer
Copy link
Contributor Author

josh-ramer commented May 11, 2024

I don't have write access for the merge. Thanks for working with me, I appreciate you. Looking forward to the next time.

@mofosyne mofosyne merged commit fed0108 into ggerganov:master May 11, 2024
21 checks passed
@mofosyne
Copy link
Collaborator

All good! At least you have now made an easier way for you and others to debug this.

Definitely consider making a writeup of your deleted content in your personal blog and point to this PR so people can better recognise your contribution and also discover this option.

Aside from that, all the best and will be more than happy to check out your future contributions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation review complexity : low Trivial changes to code that most beginner devs (or those who want a break) can tackle. e.g. UI fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants