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

Force hipify to use copied version from rocm-6.3.0-14776 build #69

Merged
merged 3 commits into from
Oct 10, 2024

Conversation

TedThemistokleous
Copy link

Description

Copied out hipifiy-perl from working docker container as mentioned by @xinyazhang
Retargeted amd_hipify to use the copied item found in onnxruntime root

Motivation and Context

Changed as an upstream change to the rocm stack requires a bunch of work required to get a working rebuild of ROCm EP.
Currently this inhibits development and blocks 6.3 wheel builds

Copy link

@causten causten left a comment

Choose a reason for hiding this comment

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

Does this need to be merged upstream? Would the path for hipify-perl be appropriate? Seems like you are suggesting it belongs in the root.

Copy link

@xinyazhang xinyazhang left a comment

Choose a reason for hiding this comment

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

hipify-perl should be placed under the same directory of amd_hipify.py rather than root of the source tree, unless there is really good reason.

@xinyazhang
Copy link

Does this need to be merged upstream? Would the path for hipify-perl be appropriate? Seems like you are suggesting it belongs in the root.

This is a temporary solution and we are working on another PR that eliminates -roc argument from

s = subprocess.run([hipify_perl_path, "-roc", src_file_path], stdout=subprocess.PIPE, text=True, check=False).stdout

, which should work for newer hipify-perl

@TedThemistokleous
Copy link
Author

hipify-perl should be placed under the same directory of amd_hipify.py rather than root of the source tree, unless there is really good reason.

Done. Just retesting this. and will update once done.

@TedThemistokleous
Copy link
Author

Does this need to be merged upstream? Would the path for hipify-perl be appropriate? Seems like you are suggesting it belongs in the root.

No it shouldn't for now. The plan is to revert this once the changes for hipify are done here for the ROCm EP

@TedThemistokleous
Copy link
Author

Moved it and had to update permissions. Looks like its built and working minus some other test thats failed

image

Copy link

@xinyazhang xinyazhang left a comment

Choose a reason for hiding this comment

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

Do not use absolute path. Compute it in your amd_hipify.py

@@ -205,4 +205,4 @@ def hipify(hipify_perl_path, src_file_path, dst_file_path):
parser.add_argument("src", help="src")
args = parser.parse_args()

hipify(args.hipify_perl, args.src, args.output)
hipify("/onnxruntime/tools/ci_build/hipify-perl", args.src, args.output)

Choose a reason for hiding this comment

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

Use

hipify(os.path.join(os.path.dirname(__file__), "hipify-perl"), args.src, args.output)

Instead

Copy link
Author

@TedThemistokleous TedThemistokleous Oct 10, 2024

Choose a reason for hiding this comment

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

Done - retested this and works

@TedThemistokleous TedThemistokleous merged commit ad60c03 into rocm6.3_internal_testing Oct 10, 2024
10 of 15 checks passed
@TedThemistokleous TedThemistokleous deleted the retarget_hipify_tool branch October 10, 2024 21:16
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.

3 participants