This repository was archived by the owner on Sep 10, 2025. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 248
Fast non-model CLI commands #1349
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
Oops, something went wrong.
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.
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.
We might defer that to commands that actually use torch, and then we can just use torch.version ? No point in raising a version issue when all we do is run help or similar non-model cli commands?
Wdyt @Jack-Khuu
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 like having it here, since we can catch it at the very beginning as opposed to having version checking in every file.
Doing it this way will speed up the interface by a lot. Right now --help and --command is super slow. It also bogs down CI.
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 could see it either way, but I think I agree with @byjlw. I don't think the non-running commands are valuable in-and-of-themselves, so I think the argument would be that this is really a property of the tool (there is no
torchchatwithouttorch) and doing the lazy imports is just a speed optimization.If there were standalone commands that didn't need
torch(i.e. if people start usingdownloadjust to fetch models, but then run them with something else), then I could see making this a soft requirement and deferring it to the commands that need 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 actually added what @mikekgfb is suggesting a while back, so this check being in
arg_initactually does what all 3 y'all suggest (check only if needed, and check in one place)torchchat/torchchat.py
Lines 62 to 63 in 4a7dab8
arg_initis only called when it is pertinent