-
Notifications
You must be signed in to change notification settings - Fork 415
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
Remove all old timing calls #594
Conversation
1. Remove all uses of `state.max_epochs`; replaced with `state.max_duration` 2. Replaced all `state.step` with `int(state.timer.batch)` 3. Replaced all `state.epoch` with `int(state.timer.epoch)` 4. Removed the constraints that max_duration be specified in epochs; it can now be in any unit. Added test cases. 5. Added a helper method to the `Time` class to convert it to a timestring (with a test case) Closes #146 Closes #229 Closes #512
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.
Hmm would it make sense to attach to state the epoch, etc. properties? That would reduce the code verbosity here and also be easier for developers to access directly.
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.
Overall looks great just a few comments
Unfortunately, this would lead to naming conflicts, since I could rename it in both places, but am not a huge fan of having everything in the Alternatively, we could rename Agree it would be nice if these properties were more visible, but a good solution doesn't come to mind. Thoughts? |
Any import from a submodule will attempt to load the parent module's |
Good point on the naming conflicts -- let's keep them in |
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.
For the method cards, the actual READMEs should sit in the composer/algorithms/
folder, with a symlink in the docs/source
.
Whoops, good catch. Just fixed this. |
9d8ba29
to
b2441c0
Compare
state.max_epochs
; replaced withstate.max_duration
-- specifically in stochastic depth (Remove old timing calls from stochastic depth #229) and sequential length warmup (Remove old timing calls from Sequential Length Warmup #226). However, it does not fix the latter, as we still need to update the algorithm to support max_duration in terms of tokens or samples.state.step
withint(state.timer.batch)
state.epoch
withint(state.timer.epoch)
state.batch_idx
withint(state.timer.batch_in_epoch)
Time
class to convert it to a timestring (with a test case)Closes #146
Closes #229
Closes #512