-
Notifications
You must be signed in to change notification settings - Fork 115
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
On macOS this was returning an incorrect path #741
Conversation
Reviewer's Guide by SourceryThe pull request fixes an issue where the model path was hardcoded to Sequence diagram for model path resolutionsequenceDiagram
participant Client
participant ModelManager
Client->>ModelManager: get_model_path(args)
alt Before Fix - with --dryrun
ModelManager-->>Client: Return '/path/to/model'
else After Fix - with --dryrun
ModelManager->>ModelManager: exists(args)
alt Model exists
ModelManager-->>Client: Return actual model path
else Model doesn't exist
ModelManager->>ModelManager: pull(args)
ModelManager-->>Client: Return downloaded model path
end
end
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:
- Since you've removed the dryrun override, please verify that the behavior during dry runs is still correct on macOS. Double-check that tests (or add tests if needed) cover the scenario to ensure the new logic doesn't affect the expected behavior during a dry run.
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.
1a68aa1
to
61cfff1
Compare
61cfff1
to
971fb5d
Compare
$ ramalama --dryrun run granite-code llama-run -c 2048 --temp 0.8 /path/to/model At least try and show the correct path. Signed-off-by: Eric Curtin <[email protected]>
971fb5d
to
abc065d
Compare
Very tempting to turn off Fedora 42 packit build for a while... |
model_path = self.exists(args) | ||
if model_path: | ||
return model_path | ||
|
||
if args.dryrun: | ||
return "/path/to/model" |
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.
In this case, should we return something that indicates the model doesn't exist in the store already and we'd try to pull it?
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.
I don't think we should pull for "--dryrun", this is kinda just for, please print an approximate "podman run" command, "--dryrun" implies, don't do things like pull.
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.
Agreed, nice fix though.
You can actually see the real command by executing ramalama --debug run MODEL.
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.
I am not sure if we should print an error in the case that the model does not exists. But I could be swayed either way.
LGTM |
$ ramalama --dryrun run granite-code
llama-run -c 2048 --temp 0.8 /path/to/model
At least try and show the correct path.
Summary by Sourcery
Bug Fixes: