Skip to content

Conversation

@conglongli
Copy link
Contributor

@conglongli conglongli commented Oct 8, 2021

CL integration for big science project. Currently only integrated for GPT pre-training. Includes an example train script and a tutorial. Before merge need @stas00 to test it on real big science experiments, also probably need @ShadenSmith 's code review since there are some changes related to DeepSpeed pipeline parallelism support.

Corresponding changes needed on DS side: deepspeedai/DeepSpeed#1440

Blocking event:

Copy link
Contributor

@stas00 stas00 left a comment

Choose a reason for hiding this comment

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

Thank you, @conglongli!

I added a few suggestions to the tutorial's wording and one question.

Next to testing it out.

conglongli and others added 3 commits October 9, 2021 11:31
Co-authored-by: Stas Bekman <stas00@users.noreply.github.com>
@stas00
Copy link
Contributor

stas00 commented Oct 9, 2021

Added a failing test, but can't push into this PR, since your branch doesn't allow maintainer edit.

E           RuntimeError: The expanded size of the tensor (16) must match the existing size (22) at non-singleton dimension 3.  Target sizes: [1, 2, 16, 16].  Tensor sizes: [1, 1, 22, 22]

Copy link
Contributor

@stas00 stas00 left a comment

Choose a reason for hiding this comment

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

Just trying to make the code easier to grasp faster

conglongli and others added 4 commits October 9, 2021 13:55
Co-authored-by: Stas Bekman <stas00@users.noreply.github.com>
Co-authored-by: Stas Bekman <stas00@users.noreply.github.com>
@stas00 stas00 self-requested a review October 9, 2021 23:32
Copy link
Contributor

@stas00 stas00 left a comment

Choose a reason for hiding this comment

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

All is now working, but need to wait for deepspeedai/DeepSpeed#1443 to be merged before we can merge this.

Thank you, @conglongli

@stas00
Copy link
Contributor

stas00 commented Oct 10, 2021

CI seems to check out main branch's head, which is absolutely wrong. CI really needs some attention.

But the tests pass, so merging this.

@stas00 stas00 merged commit 0035f06 into bigscience-workshop:main Oct 10, 2021
adammoody pushed a commit to adammoody/Megatron-DeepSpeed that referenced this pull request Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants