Skip to content

Conversation

@jithunnair-amd
Copy link
Collaborator

@jithunnair-amd jithunnair-amd commented Oct 8, 2024

...due to pytorch/pytorch#137157

NOTE: This PR needs to be on-hold until the upstream hipify changes are re-merged, as they have been reverted in pytorch/pytorch@7e8dace since they broke Meta internal builds.

help="Source files to be excluded for hipify",
required=False)

parser.add_argument(
Copy link
Contributor

Choose a reason for hiding this comment

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

This PR removes some internal changes related to CuPy. Please don't remove it. This will impact other frameworks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for reviewing, @lcskrishna. I agree we should keep these changes. @liligwu Any reason to change hipify_cli.py? It doesn't even exist in PyTorch upstream, so it isn't about conflicts...

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for reviewing, @lcskrishna. I agree we should keep these changes. @liligwu Any reason to change hipify_cli.py? It doesn't even exist in PyTorch upstream, so it isn't about conflicts...

Yes, this file doesn't exist in Pytorch's hipify. but when I run the hipify_torch in FBGEMM, it reports "custom_map_list is not an argument ...", something like that. I have to remove all the statements related to custom_map_list, I think it's because of the API change in hipify_python.hipify()

Copy link
Contributor

Choose a reason for hiding this comment

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

@liligwu Interesting, just saw the codebase of fbgemm looks like its using CMake API. There were some changes after we added custom map in CMake way. I would recommend rebase your code with latest hipify_torch rather than removing it altogether. This might impact xformers as well.
cc: @jithunnair-amd

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am working on modifying the changes in this PR to reinstate some of the features we had added to hipify_torch. The custom_map_list was one of those features and it involved an addition of a keyword argument to hipify. It'll be part of the changes I'll reinstate

@lcskrishna
Copy link
Contributor

@jithunnair-amd You might have to add changes manually into this repo to sync with PT upstream as it contains some updates into this repo which are not updated to PT upstream.
cc: @amathews-amd

@jithunnair-amd jithunnair-amd changed the title Port changes from upstream PyTorch [DO NOT MERGE] Port changes from upstream PyTorch Oct 9, 2024
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