Align runtime arguments with run, serve, bench, and perplexity #820
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cgruver
requested review from
rhatdan,
ericcurtin,
bmahabirbu,
maxamillion,
swarajpande5,
jhjaggars,
slp and
engelmi
as code owners
Reviewer's Guide by SourceryThis pull request refactors command-line arguments to improve consistency and flexibility. The global Updated class diagram for CLI argument parsingclassDiagram
class ArgumentParser {
+add_argument(...)
}
class Subparsers {
+add_parser(name, help)
}
class PerplexityParser {
+MODEL
+func
}
class BenchParser {
+MODEL
+func
+--ngl
+--network
}
class RunParser {
+--ctx-size
+--device
+-n --name
+--network
+--privileged
+--seed
+--temp
+--ngl
}
ArgumentParser *-- Subparsers : subparsers
Subparsers -- PerplexityParser : "perplexity"
Subparsers -- BenchParser : "bench"
Subparsers -- RunParser : "run"
note for RunParser "Added --ngl"
note for BenchParser "Added --ngl"
note for RunParser "Added common runtime arguments"
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @cgruver - I've reviewed your changes - here's some feedback:
Overall Comments:
- It might be helpful to add a brief explanation of why the global
--gpuargument was removed. - Consider adding a test case to verify the functionality of the new
--nglargument across different subcommands.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 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.
|
This is a draft. I have not been able to fully test yet. |
|
@ericcurtin or @rhatdan I'd appreciate a sanity check on this. After I land and get home I plan to run this change through live testing. All the CI checks passed, but since I moved args around I'm not sure if the test coverage works for
|
|
LGTM. but you need a rebase. |
Signed-off-by: Charro Gruver <cgruver@redhat.com>
…consistency. Signed-off-by: Charro Gruver <cgruver@redhat.com>
Signed-off-by: Charro Gruver <cgruver@redhat.com>
There was a problem hiding this comment.
Hey @cgruver - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a section to the documentation explaining how GPU selection is handled via environment variables now.
- It might be helpful to provide a migration guide or a note for users who relied on the removed
--gpuflag.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 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.
Comment on lines
+31
to
+32
| #### **--ctx-size**, **-c** | ||
| size of the prompt context (default: 2048, 0 = loaded from model) |
There was a problem hiding this comment.
suggestion: Clarify the meaning of context size.
The documentation should explain what "prompt context" refers to and how it affects the model's behavior.
Suggested change
| #### **--ctx-size**, **-c** | |
| size of the prompt context (default: 2048, 0 = loaded from model) | |
| #### **--ctx-size**, **-c** | |
| size of the prompt context (default: 2048, 0 = loaded from model) | |
| The prompt context refers to the maximum amount of input text (measured in tokens) that the model uses to generate responses. A larger context allows the model to consider more preceding text, potentially improving the coherence and relevance of its output, but it may also increase memory usage and processing time. |
There was a problem hiding this comment.
This is a good suggestion if we want to add this in this PR, it's well worded
There was a problem hiding this comment.
Thank you for your feedback, we will generate more comments like this in the future.
| @@ -186,20 +186,6 @@ def configure_arguments(parser): | |||
| help="""run RamaLama using the specified container engine. | |||
| The RAMALAMA_CONTAINER_ENGINE environment variable modifies default behaviour.""", | |||
| ) | |||
| parser.add_argument( | |||
There was a problem hiding this comment.
issue (complexity): Consider extracting the common CLI argument "--ngl" into a helper function to reduce code duplication.
Consider extracting the common CLI argument (e.g. "--ngl") into a helper function to avoid duplication and ensure consistency. For example:
```python
def add_ngl_argument(parser, config):
parser.add_argument(
"--ngl",
dest="ngl",
type=int,
default=config.get("ngl", -1),
help="Number of layers to offload to the gpu, if available",
)Then update the subcommand and _run parser definitions by replacing the duplicated code with a call to the helper:
def bench_parser(subparsers):
parser = subparsers.add_parser("bench", aliases=["benchmark"], help="benchmark specified AI Model")
parser.add_argument("--network", type=str, default="none", help="set the network mode for the container")
add_ngl_argument(parser, config)
parser.add_argument("MODEL")
parser.set_defaults(func=bench_cli)
def _run(parser):
parser.add_argument("--authfile", help="path of the authentication file")
parser.add_argument("-c", "--ctx-size", dest="context", default=config.get('ctx_size', 2048), help="size of the prompt context (0 = loaded from model)")
# other arguments...
add_ngl_argument(parser, config)
# remaining definitions...This maintains full functionality while reducing complexity and easing future maintenance.
ericcurtin
approved these changes
Feb 14, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Removed the global argument
--gpuAligned the runtime argument
--nglwith the subcommandsbench,perplexity,run,serveAdded arguments to the subcommand perplexity to align it with common arguments of
runandserveSummary by Sourcery
Remove the global
--gpuargument and introduce new runtime arguments for theperplexitysubcommand, aligning it with therunandservesubcommands. Standardize the--nglargument across all relevant subcommands. Update the documentation to reflect these changes.Enhancements:
--gpuargument.--nglruntime argument acrossbench,perplexity,run, andservesubcommands.perplexitysubcommand withrunandserveby adding common runtime arguments.Documentation:
perplexity,run,serve, andbenchcommands to reflect the addition of new runtime arguments and removal of the global--gpuargument.--authfile,--ctx-size,--device,--name,--network,--ngl,--privileged,--seed,--temp, and--tls-verify.--deviceargument in the documentation forrunandservecommands.runcommand documentation.