Skip to content

Commit

Permalink
Bug Fix: Check for explicit cli device (fast) (#1374)
Browse files Browse the repository at this point in the history
  • Loading branch information
@Jack-Khuu
Jack-Khuu authored Nov 14, 2024
1 parent ed0fb30 commit 4697764
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion torchchat/cli/cli.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -533,7 +533,7 @@ def arg_init(args):
# Localized import to minimize expensive imports
from torchchat.utils.build_utils import get_device_str

if args.device is None:
if args.device is None or args.device == "fast":
args.device = get_device_str(
args.quantize.get("executor", {}).get("accelerator", default_device)
)
Expand Down

2 comments on commit 4697764

@mikekgfb
Copy link
Contributor

@mikekgfb mikekgfb commented on 4697764 Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Jack-Khuu Not sure why args.device == "fast" should be treated as a functional nop relative to executor spec? Did I miss something with the original PR?

My thinking was that if the user says "fast" device, they should get the fastest device on this node with them same outcome as always.(To enable this, "fast" is no longer the arg parser default, so we should not get args.device=="fast" unless user specifically said so. )

@Jack-Khuu
Copy link
Contributor Author

@Jack-Khuu Jack-Khuu commented on 4697764 Nov 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did I miss something with the original PR?

Prior to adding this condition, providing fast as an arg crashed the entire command since it was never resolved to an actual device.
#1373

Edit: Ah but, this fix PR is also wrong since fast can no longer override the quantize config, I'll spin something up

Please sign in to comment.