Skip to content

Conversation

@TedThemistokleous
Copy link
Contributor

-Add MIGraphX-EP page for install/example
-Update version info for ROCm version for both MIGraphx and ROCm EPs to 5.4 -Update hooks to the latest ROCm Pytorch supported (1.12.1 -> 1.13) -Remove (Preview) from MIGraphx and ROCm EP notes
-Update ROCm & MIGraphX EP.md files with ROCm version and pytorch links

Description

Update documentation about ROCm and MIGraphx with newest ROCm 5.4 stack

Motivation and Context

Update things for whats able to be supported.

-Add MIGraphX-EP page for install/example
-Update version info for ROCm version for both MIGraphx and ROCm EPs to 5.4
-Update hooks to the latest ROCm Pytorch supported (1.12.1 -> 1.13)
-Remove (Preview) from MIGraphx and ROCm EP notes
-Update ROCm & MIGraphX EP.md files with ROCm version and pytorch links
@TedThemistokleous
Copy link
Contributor Author

TedThemistokleous commented Dec 15, 2022

@cloudhan this is important since a lot of version info is outdated on these pages in terms of compatibility. Feel free to let us know if there's anything else required for these changes.

@cloudhan cloudhan requested review from PeixuanZuo and faxu December 16, 2022 05:25
@PeixuanZuo
Copy link
Contributor

We have some other "Preview" on https://onnxruntime.ai/, the file is https://github.com/microsoft/onnxruntime/blob/gh-pages/index.html#L243, Could you please remove it on this PR?

@PeixuanZuo PeixuanZuo requested a review from ytaous December 16, 2022 08:01
@ytaous
Copy link
Contributor

ytaous commented Dec 16, 2022

Can u please add preview link like so? #12971
Also, this would be good ref for where "other Preview" might be, thx.

@PeixuanZuo
Copy link
Contributor

