Fix CUB imports for CUDA and CMake#31631
Conversation
spackbot-app
bot
added
build-environment
core
scheibelp
left a comment
scheibelp
left a comment
There was a problem hiding this comment.
This overall looks good: I have a couple minor requests about comments. I have to think more about what to suggest regarding my comment about the side effects of modifications_for_dep (ideally running that function to collect CMAKE_PREFIX_PATH modifications wouldn't also do dpkg.setup_dependent_package(spec.package.module, spec))
lib/spack/spack/build_environment.py
Outdated
| # normal circumstances, it has to come from spack. include these second | ||
| # after the spack_built dependencies but before externals | ||
| env_mod_list = modifications_from_dependencies(pkg.spec, 'build', | ||
| custom_mods_only=True) |
There was a problem hiding this comment.
This should also set set_package_py_globals=False.
With that, and if modifications_for_dep skipped dpkg.setup_dependent_package(spec.package.module, spec), this would have no side effects (but I'd need to think about what to suggest to skip the latter - maybe another parameter? That might be confusing for the interface).
There was a problem hiding this comment.
@scheibelp this will be my first (second? not sure) contribution to spack core. Other than adding the argument you suggested I don't have context to understand what you want me to do here.
There was a problem hiding this comment.
Sorry yes I need to think on it: my first naive suggestion is that we would need another parameter like set_package_py_globals which (when set =False) would skip dpkg.setup_dependent_package(spec.package.module, spec), but I'm thinking that having so many parameters to control small parts of the function's behavior will make it confusing to understand, so I'm thinking if I should work on refactoring it (which I could as a commit here if that's alright with you, but I wouldn't have time to do that until next week).
|
@scheibelp Sorry for the delay on this. I have pushed changes to the comments, and added a doc string. Please let me know if you want any more changes. |
|
@scheibelp ping |
|
@scheibelp This is blocking a fix for cuSZ being submitted. Is there a timeline on this being merged? |
There was a problem hiding this comment.
I looked at this more and the side effects I am concerned about in #31631 (comment) should not be a problem. But I still think a refactor is in order, so I have added a suggestion for a TODO comment here. If you choose to commit it and the tests pass (assuming I formatted this right in the Github UI) then I'll merge it.
|
@scheibelp i can’t tell from the build log if that failure was spurious. However since it depends on Cuda, I don’t want to take a chance. How can we debug this failure? |
It looks like the log for the failed package (mfem) is truncated - I'm checking if there's a way we can extend it. I notice a couple of files are out of date - apologies for not noticing that before but FYI this will need to merge-from/rebase-on develop as well. It's possible that the errors will be resolved (if not though, we'll want to get the full log). |
|
I think I know how to get the full logs, but I suspect that since the latest run was a long time ago that they were discarded, so if the problem comes up again after a merge/rebase, I can see where the mfem build went wrong. |
robertu94
force-pushed
the
cuda_cmake_fix
branch
from
214adab to
196c0d7
Compare
|
(note you can run |
|
@spackbot fix style |
|
Let me see if I can fix that for you! |
|
I was able to run spack style --fix==> Running style checks on spack
selected: isort, mypy, black, flake8
==> Modified files
lib/spack/spack/build_environment.py
var/spack/repos/builtin/packages/cuda/package.py
==> Running isort checks
isort checks were clean
==> Running mypy checks
Success: no issues found in 558 source files
mypy checks were clean
==> Running black checks
reformatted var/spack/repos/builtin/packages/cuda/package.py
reformatted lib/spack/spack/build_environment.py
All done! ✨ 🍰 ✨
2 files reformatted.
black checks were clean
==> Running flake8 checks
flake8 checks were clean
==> spack style checks were clean
I've updated the branch with isort fixes. |
|
@scheibelp at least some of these failures seem to have come from other parts of spack not change by the PR that couldn't have been caused by this PR (i.e. undefined variables in code that I didn't change). What should I do next? |
|
@robertu94 unfortunately after some investigation this error looks legitimate:
That process will take at least a few days |
|
@scheibelp following up to see what the status is on this. |
|
@robertu94 I spoke with @becker33 today and he proposed something I thought might address your needs in a simpler manner: could you let me know if #32547 would be an acceptable approach for you? |
2 participants
tl;dr Cuda installs CUB's (a cuda library) cmake files into a place where spack doesn't know about which means it can't tell cmake about it. We provide a mechanism to tell spack where to find this additional path, and then use it in the cuda package.
fixes #31568
@scheibelp you were the last person to work on the CMAKE_PREFIX_PATH logic. Could you please review this patch?
@ax3l @Rombur you two are the Cuda maintainers. Could you please review the change the the Cuda package?