Claim the bundle's vendor libraries, and drop the rocSPARSE version probe - #1063
Merged
Conversation
Member
|
I'm really not a big fan of introducing even more complexity just for the librocsparse version check. With #1028, we might want to revisit whether the check is truly needed anymore, since now the user would have to explicitly request AMDGPU.jl to use a broken system ROCm install for this to matter. I personally think it's not too unexpected that Julia would crash in such a situation, so I'd propose reverting #1001 altogether as part of #1028 |
Member
Author
|
Yeah, that's the other alternative, to remove all this machinery with the one caveat is that we may get inaccurate version for rocSPARSE if we ever encounter using it from system. I can do it here. |
Member
Author
|
This is now mostly rolling back #1001 but adding some guards. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
EDIT: rolled-back most of #1001
In artifact mode on a host with another ROCm on
LD_LIBRARY_PATH(here a CSCS uenv),versioninfo()reported rocSPARSE aserrand the core test asserting the probe returns a version failed. The cause is not rocSPARSE.PRELOAD_LIBRARIESclaimed the runtime closure but no vendor library, and AMDGPU claims those lazily on the firstccall, so whichever is touched first wins. TheRock's libraries carryDT_RUNPATH, whichLD_LIBRARY_PATHoutranks, so touching rocSPARSE first pulled its dependencies from the environment. Measured in artifact mode afterrocSPARSE.version():FOREIGN_LIBRARY_NAMESlisted only runtime libraries, soversioninfo()printed "Foreign ROCm libraries loaded: none" while two rocBLAS versions were live in the process.The isolated probe from #1051 hit the same thing: a bare child has claimed nothing at all, so it resolved the bundle's
librocsparseagainst uenv HIP 7.2.3 androcsparse_create_handlesegfaulted, which is what surfaced aserr.Changes
PRELOAD_LIBRARIESalso claimslibamdhip64,libroctx64,libhipblasltandlibrocblas— the vendor libraries that other vendor libraries pull in (librocsparseneedslibrocblasandlibroctx64;librocblasneedslibhipblaslt). Order is load-bearing: HSA must precede HIP, or HIP'slibhsa-runtime64.so.1binds outside the bundle and the dlopen fails withundefined symbol: hsa_amd_queue_create, version ROCR_1.FOREIGN_LIBRARY_NAMESgains the vendor libraries, so a mixed process is reported instead of passing as clean._version_subprocess,_rocsparse_version_isolated, the cache global and its__init__reset). With the libraries claimed correctly there is nothing left for it to protect against, and it was a lot of machinery for one cell of a table.versioninfoqueries rocSPARSE in-process like every other library. The shared_verhelper now guards all of them, so a failing query showserrinstead of taking down the whole report — previously none were guarded.Result
On beverin (2x MI300A, artifact ROCm under uenv), before -> after:
versioninforocSPARSEerr+ misleading warning5.0.0librocblas/libroctx64/libhipblasltforeign_libraries()String[](wrong)String[](correct)Trade-off
A SIGSEGV inside
rocsparse_create_handle(#920) again takes down the session rather than degrading toerr. Two things make that acceptable: the preload fix removes the mixed-library cause that produced the crash here, andversioninfoalready initialises HIP and enumerates devices in-process, so a genuinely broken install fails before the rocSPARSE row either way.try/catchstill covers every non-fatal failure.