-
Notifications
You must be signed in to change notification settings - Fork 1
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
Support associating tasks with tags, minor cleanups #21
Conversation
Also addresses some minor TODOs.
Tags are working in FakeTracker, but not WandbTracker...
Likely why WandbTracker was crashing.
Ensures that dependencies in pyproject.toml stay sorted.
… dmcc/wandb-tags/sc-14754
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 somehow can't leave a code comment on this line, but action.py:12 has an ambiguous "they", which I think should be "subclasses"
Only now do I see I could have viewed the union of all the commits and reviewed that instead. Next time! |
- `tags` should be a set, not a list. Changes throughout. - Add `--tags` to `click_options` so it shows up in the correct group. - Cleanups and improvements suggested by @drewpca.
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.
Thanks Drew! Please take another look.
We now parse CSV lists and lexicalize shell commands as Click callbacks. As a result, all recipients of Click options can avoid parsing any options from the command line and instead operate on more appropriate types.
Fixed! |
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.
Thanks Drew for the many rounds of comments!
tl;dr: You can now use
--tags tag1,tag2
when launching tasks, fixes #20. All tags in the list will be associated with all launched tasks.This PR also includes some overdue Ruff and pre-commit updates, addressing their changes. Got a little carried away -- ideally, these would probably be a separate PR.