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

ROCm 6.0 prep changes #4537

Merged
merged 4 commits into from
Oct 20, 2023
Merged

ROCm 6.0 prep changes #4537

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6776b4b
ROCm 6.0 prep changes
loadams Oct 18, 2023
2b93fae
PR feedback
loadams Oct 18, 2023
1c48468
Try updating apex
loadams Oct 18, 2023
4023d36
Merge branch 'master' into loadams/rocm6-changes
loadams Oct 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions op_builder/cpu_adagrad.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,4 @@ def include_paths(self):
CUDA_INCLUDE = []
elif not self.is_rocm_pytorch():
CUDA_INCLUDE = [os.path.join(torch.utils.cpp_extension.CUDA_HOME, "include")]
else:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rraminen @jithunnair-amd @BLOrange-AMD - this is a re-implementation of PR #4172. Since I'm not able to re-run the tests on that PR due to the permissions on the ROCm fork that it is from. Do these changes make sense and should we go ahead with merging these changes?

Copy link

@BLOrange-AMD BLOrange-AMD Oct 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Looks like CUDA_INCLUDE is not initialized with ROCm. Original PR has this: https://github.com/ROCmSoftwarePlatform/DeepSpeed/blob/00120efcc7796d302c395f6d1f0e9007335ea5c1/op_builder/cpu_adagrad.py#L42

Please apply this to other locations as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

@loadams "Since I'm not able to re-run the tests on that PR due to the permissions on the ROCm fork that it is from" ... Can you please elaborate what we need to change on the permissions for our fork, so we can enable CI running on PRs from our fork?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@BLOrange-AMD - good catch, thanks. I'm also happy to merge your original PR, just I'm not able to re-run the transient test/merge the latest changes into it. If you could do that we can just merge that one?

Copy link
Contributor

Choose a reason for hiding this comment

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

@loadams Since you're able to kick off CI on MI200 using this PR, let's use this PR as the one to target merge. I wouldn't want to merge a PR that we cannot run CI on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jithunnair-amd - running here on updated PR - let me know if this looks good to merge?

Copy link
Contributor

@jithunnair-amd jithunnair-amd Oct 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@loadams The above run failed with "5 failed, 42 passed, 10 skipped, 16 warnings", whereas one of your earlier runs with the apex fixes ran many more tests: 79 failed, 735 passed, 129 skipped, 40 warnings. That looks like the failures in the above job just terminated the run?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, @jithunnair-amd - that's due to the environment issues on our side we are still trying to work out to get resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But if the overall PR is fine (and doesn't appear to be causing these from our side), then we should be fine to go ahead and complete the PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, sounds good!

Btw, we have taken an internal action item to look at the DeepSpeed unit test status upstream so we can get this workflow to green status. If there are certain failures that you know are due to CI environment issues, please let us know so we can exclude them from our investigation.

CUDA_INCLUDE = [
os.path.join(torch.utils.cpp_extension.ROCM_HOME, "include"),
os.path.join(torch.utils.cpp_extension.ROCM_HOME, "include", "rocrand"),
os.path.join(torch.utils.cpp_extension.ROCM_HOME, "include", "hiprand"),
]
return ['csrc/includes'] + CUDA_INCLUDE
6 changes: 0 additions & 6 deletions op_builder/cpu_adam.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,4 @@ def include_paths(self):
CUDA_INCLUDE = []
elif not self.is_rocm_pytorch():
CUDA_INCLUDE = [os.path.join(torch.utils.cpp_extension.CUDA_HOME, "include")]
else:
CUDA_INCLUDE = [
os.path.join(torch.utils.cpp_extension.ROCM_HOME, "include"),
os.path.join(torch.utils.cpp_extension.ROCM_HOME, "include", "rocrand"),
os.path.join(torch.utils.cpp_extension.ROCM_HOME, "include", "hiprand"),
]
return ['csrc/includes'] + CUDA_INCLUDE
3 changes: 0 additions & 3 deletions op_builder/random_ltd.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,4 @@ def sources(self):

def include_paths(self):
includes = ['csrc/includes']
if self.is_rocm_pytorch():
from torch.utils.cpp_extension import ROCM_HOME
includes += ['{}/hiprand/include'.format(ROCM_HOME), '{}/rocrand/include'.format(ROCM_HOME)]
return includes
3 changes: 0 additions & 3 deletions op_builder/transformer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,4 @@ def sources(self):

def include_paths(self):
includes = ['csrc/includes']
if self.is_rocm_pytorch():
from torch.utils.cpp_extension import ROCM_HOME
includes += ['{}/hiprand/include'.format(ROCM_HOME), '{}/rocrand/include'.format(ROCM_HOME)]
return includes