Repository navigation
Make SDMA optional and explicitly enabled per Iris context - #568
Conversation
|
@drprajap Can we address the comments and merge this? Also needs the branch updated with main. |
mawad-amd
left a comment
There was a problem hiding this comment.
My final three comments. Thanks!
efric
left a comment
There was a problem hiding this comment.
Overall LGTM % some comments!
|
another nit: and can we update the description with some mention of why we'd like to keep copy engine initialization conditional (due to cost of queues as described in #579? |
Shader load/store users no longer need rocm-xio. It moves from the required dependencies to the sdma extra, and import iris no longer imports xio. iris.iris() / Iris() take enable_copy_engine (True/False/None). None reads IRIS_ENABLE_COPY_ENGINE (1/0/auto, strictly parsed); in auto mode SDMA is enabled only if rocm-xio is installed. With SDMA off, get_copy_engine_ctx() returns None and the host SDMA APIs raise RuntimeError. CI installs .[sdma]; external validation stays on the base install. SDMA tests skip when the copy engine is disabled. Signed-off-by: Prajapati, Dimple <[email protected]>
…tra cover it - Remove the env var and its parser; None (default) enables SDMA only if rocm-xio is installed. - Bind rocm-xio in test_sdma_packets.py and skip it without rocm-xio. - Trim the flag tests to the no-rocm-xio cases and rename to test_optional_sdma.py. - Docs: use --force-reinstall --no-deps for a standalone rocm-xio install. Signed-off-by: Prajapati, Dimple <[email protected]>
- Bind rocm-xio lazily only when copy_engine="sdma"; ImportError points to iris[sdma]. - Skip SDMA tests when rocm-xio is not installed. - test_optional_sdma.py: no-xio import, fail-fast, and default shader-store checks. - Docs: installation covers the extra, class reference covers copy_engine. Signed-off-by: Prajapati, Dimple <[email protected]>
Signed-off-by: Prajapati, Dimple <[email protected]>
engines argument, sdma_utils owns sdma_ep, simpler install docs Signed-off-by: Prajapati, Dimple <[email protected]>
run_core_tests.sh runs every tests/<dir>/test_*.py, including the SDMA tests, which skip without rocm-xio. Match run_example_slurm.sh. Signed-off-by: Prajapati, Dimple <[email protected]>
fb94689 to
edd4d1d
Compare
Yes, I will align the PR description as per latest consolidation |
|
Updated the title and description to match the consolidated API ( |
Shader stores already run on a default Iris context. Keep the import check and the host put/quiet rejection, and drop the extra copy-engine assert from the device-context test. Signed-off-by: Prajapati, Dimple <[email protected]>
e679481 to
638f3d0
Compare
mawad-amd
left a comment
There was a problem hiding this comment.
Looks great. Thanks for addressing the comments, and for your patience.
Only conflict was get_copy_engine_ctx's docstring: ROCm#568 added the -> torch.Tensor | None annotation and shortened the prose to one line, while this branch had expanded it as part of documenting the 11 undocumented public APIs. Kept the annotation and the fuller docstring, reworded to name engines=("sdma",) since ROCm#568 replaced copy_engine=.
Quote the return annotation on get_copy_engine_ctx. ROCm#568 added `-> torch.Tensor | None`, but torch is in autodoc_mock_imports, so the annotation evaluates mock | None at class-definition time and raises TypeError: unsupported operand type(s) for |: 'Tensor' and 'NoneType'. That killed the import of iris.host.iris and took 45 autodoc directives with it -- the API reference went empty again, by a different route than the xio import did. Quoting defers evaluation; it is the only PEP 604 annotation on a mocked type in iris/, and nothing introspects annotations at runtime. Defer the get_copy_engine_ctx index entry to class.md. ROCm#568 documents the method in its new "Optional SDMA Copy Engine" section and this branch already documented it on the dedicated copy-engine page, so Sphinx warned about a duplicate object description. Added :no-index: to this branch's copy rather than touching the section ROCm#568 just added. Verified on 3.13: 39 pages, 0 autodoc import failures, 4 warnings (all external intersphinx), and every page rendered in headless Chrome over HTTP -- no dead links, no broken images, no page errors beyond two the rocm-docs-core theme also throws on rocm.github.io/iris. jQuery errors per page: 4 upstream, 0 here.
…ssing anchors atomic_sub and quiet are iris.mem.triton.ops device functions, but sat on reference/triton/ops.md with the iris.ops host namespace because both surfaces are called "ops". Moved to device-functions.md -- atomic_sub with the other atomics, quiet under a new Synchronization heading. ops.md is now only the four fused host ops. "Free function" is C++ for a non-member function and means nothing in Python. Replaced with module-level function vs method, shown rather than described. :noindex: was copied from the Gluon page onto types.md and device-context.md, where nothing else documents those objects, so it suppressed their index anchors for no reason. Removed, 29 lines; the site index goes from 60 to 114 API objects. copy-engine.md keeps its one: ROCm#568 documents get_copy_engine_ctx on class.md too, so without it Sphinx warns about a duplicate object description. Checked by removing each one and rebuilding. Built on 3.13: 34 pages, 0 autodoc import failures, 0 duplicate objects.
Shader load/store, atomics, and collectives work without SDMA.
import irisno longer imports rocm-xio, and an Iris context does not create SDMA queues unless it asks for them.Consolidates #579. Queue setup stays conditional because each SDMA queue reserves 8 MiB of host/device memory, and the total grows with the number of local ranks. Shader-only callers, including TokenSpeed paths that are not on SDMA yet, should not pay that cost or take the rocm-xio dependency. On MI450 we also want SDMA off by default so CU and SDMA issues can be separated while it is brought up.
Behavior
Iris/iris.iris()takeengines, default(). Passengines=("sdma",)to create the host- and device-initiated SDMA queues. Any other name raisesValueError.get_copy_engine_ctx()returnsNone. Hostput,put_tile,put_tiles, andquietraiseRuntimeError.sdmaextra:pip install "iris[sdma]". It is imported only when"sdma"is inengines. If it is missing, that raisesModuleNotFoundErrorand points at the extra.engines.CI and docs
.[sdma].docs/getting-started/installation.mddocuments the extra.docs/reference/host/class.mddocumentsengines.Test plan
tests/unittests/test_optional_sdma.py:import irisdoes not loadxio; a default context does a cross-rank shader store,get_copy_engine_ctx()isNone, and hostput/quietraiseRuntimeError.engines=("sdma",)and skip when rocm-xio is not installed.Submission Checklist