-
Notifications
You must be signed in to change notification settings - Fork 124
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
benchmark failing because of lack of flag #888
Conversation
Reviewer's Guide by SourceryThis pull request introduces a new function Updated class diagram for CLI argument parsingclassDiagram
class bench_parser{
+MODEL
+func
}
class run_serve_perplexity_args{
+ctx_size
}
class bench_run_serve_perplexity_args{
+authfile
+device
}
bench_parser -- bench_run_serve_perplexity_args : uses
run_serve_perplexity_args -- bench_run_serve_perplexity_args : uses
note for bench_parser "Arguments for benchmarking"
note for run_serve_perplexity_args "Arguments for running, serving, and perplexity"
note for bench_run_serve_perplexity_args "Shared arguments"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @ericcurtin - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider renaming
bench_run_serve_perplexity_args
to something more descriptive, as it's now used in multiple contexts.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
2e9079c
to
6c991e5
Compare
Can you add a test to run bench without checking for output, to at least catch crashes. |
6c991e5
to
172b836
Compare
Specifically priviledged because it's not present in the args object. Signed-off-by: Eric Curtin <ecurtin@redhat.com>
172b836
to
c2f81c5
Compare
LGTM |
benchmark failing because of lack of flag Signed-off-by: utherp0 <ian.lawson@redhat.com>
Specifically priviledged because it's not present in the args object.
Summary by Sourcery
Fixes a bug where the
bench
command was missing some arguments. This change introduces a new functionbench_run_serve_perplexity_args
to define arguments shared betweenbench
,run
andserve
commands. Thengl
argument was removed from thebench
command and is now loaded from the model.