Improve the artifact machinery - #1035
Conversation
simeonschaub
left a comment
simeonschaub
left a comment
There was a problem hiding this comment.
Thanks! Adding an arch option to AMDGPU.set_rocm_version! makes sense to me
|
|
||
| The artifact is chosen from the GPU architectures detected on the host, which on Linux are read from the `/sys/class/kfd/kfd/topology` nodes. If nothing is detected — a container without KFD passthrough, a login node, or a host where the `amdgpu` driver is not loaded — or if the detected architecture is not one of the shipped bundles, then no artifact is downloaded and AMDGPU.jl reports its ROCm components as unavailable. | ||
|
|
||
| This is not a fallback: an existing system-wide ROCm is only picked up after explicitly opting in with the `local` preference above. The bundles currently shipped cover `gfx908`, `gfx90a`, `gfx94x` and `gfx950` for Instinct, and `gfx101x`, `gfx103x`, `gfx110x`, `gfx1150` through `gfx1153` and `gfx120x` for Radeon. |
There was a problem hiding this comment.
| This is not a fallback: an existing system-wide ROCm is only picked up after explicitly opting in with the `local` preference above. The bundles currently shipped cover `gfx908`, `gfx90a`, `gfx94x` and `gfx950` for Instinct, and `gfx101x`, `gfx103x`, `gfx110x`, `gfx1150` through `gfx1153` and `gfx120x` for Radeon. | |
| Due to limitations of the artifact system, there is no fallback path. An existing system-wide ROCm is only picked up after explicitly opting in with the `local` preference above. The bundles currently shipped cover `gfx908`, `gfx90a`, `gfx94x` and `gfx950` for Instinct (Linux only), and `gfx101x`, `gfx103x`, `gfx110x`, `gfx1150` through `gfx1153` and `gfx120x` for Radeon. |
Though I wonder whether there still might be a way to implement a fallback. Can always be added later though
There was a problem hiding this comment.
Good point. I'll check that. Also, I may trim the explicit GPU names to not get ourselves locked into docs that deprecate too quickly.
There was a problem hiding this comment.
Note, the artifact system is not what prevents a fallback. The provider is chosen at load time by a compile-time preference that decides which package gets using'd in src/libs.jl, so by the time resolution fails there is nothing to fall back to; a fallback would need both packages loaded and the choice deferred to runtime.
Also, I now read the arch list from Artifacts.toml which is the source of truth. It also derives the Linux-only fact from the data. Seems more robust.
| Pkg.update() | ||
| Pkg.test(; coverage=true)' | ||
| Pkg.update()' | ||
| julia --project -e 'using Preferences; set_preferences!(Base.UUID("3129f4d2-de71-4ff3-9833-76037e3ea355"), "local" => "false"; force=true)' |
There was a problem hiding this comment.
It might be easier to just write the LocalPreferences.toml file directly? We might also need to set this before calling Pkg.update(), otherwise won't we be downloading the artifacts anyways? (Same below)
There was a problem hiding this comment.
Seems every entry in ROCm_Runtime/Artifacts.toml is lazy = true, and the package uses LazyArtifacts + @artifact_str at __init__. So Pkg.update() downloads nothing. The fetch happens on the first using AMDGPU. Setting the pin after Pkg.update() but before any load looks sufficient.
| Pkg.test(; coverage=true)' | ||
| Pkg.update()' | ||
| julia --project -e 'using Preferences; set_preferences!(Base.UUID("3129f4d2-de71-4ff3-9833-76037e3ea355"), "local" => "false"; force=true)' | ||
| julia --project -e 'using AMDGPU; AMDGPU.versioninfo(); AMDGPU.local_rocm && error("this step selected the ROCm artifacts, but AMDGPU resolved to a local ROCm")' |
There was a problem hiding this comment.
Running tests prints the versioninfo anyways and that already shows whether we are using the jll or not, so I don' t think this line is needed (below as well)
There was a problem hiding this comment.
Good point. We can drop it but I would maybe keep the assertion for now just to be sure
|
@simeonschaub no more changes on these points from my side - happy to address any further comments you may have but too me it looks this is a fairly robust handling to possibly be further improved. |
simeonschaub
changed the title
|
LGTM, feel free to merge into my branch once CI comes back (does MI300 CI need to be manually retriggered?). Might want to squash merge this, but I'll leave that up to you, if you'd rather keep the commit history here |
luraess
changed the title
|
I can squash merge. Unfortunately MI300 can only be triggered when targeting main. It will be upon merging into |
|
Ah, in that case we can also just merge right away |
2 participants
Follow-ups to #1028 around the artifact/preference machinery.
LocalPreferences.toml) and asserts what AMDGPU actually resolved to, so a change of default cannot silently swap the ROCm underneath a job. Adds aJulia 1.12 (local ROCm)step so both paths are covered.set_rocm_version!(; arch)exposes the existingarchpreference, previously only settable by hand-editingLocalPreferences.toml;reset_rocm_version!clears it.[ROCm_Runtime]preferences (local,version,arch); clarify whenROCM_PATH,DEVICE_LIB_PATHandHIP_DEVICE_LIB_PATHapply. Generate the shipped-bundle list fromArtifacts.tomlso it cannot rot.JULIA_AMDGPU_DISABLE_ARTIFACTS(unread since Remove artifacts support & fix rocSPARSE bug #674) andJULIA_AMDGPU_LOGGING_ENABLED(unread since Cleanup logging #492) from the pipeline.