-
Notifications
You must be signed in to change notification settings - Fork 32
Adds a components variant to Axom's spack package
#1740
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
Conversation
| when="@:0.12.0", | ||
| description="Build with hooks for Adiak/Caliper performance analysis. " \ | ||
| "Deprecated -- use the adiak and/or caliper variants directly.", | ||
| ) |
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 profiling variant added a dependency on adiak and caliper. Since sina has a soft dependency on adiak, I replaced it with direct variants for adiak and caliper.
Should we remove the profiling variant? In this PR I added a description that this variant in deprecated and changed the when line.
Is there a better way to do this? I noticed this open issue on spack spack/spack#38829
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 deprecating before removing is the recommended way. It sends the signal that users should prepare for removal, while reducing the risk that they still use the variant at next release when you effectively remove the variant... at the cost of maintaining both for a while.
Drawback: without deprecation support like suggested in the mentioned PR, the deprecation will likely go unnoticed.
| variant("tutorials", default=True, description="Build tutorials") | ||
|
|
||
| # Hard requirement after Axom 0.6.1 | ||
| variant("cpp14", default=True, description="Build with C++14 support") |
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.
Given that we have a hard requirement on c++17 and soon c++20, we should remove/update the cpp14 variant, but I wasn't sure about the best way to do that. E.g. do we need to keep old variants around and change the when logic?
Presumably, we should use cxxstd instead of hard-coding the language version.
I'll create an issue to track this.
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.
Update: #1741
BradWhitlock
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.
These changes look reasonable to me. Thanks!
| depends_on("conduit+python", when="+devtools") | ||
| depends_on("conduit~python", when="~devtools") |
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.
@bmhan12 -- I think this conduit+python dependency on devtools is meant to be temporary.
If so, could you please add an issue to describe and track 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.
Tracked in issue #1742!
| # we're planning to remove support for the profiling variant, but still need to support it for now | ||
| depends_on("adiak", when="+adiak") | ||
| depends_on("caliper", when="+caliper") | ||
| with when("+profiling"): | ||
| depends_on("adiak") | ||
| depends_on("caliper+adiak~papi") | ||
|
|
||
| depends_on("caliper+cuda", when="+cuda") | ||
| depends_on("caliper~cuda", when="~cuda") | ||
| depends_on("caliper") | ||
|
|
||
| depends_on("caliper+rocm", when="+rocm") | ||
| depends_on("caliper~rocm", when="~rocm") | ||
| with when("^adiak"): | ||
| for fwd in ("mpi", "shared"): | ||
| depends_on(f"adiak+{fwd}", when=f"+{fwd}") | ||
|
|
||
| for dep in ["adiak", "caliper"]: | ||
| depends_on(f"{dep}+mpi", when="+mpi") | ||
| depends_on(f"{dep}~mpi", when="~mpi") | ||
| depends_on(f"{dep}+shared", when="+shared") | ||
| depends_on(f"{dep}~shared", when="~shared") | ||
| with when("^caliper"): | ||
| for fwd in ("cuda", "rocm", "mpi", "shared"): | ||
| depends_on(f"caliper+{fwd}", when=f"+{fwd}") | ||
|
|
||
| for val in CudaPackage.cuda_arch_values: | ||
| ext_cuda_dep = f"+cuda cuda_arch={val}" | ||
| depends_on(f"raja {ext_cuda_dep}", when=f"+raja {ext_cuda_dep}") | ||
| depends_on(f"umpire {ext_cuda_dep}", when=f"+umpire {ext_cuda_dep}") | ||
| depends_on(f"caliper {ext_cuda_dep}", when=f"+profiling {ext_cuda_dep}") | ||
| depends_on(f"caliper {ext_cuda_dep}", when=f"^caliper {ext_cuda_dep}") | ||
| depends_on(f"mfem {ext_cuda_dep}", when=f"+mfem {ext_cuda_dep}") | ||
|
|
||
| for val in ROCmPackage.amdgpu_targets: | ||
| ext_rocm_dep = f"+rocm amdgpu_target={val}" | ||
| depends_on(f"raja {ext_rocm_dep}", when=f"+raja {ext_rocm_dep}") | ||
| depends_on(f"umpire {ext_rocm_dep}", when=f"+umpire {ext_rocm_dep}") | ||
| depends_on(f"caliper {ext_rocm_dep}", when=f"+profiling {ext_rocm_dep}") | ||
| depends_on(f"caliper {ext_rocm_dep}", when=f"^caliper {ext_rocm_dep}") | ||
| depends_on(f"mfem {ext_rocm_dep}", when=f"+mfem {ext_rocm_dep}") |
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.
Could someone please double-check these changes? I was trying to support the new adiak and caliper variants as well as the old profiling one, so I changed the +foo (which checks a variant) to ^foo (which checks a dependency). It seems to work and makes sense to me, but please let me know if there's a better way to do it.
| path_replacements = {} | ||
|
|
||
| # Validate the 'components' spec, which can selectively enable components | ||
| self._validate_component_deps() |
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.
Should the validation happen in initconfig? i.e. after concretization and after all of axom's dependencies have been built.
Please let me know if there's an earlier place that this can be done.
0029e61 to
e9d8309
Compare
| caliper: | ||
| require: | ||
| - spec: "~kokkos" | ||
| - spec: "~kokkos~papi" |
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 moved caliper~papi to our defaults.yaml since the previous way would impose constraints on other axom users.
Is there a reason to keep it in axom/package.py?
|
The axom package looks way better (160 added lines, nearly all declarative vs. 242 in the first revision with custom consistency checking). We can prioritize improving the error messages for package requirements in Spack. |
| requires("components=slic,spin,primal", when="components=bump") | ||
| requires("components=sidre,slic,primal", when="components=inlet") |
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.
Because I never used it, I guess it is trivial that spack will merge the required components lists ?
This can be tested by concretizing axom components=bump,inlet, which should activate bump,inlet,sidre,slic,spin,primal.
Based on documentation, I have no doubt multi-valued variants are mergeable, but documentation on requires is not explicit on this possibility in that particular case.
| if spec.satisfies("+profiling"): | ||
| dep_dir = get_spec_path(spec, "adiak", path_replacements) | ||
| entries.append(cmake_cache_path("ADIAK_DIR", dep_dir)) | ||
|
|
||
| dep_dir = get_spec_path(spec, "caliper", path_replacements) | ||
| entries.append(cmake_cache_path("CALIPER_DIR", dep_dir)) | ||
|
|
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 I read correctly, +profiling will still add ^adiak and caliper, but not +adiak and +caliper.
Therefore, if you remove the above section, I wonder how ADIAK_DIR and CALIPER_DIR will be set.
Unless they are replaced by TPL_ROOT ?
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 agree that if you use a variant to force a conditional dep, you should force the variant here and not the dep (doing it this way introduces two ways to get the dependency, when you really want ^name to be satisfied if and only if +name is also satisfied)
rhornung67
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.
Thank you @kennyweiss
|
I'm getting some vague error messages from spack with It feels like spack thinks it's telling me 8 different things, but all I'm getting from it Update -- I found the problem! I was trying to test the variant(
"profiling",
default=False,
when="@:0.12.0", ## <---- Version restriction here
...
)So, spack was doing the right thing, but it didn't tell me why. @scheibelp @tgamblin @becker33 -- this seems like a good candidate for improved Spack error messages. |
The components can either be "all" (default), which enables all Axom components, or it can be a comma separated list of components, e.g. 'components=slic,sidre'. If components is not specified, all components will be enabled. Use 'components=none' to disable all components (other than core, which is always on). We validate the list to ensure that all hard dependencies of the requested components are enabled, e.g. 'components=inlet,sidre,slic' would pass, but 'components=inlet' would fail since inlet depends on sidre.
And adds a fix to allow Axom to configure with '~conduit'
…g "profiling" variant Users have requested the option to enable/disable adiak and caliper separately. I left in support for the "profiling" variant for now.
sina's tests use slic
The mir component no longer has a hard dependency on raja or umpire.
We now add any missing components and dependencies to satisfy required dependencies. E.g. if `components=sina`, we will add `slic` and `conduit` to ensure a valid configuration. Credits: Thanks @tgamblin for the suggestions!
…conduit Specifically, we skip some of the entries, such as "lua", which can be in a different installation path. We also take the directory of the main package (axom-*) since the library we're checking might start with the same letter (e.g. adiak).
8f4c24b to
6527843
Compare
Summary
componentsvariant to Axom's spack package to allow users to provide an explicit list of components to enable.componentsis not provided, or whencomponents=all) all components are enabledcomponents=none- The components are validated during the initconfig stage -- if the component's required dependencies are not enabled, we get aSpackErrorindicating the missing dependencies. We also validate against the component's required package dependenciesWe use Spack's requires/conflicts mechanisms to any missing inter-component or package dependencies. E.g. since
inletdepends onsidreandconduit(among others), addingcomponents=inletto the spec will enablesidreandconduit. Thanks @tgamblin for the suggestions!This PR also adds a
conduitvariant (enabled by default). Previously,conduitwas always requiredThis PR also deprecates the
profilingvariant in favor of newadiakandcalipervariants.Resolves Sina Spack Variant #1698 (tag @ldowen)
Related to What extra configuration tests are needed? #1722
Some test runs (updated after improving validation)
Run w/
components=inlet:Concretized after adding dependencies on
primal,sidreandslic(also adds+conduit, but that was on by default):Invalid run:
components=inlets ~conduit:==> Error: failed to concretize
Note: the error message doesn't explicitly note the failure as
+conduit/~conduit. I'll see if spack can improve this.Update -- we get a much better error message in spack@develop (due to this PR: spack/spack#45800)
Thanks for anticipating this @scheibelp!
Run w/
components=all:Concretized to:
Run w/
components=none:Concretized to: