-
Notifications
You must be signed in to change notification settings - Fork 517
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
Update Qwen2.5 configs #1999
Update Qwen2.5 configs #1999
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/torchtune/1999
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 8596e5a with merge base 18d97f0 (): This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
||
# Model arguments | ||
model: | ||
_component_: torchtune.models.qwen2_5.qwen2_5_0_5b |
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 understand that you're making the filename change 0_5B -> 0.5B for consistency with other configs, but honestly would prefer to just move everything to 0_5B so it matches the builders (doesn't have to be in this PR though)
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.
+1, i would prefer if we avoided using periods for names, and only use them when they are a path or file type
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.
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.
This is the same thing we have here, no?
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 thought we use underscores instead of periods?: #1863 (comment)
log_every_n_steps: 1 | ||
log_peak_memory_stats: False | ||
|
||
# Profiler (disabled) |
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.
controversial take but if we want consistency we should leave these in. idc too much for this PR but I thought that was the whole point of a bunch of @felipemello1's changes. Either way would like to compress this config substantially in a separate PR
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 ok with making the profiler simpler is a separate PR
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 hate decimal points
Yeah this is a misleading comment. We do use underscores for model builders, but the model should just get downloaded to a directory with the exact same name as the model on the Hub. |
Everything else is cosmetic.