Skip to content

Added a single test function script and fix debug-test.sh to be more robust #7279

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

Merged
merged 6 commits into from
May 17, 2024

Conversation

mofosyne
Copy link
Collaborator

@mofosyne mofosyne commented May 14, 2024

Continuation of #7096 somewhat to address cases where a person may just want to run a single test.

Also discovered that the test binaries expects to be run in the git root context so need to fix debug-test.sh to take that into account.

@josh-ramer you may want to have a quick look a this to see if this would also be useful to you as well.

This is what the output visually looks like. With emphasis on the last three lines for clearer feedback

image


(Created this to try and more quickly pin down a CI issue)

@mofosyne
Copy link
Collaborator Author

Actually @josh-ramer when I ran ./scripts/debug-test.sh test 24 with this script I noticed this printout

Ran Test #24: test-eval-callback
GDB args: /home/mofosyne/gitextern/llama.cpp/build-ci-debug/bin/eval-callback "--hf-repo" "ggml-org/models" "--hf-file" "tinyllamas/stories260K.gguf" "--model" "stories260K.gguf" "--prompt" "hello" "--seed" "42" "-ngl" "0"
GDB args @: /home/mofosyne/gitextern/llama.cpp/build-ci-debug/bin/eval-callback~/gitextern/llama.cpp ~/gitextern/llama.cpp/build-ci-debug ~/gitextern/llama.cpp ~/gitextern/llama.cpp

Am i reading it correctly? I would have thought that args[@] would be the same as gdb_args[test_number] but with path expanded, but that doesn't seem to be the case.

@mofosyne mofosyne added script Script related 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 14, 2024
@josh-ramer
Copy link
Contributor

josh-ramer commented May 14, 2024

That comment is a little deceptive, what the code really does is find paths with /../ in them & replace the path/../path with ../path. There is probably a better solution than that out there if you're pretty skilled in bash scripting. Maybe one easy alternative would be to remove the suffix before /../.

Nice addition, makes sense to be able to execute as well.

@mofosyne
Copy link
Collaborator Author

Maybe you can suggest a better comment? Either way, see if this script is working well for your flow and if I didn't break your gdb flow, then well should be good to go!

@josh-ramer
Copy link
Contributor

I've got a review started, but there are no EC2 spot instance available right now (first time I've had that happen to my workflow) so I haven't run it yet to make sure it works, but as soon as they're available, I plan to run it for you.

Copy link
Contributor

@josh-ramer josh-ramer left a comment

Choose a reason for hiding this comment

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

LGTM, will run as soon as I have the compute available.

@mofosyne mofosyne self-assigned this May 15, 2024
@mofosyne mofosyne marked this pull request as draft May 15, 2024 01:49
@mofosyne mofosyne marked this pull request as ready for review May 15, 2024 23:25
@mofosyne mofosyne added merge ready indicates that this may be ready to merge soon and is just holding out in case of objections and removed merge ready indicates that this may be ready to merge soon and is just holding out in case of objections labels May 15, 2024
@mofosyne
Copy link
Collaborator Author

@josh-ramer ready

@josh-ramer
Copy link
Contributor

LGTM

@mofosyne
Copy link
Collaborator Author

@josh-ramer see if you can press approve as there needs to be one other approval before it can merge in.

Copy link
Contributor

@josh-ramer josh-ramer left a comment

Choose a reason for hiding this comment

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

LGTM

@josh-ramer
Copy link
Contributor

@josh-ramer see if you can press approve as there needs to be one other approval before it can merge in.

Looks like I don't have write access. I think that means I'd have to be added as a collaborator like you.

@mofosyne mofosyne merged commit 51e9d02 into ggml-org:master May 17, 2024
11 checks passed
@mofosyne mofosyne deleted the run-single-test-script branch May 17, 2024 12:40
@mofosyne
Copy link
Collaborator Author

@josh-ramer thanks. It's in now, hope it helps!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review Complexity : Low Trivial changes to code that most beginner devs (or those who want a break) can tackle. e.g. UI fix script Script related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants