Skip to content

Added 4-byte alignment on NCCL/RCCL#1328

Merged
tjruwase merged 19 commits into
deepspeedai:masterfrom
amathews-amd:msrcha_137_alignment_fix_upstream
Sep 9, 2021
Merged

Added 4-byte alignment on NCCL/RCCL#1328
tjruwase merged 19 commits into
deepspeedai:masterfrom
amathews-amd:msrcha_137_alignment_fix_upstream

Conversation

@amathews-amd

@amathews-amd amathews-amd commented Aug 26, 2021

Copy link
Copy Markdown
Contributor

Fixes https://ontrack.amd.com/browse/MSRCHA-137

  • Adds 4-byte alignment on NCCL/RCCL workloads to speed up workloads.
    • The start location of all data partitions (across worldsize) is aligned to 4-byte boundary.

@amathews-amd

Copy link
Copy Markdown
Contributor Author

@suffiank , @jithunnair-amd , @jeffdaily , @sunway513 please review.

@tjruwase

Copy link
Copy Markdown
Contributor

@amathews-amd, the code is failing CI due to formatting issues. Please see here for how to address these issues. Thanks.

@amathews-amd

Copy link
Copy Markdown
Contributor Author

Looks like checkpointing has an issue; debugging.

@tjruwase

Copy link
Copy Markdown
Contributor

@amathews-amd, can you add unit tests for this?

@amathews-amd

Copy link
Copy Markdown
Contributor Author

@amathews-amd, can you add unit tests for this?

Let me see what I can do.

Can you point me to instructions on how to setup a development environment and environment to test (num-gpus, installed package dependencies) ? I am currently using https://github.com/microsoft/DeepSpeed/blob/master/docker/Dockerfile and running pytest, but there seems to be other dependencies.

Comment thread deepspeed/runtime/zero/stage2.py Outdated
@jeffra

jeffra commented Aug 27, 2021

Copy link
Copy Markdown
Collaborator

@amathews-amd, can you add unit tests for this?

Let me see what I can do.

Can you point me to instructions on how to setup a development environment and environment to test (num-gpus, installed package dependencies) ? I am currently using https://github.com/microsoft/DeepSpeed/blob/master/docker/Dockerfile and running pytest, but there seems to be other dependencies.

This should include all the packages needed to run our tests, we currently assume the machine that is running the tests has 4 gpus available.

https://github.com/microsoft/DeepSpeed/blob/master/requirements/requirements-dev.txt

Comment thread deepspeed/runtime/zero/stage2.py Outdated
@tjruwase

Copy link
Copy Markdown
Contributor

@amathews-amd, can you please share some perf improvement numbers that you see with these changes? Thanks!

@amathews-amd

Copy link
Copy Markdown
Contributor Author

@tjruwase , on MI100 gpu's, the improvement for the dataset that trips this issue was 2.05x in training samples per second. The core developers of RCCL believe that this will improve NCCL performance also. When I tested on V100, I saw that this have not improved performance improvement significantly (1.02x), but that might be due to hardware that I tested on.

I believe @suffiank has also verified improvement internally to MSFT.

@tjruwase

Copy link
Copy Markdown
Contributor

@tjruwase , on MI100 gpu's, the improvement for the dataset that trips this issue was 2.05x in training samples per second. The

That is awesome. Thanks.
Regarding this PR, it seems only the unit tests are outstanding. The changes LGTM.

@amathews-amd

Copy link
Copy Markdown
Contributor Author

@tjruwase , on MI100 gpu's, the improvement for the dataset that trips this issue was 2.05x in training samples per second. The

That is awesome. Thanks.
Regarding this PR, it seems only the unit tests are outstanding. The changes LGTM.

Done. Please let me know if the tests are not in the right place.

@tjruwase

tjruwase commented Aug 30, 2021

Copy link
Copy Markdown
Contributor

@amathews-amd, this looks great. Waiting for CI to complete and this can be merged. In the meantime, I will alert other team members.

@samyam, @jeffra FYI,

Comment thread deepspeed/runtime/zero/stage2.py Outdated
Comment thread deepspeed/runtime/zero/stage2.py Outdated
Comment thread deepspeed/runtime/zero/stage2.py Outdated
Comment thread deepspeed/runtime/zero/stage2.py Outdated
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.

4 participants