Because of the issue(#13676), we don't have ROCm5.4 python package yet. It's better to merge this PR after we release ROCm5.4 nightly build python package. Thanks.

remove additional Preview tags from ROCm/MIGraphX
Update build support for pytorch
Update builds for ROCm & MIGraphX EP
@TedThemistokleous
Copy link
Contributor Author

@ytaous I think the ones for MIGraphX updates are #13564 or #9317

@PeixuanZuo Sounds good. Please let me know if there's anything else as well on this PR we should change before #13676 goes in.

@ytaous
Copy link
Contributor

ytaous commented Dec 17, 2022

@ytaous I think the ones for MIGraphX updates are #13564 or #9317

@PeixuanZuo Sounds good. Please let me know if there's anything else as well on this PR we should change before #13676 goes in.

Thanks, did u mean the staged view over here? https://faxu.github.io/onnxruntime/docs/ ? If so, as @PeixuanZuo pointed out, you will see "Preview" still there in some places. Might be good to do global search on the cloned repo as needed.
thx
image

@TedThemistokleous
Copy link
Contributor Author

@ytaous I think the ones for MIGraphX updates are #13564 or #9317
@PeixuanZuo Sounds good. Please let me know if there's anything else as well on this PR we should change before #13676 goes in.

Thanks, did u mean the staged view over here? https://faxu.github.io/onnxruntime/docs/ ? If so, as @PeixuanZuo pointed out, you will see "Preview" still there in some places. Might be good to do global search on the cloned repo as needed. thx image

@ytaous Saw the extra "Preview" tags and just removed them when doing a find replace in install/index.md.

@PeixuanZuo , I've also added the link for ROCm 5.4 here as well.

Please let me know if there is any other changes needed.

@ytaous
Copy link
Contributor

ytaous commented Jan 3, 2023

pls also click thru new links you created to make sure it doesn't lead to invalid pages, thx.


In reply to: 1369935611

Copy link
Contributor

@ytaous ytaous left a comment

Choose a reason for hiding this comment

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

pls also wait for review from Peixuan, thx.

@TedThemistokleous
Copy link
Contributor Author

TedThemistokleous commented Jan 4, 2023

@ytaous I'd like to hold off for another day or so, as we've just recently discovered issue #14126

I think there's a change that got in to mainline just before the holiday break that's broken builds, and it looks like there's a hole in your CI as tests are passing

It appears a change #13495 is what's caused this

@ytaous
Copy link
Contributor

ytaous commented Jan 4, 2023

@ytaous I'd like to hold off for another day or so, as we've just recently discovered issue #14126

I think there's a change that got in to mainline just before the holiday break that's broken builds, and it looks like there's a hole in your CI as tests are passing

It appears a change #13495 is what's caused this

@cloudhan and @PeixuanZuo

@PeixuanZuo
Copy link
Contributor

@ytaous I'd like to hold off for another day or so, as we've just recently discovered issue #14126

I think there's a change that got in to mainline just before the holiday break that's broken builds, and it looks like there's a hole in your CI as tests are passing

It appears a change #13495 is what's caused this

Can we continue this PR if the issue is fixed?

@TedThemistokleous
Copy link
Contributor Author

TedThemistokleous commented Jan 17, 2023

@ytaous I'd like to hold off for another day or so, as we've just recently discovered issue #14126
I think there's a change that got in to mainline just before the holiday break that's broken builds, and it looks like there's a hole in your CI as tests are passing
It appears a change #13495 is what's caused this

Can we continue this PR if the issue is fixed?

Issue is fixed and confirmed builds are running on our end. I'm finishing up CI and have seen things pass of main branch from last week. This PR should be good to add in.

@ytaous
Copy link
Contributor

ytaous commented Jan 17, 2023

@faxu

@@ -0,0 +1,77 @@
---
Copy link
Contributor

Choose a reason for hiding this comment

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

There is already a page for this EP here: https://onnxruntime.ai/docs/execution-providers/community-maintained/MIGraphX-ExecutionProvider.html

Can/should it be consolidated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this to mirror what was done for the ROCm EP documentation. Should we be removing the other then is what you're saying?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consolidated info to this document page instead and removed the one under community-maintained.

@faxu
Copy link
Contributor

faxu commented Jan 18, 2023

@TedThemistokleous Could you stage this page for preview to more easily confirm formatting is correct? See instructions here: https://github.com/microsoft/onnxruntime/wiki/Update-ONNX-Runtime-Docs#preview-change

Copy link
Contributor

@faxu faxu left a comment

Choose a reason for hiding this comment

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

Please address comment for consolidation of the EP page and stage a preview of the docs.

Remove previous iteration of MIGraphX page, and consolidate info at the same level
as the ROCm EP's documentation.
previous EP page consoldation resulted in this broken link.
@TedThemistokleous
Copy link
Contributor Author

@faxu update once your CI found a broken link. This should be good to go now

@faxu
Copy link
Contributor

faxu commented Jan 19, 2023

@TedThemistokleous
Copy link
Contributor Author

@TedThemistokleous Please stage this for preview. See instructions here: https://github.com/microsoft/onnxruntime/wiki/Update-ONNX-Runtime-Docs#preview-change

Hi @faxu I've done this here's the link: https://tedthemistokleous.github.io/onnxruntime/

Oddly I never got an email when this completed. Please let me know if there's anything else required on my end. Thank you for your feedback!


||Official build (location)|Nightly build (location)|
|---|---|---|
|PyTorch 1.8.1 (CUDA 10.2)|[**onnxruntime_stable_torch181.cu102**](https://onnxruntimepackages.z14.web.core.windows.net/onnxruntime_stable_torch181.cu102.html)|[onnxruntime_nightly_torch181.cu102](https://onnxruntimepackages.z14.web.core.windows.net/onnxruntime_nightly_torch181.cu102.html)|
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this intended to remove the CUDA builds? @ytaous / @PeixuanZuo

|[TVM](../execution-providers/community-maintained/TVM-ExecutionProvider.md) (*preview*)|[DirectML](../execution-providers/DirectML-ExecutionProvider.md)|[Android Neural Networks API](../execution-providers/NNAPI-ExecutionProvider.md)|[Huawei CANN](../execution-providers/community-maintained/CANN-ExecutionProvider.md) (*preview*)|
|[Intel OpenVINO](../execution-providers/OpenVINO-ExecutionProvider.md)|[AMD MIGraphX](../execution-providers/community-maintained/MIGraphX-ExecutionProvider.md) (*preview*)|[ARM-NN](../execution-providers/community-maintained/ArmNN-ExecutionProvider.md) (*preview*)|
||[AMD ROCm](../execution-providers/ROCm-ExecutionProvider.md) (*preview*)|[CoreML](../execution-providers/CoreML-ExecutionProvider.md) (*preview*)|
|[Intel OpenVINO](../execution-providers/OpenVINO-ExecutionProvider.md)|[AMD MIGraphX](../execution-providers/MIGraphX-ExecutionProvider.md)|[ARM-NN](../execution-providers/community-maintained/ArmNN-ExecutionProvider.md) (*preview*)|
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment about "preview" as on the main matrix page.
Also, could you help fix the position of "XNNPACK" in the CPU column? There's a 3-row gap. (not due to your change, but just noticed it)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure let me add this in

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done let me know if it looks good

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the XNNPACK originally listed in the IoT/Edge/Mobile column got shifted accidentally. Can you fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@faxu
Copy link
Contributor

faxu commented Jan 25, 2023

Please also fix conflicts on the branch, thanks!

- Add back in table of contents placeholder in MIGraphX EP doc
-Rearrange order of the EPs for ROCm and MIgraphX
-fix the issue with XNNPACK having 3 columns below.
@TedThemistokleous
Copy link
Contributor Author

TedThemistokleous commented Jan 25, 2023

@faxu I don't seem to have write access to this repo to be able to fix said conflicts, nor can I see them when l take a look at the code in the PR on github.

@faxu, fixed changes and merged the head of this to fix conflicts. Let me know if you have any additional issues or changes.

@faxu
Copy link
Contributor

faxu commented Jan 26, 2023

@faxu I don't seem to have write access to this repo to be able to fix said conflicts, nor can I see them when l take a look at the code in the PR on github.

@faxu, fixed changes and merged the head of this to fix conflicts. Let me know if you have any additional issues or changes.

Thanks @TedThemistokleous! Just one more fix on the execution provider index page, otherwise looks good!

@TedThemistokleous
Copy link
Contributor Author

Done @faxu. Let me know if you need anything else.

@faxu faxu merged commit 5601b10 into microsoft:gh-pages Jan 26, 2023
@TedThemistokleous
Copy link
Contributor Author

TedThemistokleous commented Jan 27, 2023

Hi @faxu @ytaous @PeixuanZuo is there something wrong with these changes? It looks like this was somehow backed out as the site doesn't reflect these changes anymore.

@PeixuanZuo
Copy link
Contributor

Hi @faxu @ytaous @PeixuanZuo is there something wrong with these changes? It looks like this was somehow backed out as the site doesn't reflect these changes anymore.

There are some changes after this PR and lead to unchanged on https://onnxruntime.ai/. I will fix it.

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