Skip to content

Rocm ep acc test support#13306

Closed
groenenboomj wants to merge 9 commits into
microsoft:mainfrom
ROCm:rocm_ep_acc_test_support
Closed

Rocm ep acc test support#13306
groenenboomj wants to merge 9 commits into
microsoft:mainfrom
ROCm:rocm_ep_acc_test_support

Conversation

@groenenboomj
Copy link
Copy Markdown
Contributor

Description

Add support for ROCm EP in accuracy tests

Motivation and Context

This allows ONNXRT accuracy tests to be ran on the ROCm EP.

import onnxruntime

if use_gpu and "CUDAExecutionProvider" not in onnxruntime.get_available_providers():
if use_gpu and "CUDAExecutionProvider" and "ROCMExecutionProvider" not in onnxruntime.get_available_providers():
Copy link
Copy Markdown
Contributor

@cloudhan cloudhan Oct 14, 2022

Choose a reason for hiding this comment

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

address py lint "CUDAExecutionProvider" and "ROCMExecutionProvider"

Comment thread onnxruntime/python/tools/transformers/optimizer.py Outdated
assert "CUDAExecutionProvider" or "ROCMExecutionProvider" in session.get_providers() # Make sure there is GPU

assert os.path.exists(optimized_model_path) and os.path.isfile(optimized_model_path)
logger.debug("Save optimized model by onnxruntime to {}".format(optimized_model_path))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

py lint. This can be changed to logger.debug("Save optimized model by onnxruntime to %s", optimized_model_path) to delay the string formation.

@lgtm-com
Copy link
Copy Markdown

lgtm-com Bot commented Oct 20, 2022

This pull request fixes 1 alert when merging 2b6e010 into 7a3486c - view on LGTM.com

fixed alerts:

  • 1 for Unused local variable

@lgtm-com
Copy link
Copy Markdown

lgtm-com Bot commented Oct 21, 2022

This pull request fixes 1 alert when merging 8561cd8 into 928c988 - view on LGTM.com

fixed alerts:

  • 1 for Unused local variable

onnx_model_path, sess_options, providers=gpu_ep, **kwargs
)
assert "CUDAExecutionProvider" in session.get_providers() # Make sure there is GPU
assert any(gpu_ep in session.get_providers() for gpu_ep in ["CUDAExecutionProvider","ROCMExecutionProvider"]) # Make sure there is GPU
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remove this line since I think session creation will fail if no CUDA or Rocm EP, and line 76 has checked.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Tested and removed. I agree.

@groenenboomj groenenboomj force-pushed the rocm_ep_acc_test_support branch from 8561cd8 to 5c4e255 Compare November 2, 2022 21:46
@lgtm-com
Copy link
Copy Markdown

lgtm-com Bot commented Nov 2, 2022

This pull request introduces 1 alert when merging 5c4e255 into b1e1b25 - view on LGTM.com

new alerts:

  • 1 for Unused local variable

tianleiwu
tianleiwu previously approved these changes Nov 18, 2022
@groenenboomj groenenboomj marked this pull request as ready for review November 18, 2022 16:59
@tianleiwu
Copy link
Copy Markdown
Contributor

/azp run Windows CPU CI Pipeline,Windows GPU CI Pipeline,Windows GPU TensorRT CI Pipeline,ONNX Runtime Web CI Pipeline,Linux CPU CI Pipeline,Linux CPU Minimal Build E2E CI Pipeline,Linux GPU CI Pipeline,Linux GPU TensorRT CI Pipeline,Linux OpenVINO CI Pipeline,MacOS CI Pipeline

@tianleiwu
Copy link
Copy Markdown
Contributor

/azp run orttraining-amd-gpu-ci-pipeline,orttraining-linux-ci-pipeline,orttraining-linux-gpu-ci-pipeline,orttraining-ortmodule-distributed,onnxruntime-python-checks-ci-pipeline,onnxruntime-binary-size-checks-ci-pipeline

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 6 pipeline(s).

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 10 pipeline(s).

Add GPU environment check.
@tianleiwu

This comment was marked as resolved.

@tianleiwu

This comment was marked as resolved.

@azure-pipelines

This comment was marked as resolved.

@azure-pipelines

This comment was marked as resolved.

@lgtm-com
Copy link
Copy Markdown

lgtm-com Bot commented Nov 18, 2022

This pull request fixes 1 alert when merging 75ac09b into 3e9e5e9 - view on LGTM.com

fixed alerts:

  • 1 for Unused local variable

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. It looks like GitHub code scanning with CodeQL is already set up for this repo, so no further action is needed 🚀. For more information, please check out our post on the GitHub blog.

@lgtm-com
Copy link
Copy Markdown

lgtm-com Bot commented Nov 18, 2022

This pull request fixes 1 alert when merging 2461ad6 into 3e9e5e9 - view on LGTM.com

fixed alerts:

  • 1 for Unused local variable

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. It looks like GitHub code scanning with CodeQL is already set up for this repo, so no further action is needed 🚀. For more information, please check out our post on the GitHub blog.

@cloudhan

This comment was marked as resolved.

@azure-pipelines

This comment was marked as resolved.

@cloudhan

This comment was marked as resolved.

@azure-pipelines

This comment was marked as resolved.

@cloudhan

This comment was marked as resolved.

@azure-pipelines

This comment was marked as resolved.

@azure-pipelines

This comment was marked as resolved.

assert (
"CUDAExecutionProvider" in onnxruntime.get_available_providers()
), "Please install onnxruntime-gpu package to test GPU inference."
assert any(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please use set disjoint test for readibility:

set(onnxruntime.get_available_providers()).isdisjoint(["CUDAExecutionProvider", "ROCMExecutionProvider"])

import onnxruntime

if use_gpu and "CUDAExecutionProvider" not in onnxruntime.get_available_providers():
if use_gpu and not any(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same here

gpu_ep.append("ROCMExecutionProvider")

session = onnxruntime.InferenceSession(onnx_model_path, sess_options, providers=gpu_ep, **kwargs)
assert any(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and here

cloudhan pushed a commit that referenced this pull request Nov 27, 2022
Allows MIGraphX EP to run the following additional tests. Also adds support to get MIGraphX to run eval_squad.py

Reference to the Rocm EP changes: #13306

Co-authored-by: Joseph Groenenboom <joseph.groenenboom@amd.com>
Co-authored-by: Ted Themistokleous <tthemist@amd.com>
@groenenboomj
Copy link
Copy Markdown
Contributor Author

The changes in this PR were rolled into and merged with #13455. Closing.

groenenboomj added a commit to ROCm/onnxruntime that referenced this pull request Dec 8, 2022
Allows MIGraphX EP to run the following additional tests. Also adds support to get MIGraphX to run eval_squad.py

Reference to the Rocm EP changes: microsoft#13306

Co-authored-by: Joseph Groenenboom <joseph.groenenboom@amd.com>
Co-authored-by: Ted Themistokleous <tthemist@amd.com>
simon-moo pushed a commit to simon-moo/onnxruntime that referenced this pull request Dec 26, 2022
Allows MIGraphX EP to run the following additional tests. Also adds support to get MIGraphX to run eval_squad.py

Reference to the Rocm EP changes: microsoft#13306

Co-authored-by: Joseph Groenenboom <joseph.groenenboom@amd.com>
Co-authored-by: Ted Themistokleous <tthemist@amd.com>
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