Rename miopen-plugin to miopenprovider and use new artifacts#3361
Rename miopen-plugin to miopenprovider and use new artifacts#3361SamuelReeder wants to merge 8 commits intomainfrom
Conversation
…-plugin-move # Conflicts: # docs/development/windows_support.md
SamuelReeder
force-pushed
the
users/sareeder/miopen-plugin-move
branch
from
c5ec723 to
1443658
Compare
Abstract the MIOpen provider artifact name to miopenprovider across TheRock build infrastructure. The underlying rocm-libraries targets retain their original miopen_plugin naming internally. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
SamuelReeder
force-pushed
the
users/sareeder/miopen-plugin-move
branch
from
1443658 to
cc18b17
Compare
SamuelReeder
requested review from
BrianHarrisonAMD,
adickin-amd and
mousdahl-amd
| [components.test."ml-libs/miopenprovider/stage"] | ||
| include = [ | ||
| "bin/miopen_plugin*_tests", | ||
| "bin/miopen_plugin/CTestTestfile.cmake", |
There was a problem hiding this comment.
| "bin/miopen_plugin/CTestTestfile.cmake", | |
| "bin/miopenprovider/CTestTestfile.cmake", |
If possible.
There was a problem hiding this comment.
We decided to leave the artifacts as miopen_plugin, but the umbrella naming as miopenprovider. It's one word in TheRock so we can avoid switching between underscores and dashes.
There was a problem hiding this comment.
It's a little confusing, but this is how we've named it in rocm-libraries too.
There was a problem hiding this comment.
Huh, it's also problematic that hipblaslt uses our old naming everywhere (e.g. HIPBLASLT_PLUGIN, hipBLASlt_plugin).
| | `-DTHEROCK_ENABLE_SPARSE=ON` | Enables the SPARSE libraries | | ||
| | `-DTHEROCK_ENABLE_MIOPEN=ON` | Enables MIOpen | | ||
| | `-DTHEROCK_ENABLE_MIOPEN_PLUGIN=ON` | Enables MIOpen_plugin | | ||
| | `-DTHEROCK_ENABLE_MIOPENPROVIDER=ON` | Enables MIOpen provider | |
There was a problem hiding this comment.
This is the part I'm mildly concerned about where it becomes ambiguous which library the plugin is for. To remove the chance for confusion, consider:
| | `-DTHEROCK_ENABLE_MIOPENPROVIDER=ON` | Enables MIOpen provider | | |
| | `-DTHEROCK_ENABLE_HIPDNN_MIOPENPROVIDER=ON` | Enables hipDNN MIOpen-provider plug-in. |
I'm ambivalent on whether or not this define should be grouped with MIOpen vs. hipdnn though, but I'm leaning towards having this define described with the hipDNN options.
| "ctest", | ||
| "--test-dir", | ||
| f"{THEROCK_BIN_DIR}/miopen_legacy_plugin", | ||
| f"{THEROCK_BIN_DIR}/miopen_plugin", |
There was a problem hiding this comment.
Do we have to make another change in rocm-libraries to make this
| f"{THEROCK_BIN_DIR}/miopen_plugin", | |
| f"{THEROCK_BIN_DIR}/miopenprovider", |
And in a bit of hindisght, maybe this might be better moving forward:
| f"{THEROCK_BIN_DIR}/miopen_plugin", | |
| f"{THEROCK_BIN_DIR}/dnnproviders/miopenprovider", |
AFAI.
There was a problem hiding this comment.
The CTest folder might make sense to be named miopenprovider, but generally decided on having the artifacts named as miopen_plugin.
We would have to make another rocm-libraries change, so I think we're left with this.
2 participants
Motivation
The MIOpen plugin artifacts were renamed in rocm-libraries. This PR updates TheRock's build infrastructure to reflect the new naming, using
miopenprovideras the canonical artifact and subproject name.Technical Details
miopen-plugin→miopenproviderinBUILD_TOPOLOGY.tomltherock_provide_artifactinml-libs/CMakeLists.txtartifact-miopenprovider.toml) and post-hook (post_hook_miopenprovider.cmake)test_miopen_plugin.py→test_miopenprovider.pyREADME.mdCMake flag reference toTHEROCK_ENABLE_MIOPENPROVIDERTest Plan
miopenproviderartifactsSubmission Checklist