ROCm 6.0 prep changes #4537
ROCm 6.0 prep changes #4537
Conversation
loadams
requested review from
jeffra,
RezaYazdaniAminabadi and
cmikeh2
as code owners
| @@ -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: | |||
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@jithunnair-amd - running here on updated PR - let me know if this looks good to merge?
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
Yes, @jithunnair-amd - that's due to the environment issues on our side we are still trying to work out to get resolved.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
* ROCm 6.0 prep changes * PR feedback * Try updating apex
* ROCm 6.0 prep changes * PR feedback * Try updating apex
Reimplementation of #4172.
These changes should be backward-compatible with multiple previous ROCm versions, please refer to https://rocm.docs.amd.com/en/latest/understand/file_reorg.html#wrapper-header-files
Co-authored-by: BLOrange-AMD [email protected]