Skip to content
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.

Sign up for GitHub

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

Jump to bottom

ZeRO-3 Slowdown #1170

Closed
wants to merge 40 commits into from
Closed

ZeRO-3 Slowdown #1170

wants to merge 40 commits into from

Conversation

tjruwase
Copy link
Contributor

@tjruwase tjruwase commented Jun 17, 2021 •
edited
Loading

Fix significant zero-3 slowdown compared to zero-2, based on #1069.
Two key parts of this PR:

  1. Fix hook registration problem that created off-by-one tracing issue
  2. Move tracing book-keeping to parameter partition logic.

@tjruwase tjruwase requested a review from samyam June 17, 2021 16:45
@tjruwase tjruwase mentioned this pull request Jun 17, 2021
Closed
@tjruwase
Copy link
Contributor Author

tjruwase commented Sep 8, 2021

@zarzen, my teammates have reviewed this PR and it looks good. The next step is to rerun correctness and perf tests, especially since significant code was merged into the master branch for our last release. Can you also run perf tests on your side to check that there are no regressions. I will be doing the same. Although this is a bit of work, but hopefully it would make merging #1188 easier.

@zarzen
Copy link
Contributor

zarzen commented Sep 8, 2021

@zarzen, my teammates have reviewed this PR and it looks good. The next step is to rerun correctness and perf tests, especially since significant code was merged into the master branch for our last release. Can you also run perf tests on your side to check that there are no regressions. I will be doing the same. Although this is a bit of work, but hopefully it would make merging #1188 easier.

sure, I have seen you merged the master into my branch, Thanks!
I will start the regression tests ASAP, possibly complete before Friday.

@tjruwase tjruwase mentioned this pull request Oct 7, 2021
Merged
@tjruwase tjruwase mentioned this pull request Oct 21, 2021
Merged
@tjruwase
Copy link
Contributor Author

tjruwase commented Apr 6, 2022

Subsumed by #1453

@tjruwase tjruwase closed this Apr 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zarzen zarzen zarzen left review comments

@stas00 stas00 stas00 left review comments

@jfc4050 jfc4050 jfc4050 left review comments

@samyam samyam samyam approved these changes

@eltonzheng eltonzheng Awaiting requested review from eltonzheng

@ShadenSmith ShadenSmith Awaiting requested review from ShadenSmith

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@tjruwase @zarzen @samyam @stas00 @jfc4050 @jeffra