-
Notifications
You must be signed in to change notification settings - Fork 17
Upgrade to Spack v1.1.1 and related fixes. #196
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
base: develop
Are you sure you want to change the base?
Conversation
|
@cameronrutherford Do you know a fix for the error I'm seeing? Everything is building fine, but the push to the container registry is not working. |
f52f32a to
ea3b5fd
Compare
ea3b5fd to
021f9dd
Compare
Finally figured out a way to make it work! |
cameronrutherford
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some NITs, some other more interesting comments.
|
|
||
| #- name: Test Build | ||
| # run: cd $(spack -e . location --build-dir exago@develop) && ctest -VV | ||
| #- name: Test Build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't remember why this is commented out.. perhaps there's an issue for it? Would be good to test things work before pushing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason was that no tests were ever running in GH-Actions. We've been relying on manually testing on Frontier, and just checking that things build in GH-Actions.
The Spack package relied on Spack's --run-tests to set EXAGO_RUN_TESTS. If I recall correctly, we can't used the built-in testing because of the need to install first. I've just added an independent testing option in the Spack package, independent of --run-tests, which revealed a number of failing tests. I'll have to look into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could fix easily the issue where we need to install before testing. As far as I remember, there was an issue about finding test input files and it was "fixed" by installing them first, and then accessing them in the installation directory. Also see #148.
buildsystem/spack/spack_repo/exago/packages/exago/__pycache__/package.cpython-311.pyc
Outdated
Show resolved
Hide resolved
buildsystem/spack/spack_repo/exago/packages/hiop/__pycache__/package.cpython-311.pyc
Outdated
Show resolved
Hide resolved
| # -fPIC required for spdlog for use with python wrapper | ||
| set_property(TARGET spdlog PROPERTY POSITION_INDEPENDENT_CODE ON) | ||
| find_package(spdlog REQUIRED) | ||
| find_package(fmt REQUIRED) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the Python wrapper needing position independent code not relevant anymore?
Also, should we give a hint to the path here for the submodule?
Perhaps we only enforce that property here / in spack when Python is enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm requiring spdlog as an external dependency here, so I wouldn't use the submodule and PIC does not apply to the external target.
| def submodules(package): | ||
| submodules = [] | ||
| submodules.append("tpl/pybind11") | ||
| submodules.append("tpl/spdlog") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're making this an external dependency, then we should probably consider doing the following in pursuit of removing submodules (I think this is an existing issue / desirable goal):
- Figure out a way to only clone some submodules for different versions of ExaGO. I imagine it's doable as
packageis passed to the function. - Add this as a dependency at the spack level (build time, link time?) for only certain versions.
- Figure out how to express the dependency for PIC if it's necessary only for the Python wrapper(?)
This can also be another issue for later if you just want this merged and it works IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only made spdlog an external dependency, since that is truly optional. We can remove the submodule in a subsequent PR once the edits are propagated upstream. We will also need to add patches to older versions when we make that change. The Spack-level dependency only applies to develop at the moment.
I have not addressed pybind11 and toml11. One thing to consider it that making these external puts the burden on the user to have them installed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest we discuss it a little more before we decide to treat toml and pybind as external packages. ExaGO already has too many dependencies and I agree with @nkoukpaizan that we need to minimize burden on the user.
64de6e7 to
33081cf
Compare
|
LGTM if CI passes. I'm sure you'll get a better review from a maintainer about the |
| variant("sparse", default=False, description="Enable/Disable Sparse linear algebra") | ||
| variant( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think HiOp should be built with sparse support by default. We are going to deprecate mixed dense-sparse module in ExaGO and PriDec and sparse HiOp modules will be pretty much all we'll need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is the HiOp package, so I'll argue that the default value falls under the purview of HiOp. Not ExaGO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, HiOp default is that sparse linear algebra is on. Perhaps the HiOp spack package has not been updated accordingly.
Merge request type
Relates to
This MR updates
Summary
This upgrades the Spack submodule to v1.1.1 and deprecates incompatible build configurations (deception, newell and incline) that are not being tested with the new configuration.
romccis no longer a supported compiler, and mixes compiler modules are deprecated on Frontier. The only way to get a performant Fortran compiler forcoinhsl, etc., and support HIP is to loadPrgEnv-gnuandrocm. So, most packages are built built withgccunless there is HIP code.deprecatedfolders with correspondingREADMEfiles.Linked Issue(s)