Skip to content

Hipify fixes for a successful DeepSpeed build #76141

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
Closed
wants to merge 5 commits into from
Closed

Hipify fixes for a successful DeepSpeed build #76141

wants to merge 5 commits into from

Conversation

rraminen
Copy link
Contributor

These commits are required to build DeepSpeed on ROCm without the hipify errors.

ROCm@a41829d
ROCm@663c718

cc: @jeffdaily

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Apr 20, 2022 •
edited
Loading

🔗 Helpful links

💊 CI failures summary and remediations

As of commit c576d2e (more details on the Dr. CI page):

Expand to see more

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

@ezyang ezyang requested a review from jeffdaily April 21, 2022 01:41
jeffdaily
jeffdaily requested changes Apr 21, 2022
View reviewed changes
Copy link
Collaborator

@jeffdaily jeffdaily left a comment

Choose a reason for hiding this comment

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

@rraminen please fix the lint errors reported by CI. Otherwise LGTM.

@albanD albanD self-requested a review April 25, 2022 17:22
@albanD albanD added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Apr 25, 2022
@albanD
Copy link
Collaborator

albanD commented Apr 27, 2022

The xla failure is unrelated right?
It would be best to do a quick rebase to make sure we have fresh CI (current one is a week old) before merging.

@rraminen
Copy link
Contributor Author

rraminen commented Apr 27, 2022 •
edited
Loading

Triggered a new CI run

@ezyang
Copy link
Contributor

ezyang commented Apr 28, 2022

@pytorchbot merge this

@pytorchmergebot
Copy link
Collaborator

Merge failed due to Matched rule superuser, but it was not reviewed yet by any of:kwen2501,ezyang,blefaudeux,Nitrokitty,larryliu0820, ...
Raised by https://github.com/pytorch/pytorch/actions/runs/2236850341

pruthvistony
pruthvistony approved these changes Apr 28, 2022
View reviewed changes
Copy link
Collaborator

@pruthvistony pruthvistony left a comment

Choose a reason for hiding this comment

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

LGTM

@ezyang
Copy link
Contributor

ezyang commented Apr 28, 2022

@pytorchbot merge this

@pytorchmergebot
Copy link
Collaborator

Merge failed due to Matched rule superuser, but it was not reviewed yet by any of:842974287,bhosmer,pvtuan10,ziky90,madhu-fb, ...
Raised by https://github.com/pytorch/pytorch/actions/runs/2239506466

@albanD
Copy link
Collaborator

albanD commented Apr 28, 2022

@pytorchbot merge this please

@github-actions
Copy link
Contributor

Hey @rraminen.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.

facebook-github-bot pushed a commit that referenced this pull request Apr 30, 2022
@rraminen @facebook-github-bot
Hipify fixes for a successful DeepSpeed build (#76141)
ae2c0d2 
Summary:
These commits are required to build DeepSpeed on ROCm without the hipify errors.

ROCm@a41829d
ROCm@663c718

GitHub CC:
jeffdaily

Pull Request resolved: #76141
Approved by: https://github.com/jeffdaily, https://github.com/pruthvistony, https://github.com/albanD

Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/7422ccea8b564132b50227b35a9bfbf06730d71a

Reviewed By: osalpekar

Differential Revision: D36042337

fbshipit-source-id: 629a6913ec7e6006bff1913b5eb6fd2feeb0f295
@shintaro-iwasaki shintaro-iwasaki mentioned this pull request May 3, 2022
Closed
shintaro-iwasaki added a commit to shintaro-iwasaki/pytorch that referenced this pull request May 3, 2022
@shintaro-iwasaki @facebook-github-bot
[PyTorch][AMD] fix hipify_python
c9dfd24 
Summary:
This PR fixes an issue in hipify_python introduced by pytorch#76141.

pytorch#76141 made all the `includes` paths "absolute", but this was not done for `args.extra_include_dir`; `new_dir`, which is a relative path, is directly added to `includes`. This PR fixes it by passing the absolute path (`abs_new_dir`).

Test Plan: CI

Differential Revision: D36089556

fbshipit-source-id: eb9c68b8d9a99dd5bdcf621ecdd6f18470f4321b
@albanD albanD mentioned this pull request May 3, 2022
Closed
facebook-github-bot pushed a commit that referenced this pull request May 3, 2022
@shintaro-iwasaki @facebook-github-bot
[PyTorch][AMD] fix hipify_python (#76720)
2ca6487 
Summary:
Pull Request resolved: #76720

This PR fixes an issue in hipify_python introduced by #76141.

#76141 made all the `includes` paths "absolute", but this was not done for `args.extra_include_dir`; `new_dir`, which is a relative path, is directly added to `includes`. This PR fixes it by passing the absolute path (`abs_new_dir`).

Test Plan: CI

Reviewed By: albanD

Differential Revision: D36089556

fbshipit-source-id: 1607075a4cb13696c1b25923f56b08a8cb3c6578
pytorchmergebot pushed a commit that referenced this pull request May 3, 2022
@shintaro-iwasaki @pytorchmergebot
[PyTorch][AMD] fix hipify_python (#76720)
20e4d6c 
Summary:
Pull Request resolved: #76720

This PR fixes an issue in hipify_python introduced by #76141.

#76141 made all the `includes` paths "absolute", but this was not done for `args.extra_include_dir`; `new_dir`, which is a relative path, is directly added to `includes`. This PR fixes it by passing the absolute path (`abs_new_dir`).

Test Plan: CI

Reviewed By: albanD

Differential Revision: D36089556

fbshipit-source-id: 1607075a4cb13696c1b25923f56b08a8cb3c6578
(cherry picked from commit 2ca6487)
This was referenced May 20, 2022
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeffdaily jeffdaily jeffdaily approved these changes

@albanD albanD albanD approved these changes

@pruthvistony pruthvistony pruthvistony approved these changes

@fmassa fmassa Awaiting requested review from fmassa

@soumith soumith Awaiting requested review from soumith

@ezyang ezyang Awaiting requested review from ezyang

Assignees

No one assigned

Labels

cla signed open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@rraminen @facebook-github-bot @albanD @ezyang @pytorchmergebot @jeffdaily @pruthvistony @pytorchbot @jithunnair-amd