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.

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: enable cpp_extensions.load/load_inline #35897

Closed
wants to merge 11 commits into from

Conversation

t-vi
Copy link
Collaborator

@t-vi t-vi commented Apr 2, 2020

This enables cpp_extensions.load/load_inline. This works by hipify-ing cuda sources.
Also enable tests.
CuDNN/MIOpen extensions aren't yet supported, I propose to not do this in this PR.

@dr-ci
Copy link

dr-ci bot commented Apr 2, 2020

💊 CircleCI build failures summary and remediations

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


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


This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker.

See how this bot performed.

This comment has been revised 32 times.

@t-vi
Copy link
Collaborator Author

t-vi commented Apr 3, 2020

From what I can see, the errors seem to be mostly network connectivity rather than what happens in the relevant tests.

torch/utils/cpp_extension.py Outdated Show resolved Hide resolved
torch/utils/cpp_extension.py Outdated Show resolved Hide resolved
torch/utils/cpp_extension.py Outdated Show resolved Hide resolved
Copy link
Contributor

@ezyang ezyang left a comment

Choose a reason for hiding this comment

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

some minor bugs

@t-vi
Copy link
Collaborator Author

t-vi commented Apr 3, 2020

Thank you for the prompt review, @ezyang !
I changed to keep the intermediates, that also gets rid of some of the other uglyness.
Fixed the other bits.

@zou3519 zou3519 added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Apr 6, 2020
@t-vi
Copy link
Collaborator Author

t-vi commented Apr 7, 2020

So if you're OK with keeping the generated hip (after a bit more thought, I think it is preferable), this should have the things fixed.

torch/utils/cpp_extension.py Outdated Show resolved Hide resolved
@t-vi
Copy link
Collaborator Author

t-vi commented Apr 10, 2020

It the commentary OK now?

@ezyang
Copy link
Contributor

ezyang commented Apr 10, 2020

Lint.


  {
    path: 'torch/utils/hipify/hipify_python.py',
    start_line: 823,
    end_line: 823,
    start_column: 17,
    end_column: 17,
    annotation_level: 'failure',
    message: '[B006] Do not use mutable data structures for argument defaults.  They are created during function definition time. All calls to the function reuse this one instance of that data structure, persisting changes between them.'
  }

…tuable default arguments but who cares about nuances
@t-vi
Copy link
Collaborator Author

t-vi commented Apr 10, 2020

Oh. Sorry about that. 🤦‍♂️

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@ezyang is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@ezyang merged this pull request in d070c0b.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merged open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants