Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Woptim/update local packages #1338

Merged
merged 41 commits into from
Nov 21, 2022
Merged

Woptim/update local packages #1338

merged 41 commits into from
Nov 21, 2022

Conversation

adrienbernede
Copy link
Member

@adrienbernede adrienbernede commented Oct 3, 2022

Summary

  • This PR is an improvement.
  • It does the following:
    • Update local Spack packages based on current spack@develop version. Keep the logic that was only present locally when necessary to pass tests (see commit messages)
    • Modifies the CI to add necessary variants.

Important notes

The raja package diff is hard to read because of the switch to CachedCMakePackages. Reviewers should compare the new version with https://github.com/spack/spack/blob/develop/var/spack/repos/builtin/packages/raja/package.py.

By default, "shared" is "True" in the upstream spack package. I set "shared" to "false" in the global variants settings in CI here:

By default, "tests" is "False" in the upstream spack package. I set "tests" to "true" in the global variants settings in CI here:

PROJECT_RUBY_VARIANTS: "~shared +openmp +tests"

PROJECT_CORONA_VARIANTS: "~shared ~openmp +tests"

PROJECT_LASSEN_VARIANTS: "~shared +openmp +tests"

Relates to:
LLNL/Umpire#793.
LLNL/CHAI#218

@adrienbernede
Copy link
Member Author

adrienbernede commented Oct 3, 2022

@ reviewers:

I took the packages from spack@develop, ran the CI, and added the missing parts from the initial local packages to pass the tests.

It is possible that not all the diffs were ported to the new local packages. But since we are now passing the tests with packages much closer to the upstream packages, it seems to me that we should take this as a new base.

Also remember that the goal is to minimize the diffs with the upstream package. We should consider pushing the new local packages to Spack upstream.

This is part of the process of choosing a Spack reference for CI testing described here:
https://radiuss-shared-ci.readthedocs.io/en/latest/sphinx/user_guide/how_to.html#choose-a-spack-reference-commit-or-branch

@rhornung67
Copy link
Member

Adding more reviewers since we will discuss Gitlab CI, including this at the project meeting tomorrow morning.

@rhornung67
Copy link
Member

@adrienbernede there is an issue with the spack spec in one of the lassen xl+cuda builds that is causing the failure.

Copy link
Member

@rhornung67 rhornung67 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of spack package version updates.

@adrienbernede
Copy link
Member Author

@rhornung67 will do.

@rhornung67
Copy link
Member

@adrienbernede this looks good to me. I would like @davidbeckingsale to review.

@adrienbernede
Copy link
Member Author

@rhornung67 OK.

For the records, I am trying to sync things with Umpire and CHAI.

Illustration of the challenges:

  • Since CHAI@develop needs 2022.10 releases, we need the updated packages locally in CHAI.
  • the 2022.10 releases require BLT 5.2.0 which is not present in [email protected]. This means we need a local BLT package for BLT... again. Except if we push all our new packages to a Spack PR and get it merged.

Suggested path forward:

  1. Make things work without updating Spack yet.
  2. Push the new packages to Spack in a joint PR (RAJA, Umpire, CHAI, camp?).
  3. Update Spack in our project and remove identical local packages.

@adrienbernede
Copy link
Member Author

Note: This is not ready, I am finding things I need to improve first. Will report back.

Copy link
Member

@rhornung67 rhornung67 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thank you @adrienbernede

@rhornung67
Copy link
Member

@davidbeckingsale should I merge this? I don't foresee any issues with it.

@davidbeckingsale
Copy link
Member

@davidbeckingsale should I merge this? I don't foresee any issues with it.

Yup, sounds good!

@rhornung67 rhornung67 merged commit 64c3baa into develop Nov 21, 2022
@adrienbernede adrienbernede deleted the woptim/update-local-packages branch November 22, 2022 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants