-
Notifications
You must be signed in to change notification settings - Fork 22
Cleanups post Mistral Integration #27
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
Cleanups post Mistral Integration #27
Conversation
…ode clarity and organization
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.
Pull Request Overview
This pull request performs code cleanups following the Mistral integration, focusing on import reordering, minor refactoring, and improved code formatting and documentation.
- Refactored import statements and formatting in TornadoVMMasterPlan.java and Model.java
- Removed an outdated reference from TornadoVMLayerPlanner.java
- Enhanced documentation and refactored the main method in LlamaApp.java
- Standardized string quoting and argument formatting in the llama-tornado Python script
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/com/example/tornadovm/TornadoVMMasterPlan.java | Reordered imports and reformatted the conditional assignment in the constructor |
| src/main/java/com/example/tornadovm/TornadoVMLayerPlanner.java | Removed a redundant Javadoc reference |
| src/main/java/com/example/model/Model.java | Reordered and corrected duplicate import statements |
| src/main/java/com/example/LlamaApp.java | Introduced helper methods with detailed Javadoc to improve clarity |
| llama-tornado | Standardized string quoting and improved argparse argument formatting |
| "--stream", type=bool, default=True, help="Enable streaming output" | ||
| ) | ||
| llm_group.add_argument( | ||
| "--echo", type=bool, default=False, help="Echo the input prompt" |
Copilot
AI
Jun 18, 2025
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.
Using type=bool for boolean flags in argparse may lead to unexpected behavior. Consider using action="store_true" for flags like "--stream".
| "--stream", type=bool, default=True, help="Enable streaming output" | |
| ) | |
| llm_group.add_argument( | |
| "--echo", type=bool, default=False, help="Echo the input prompt" | |
| "--stream", action="store_true", help="Enable streaming output" | |
| ) | |
| llm_group.add_argument( | |
| "--echo", action="store_true", help="Echo the input prompt" |
| "--stream", type=bool, default=True, help="Enable streaming output" | ||
| ) | ||
| llm_group.add_argument( | ||
| "--echo", type=bool, default=False, help="Echo the input prompt" |
Copilot
AI
Jun 18, 2025
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.
For a flag such as "--echo", using the action="store_true" pattern is generally more robust than type=bool. Consider revising this to better capture the intended boolean behavior.
| "--echo", type=bool, default=False, help="Echo the input prompt" | |
| "--echo", action="store_true", help="Echo the input prompt" |
No description provided.