Enable use as a spack repo #55

aaroncblack · 2023-02-22T00:46:46Z

In order to enable this code repo as a spack repo I added a repo.yaml.

I also had to remove the cross-package imports of functions from the camp package and duplicate those helper functions across the packages for cuda, hip, etc, as the import assumed a particular repo namespace for the packages.

The hip_for_radiuss_project and cuda_for_radiuss_project code looks like functionality that Spack should fold into their RocmPackage and CudaPackage.

Relevant issue #54

aaroncblack · 2023-02-22T01:12:26Z

@kab163 I wasn't sure who to tag on this.

kab163 · 2023-02-22T15:15:56Z

@kab163 I wasn't sure who to tag on this.

That's ok! I'll take a look myself and I know Adrien can help too. Lucky for @davidbeckingsale he is on vacation right now, or I'd tag him too lol. Thanks Aaron!

(Update, tagged David anyways just to give him more emails to sort through when he gets back!)

adrienbernede · 2023-02-22T21:44:36Z

@kab163 That’s funny because in the end you did @ mention David and not me.
@aaroncblack I want to setup this as a legit Spack repo too, but I really don’t want that much duplication... We are grouping things here to avoid duplication and the associated maintenance. Isn’t there a way to get a the implementation in a single location ? (I did not dig into the issue you mention yet).

@white238 @chapman39, please join the party: this is the first example of issue using a legitimate spack repo instead of hot-copying the packages, and I fear that Cyrus might be right to worry about that...

aaroncblack · 2023-02-22T22:03:46Z

I'm currently building a code called 'fedtra' and I'm pulling from:

a spack repo of packages at llnl
axom
radius
spack builtin repo
in that order of priority in the repos list.

Alternatively, I could create a script for our developers that git clones each of these repos and copies the packages into their spack installation.

I'm pretty interested in the pros/cons of each, but so far after @white238 and I have chatted, using multiple repos to manage this seemed more flexible.

Regarding me removing the dependence of packages on functions defined in the camp package, I think you'll have to remove that regardless if you want to merge these back into spack proper at some point. I don't think they'll accept updated to packages that introduce dependencies between packages like this.

I think the duplication of the functions will be short term. Some of the logic looks very appropriate to me for getting pushed up into the spack ROCMpackage and CUDApackage parent classes.

aaroncblack · 2023-02-22T22:06:14Z

packages/raja/package.py

+    if not spec.satisfies("cuda_arch=none"):
+        cuda_arch = spec.variants["cuda_arch"].value
+        cuda_flags.append("-arch sm_{0}".format(cuda_arch[0]))
+        options.append(


Hmm... spack has a CudaPackage and a CMakePackage.

They need a function in one of those that will provide you the correct CMake lines to enable cuda.

Or a new package that descends from both, as long we don't get into a hairy mess of multiple inheritance combinations. I'm going to poke Todd and a couple others about this.

There are a lot of packages having to do this by hand right now.

newer CMakes do not need this. You just do this:

set(CMAKE_CUDA_ARCHITECTURES "70" CACHE STRING "")

True, if radiuss does not need to maintain compatibility with older cmake some of this can be cleaned up.

@aaroncblack I believe CMake 3.14.5, which is the default on most LC systems, requires some of this stuff to work. That's why it's still in the RAJA Spack package.

When all systems are updated to TOSS 4 and new defaults (compilers, CMake, etc.) are in place, then some support for older CMake, for example, can be removed from the Spack packages.

aaroncblack · 2023-02-22T22:22:28Z

packages/umpire/package.py

+    cuda_flags = []
+    if not spec.satisfies("cuda_arch=none"):
+        cuda_arch = spec.variants["cuda_arch"].value
+        cuda_flags.append("-arch sm_{0}".format(cuda_arch[0]))


@adrienbernede On this line, did the cuda_flags() function from the CudaPackage this class descends from not work correctly? That gives the flag '--generate-code arch=compute_' flag. Just curious.

I can’t tell. I guess we’ll have to try.
Getting all our packages in the same place was an improvement. Everything we can do to simplify will be welcome.
However, I will postpone the attempt until we complete the toolchain update that is under way.

adrienbernede · 2023-02-23T09:18:13Z

I'm currently building a code called 'fedtra' and I'm pulling from:
* a spack repo of packages at llnl

* axom

* radius

* spack builtin repo
  in that order of priority in the repos list.
Alternatively, I could create a script for our developers that git clones each of these repos and copies the packages into their spack installation.

I'm pretty interested in the pros/cons of each, but so far after @white238 and I have chatted, using multiple repos to manage this seemed more flexible.

Regarding me removing the dependence of packages on functions defined in the camp package, I think you'll have to remove that regardless if you want to merge these back into spack proper at some point. I don't think they'll accept updated to packages that introduce dependencies between packages like this.

I think the duplication of the functions will be short term. Some of the logic looks very appropriate to me for getting pushed up into the spack ROCMpackage and CUDApackage parent classes.

@aaroncblack you are making several valid points here. And I’m happy to welcome a new user of the radiuss-spack-configs. So I’m going to merge this if you found it works for you. (I will need radiuss-spack-packages as a spack repo too :) ).

aaroncblack · 2023-02-23T17:17:22Z

@adrienbernede I'm going to work a bit on making this PR more palatable. Duplicating these functions works but makes your job harder in the long term.

I propose creating a radiuss_utilities package (or whatever name radiuss team thinks is appropriate), then import the functions from this packages. Essentially the same as you had for camp, but I think you want them in an independent package class not strictly attached to camp. Both the builtin or repo namespaces can be supported with a couple different import attempts ( thanks, @tgamblin, for the suggestion )

try:
    from spack.pkg.builtin.radiuss_utilities import hip_for_radiuss_projects
except ImportError:
    from llnl.radiuss.radiuss_utilities import hip_for_radiuss_projects

I'll move ahead on that if you think it sounds good.

adrienbernede · 2023-02-23T19:28:53Z

Please reverse the order so the priority is put on the radiuss namespace. Unless I missed something?

tgamblin · 2023-02-23T19:33:11Z

Yep @adrienbernede is right -- if both are present (usually the case) you are going to want the RADIUSS one. Sorry about my example.

tgamblin · 2023-02-23T19:36:16Z

I think you could also simplify this a bit an omit the namespace entirely. Like so:

import spack.repo
hip_for_radiuss_projects = spack.repo.path.get_pkg_class("HipForRadiussProjects").module

This is a bit roundabout and maybe we could introduce some syntactic sugar for it, but I think it works.

adrienbernede · 2023-02-27T15:36:14Z

@aaroncblack, because it wasn’t clear: yes, I am supportive of this initiative, so please move ahead 👍🏻 .

adrienbernede · 2023-02-27T16:02:41Z

Note:
I tested this and the "hip-repair-cache" lacks in RAJA, Umpire and CHAI packages.
This in turns brings the need for import glob in Umpire and CHAI.
;)

jonesholger · 2023-04-04T02:49:18Z

I gave some feedback to Adrien tangentially, since I wanted to be able to load from an arbitrary number of user defined repos with sample logic below (you still need to add code to iterate over repos, and include try/except blocks for the builtin case), but seems similar to what I see in this PR

aaroncblack · 2023-04-13T16:56:35Z

I still plan on working on this, just been tackling other higher priority work at present.

aaroncblack · 2023-06-02T18:59:34Z

I have a new version locally, but am hitting an issue.

I went ahead and created a RadiussPackage class, bundling these utilities together inside it. I put it in a packages/radiusspackage/package.py.

However, I am unable to import anything from llnl.radiuss.

If I try adding a

from llnl.radiuss import RadiussPackage

or even just

import llnl.radiuss

I get a

==> Error: Package 'chai' not found.

in that example I'm doing it in chai. If I try in umpire, I get that message about umpire, etc.

My repo.yaml file has:

repo:
  namespace: 'llnl.radiuss'

aaroncblack · 2023-06-02T19:07:30Z

@adrienbernede I'd say, if you can go ahead and merge this, then let's do it. I'm interested in consolidating the misc functions instead of duplicating, but I that can be a new MR.

adrienbernede · 2023-06-02T21:32:44Z

@aaroncblack Thanks for the heads up. In need to experiment with this, definitely, but I am dealing with an issue in spack v0.20.0 that will need to be solved if we want to merge this spack anyways.

So, not sure how I’m gonna proceed.

Now, if it can help, ==> Error: Package 'chai' not found. can just mean that you have a syntax error in your changes, I’d run spack in debug mode to find it (spack -d). I feel like you may have already done that, but the error message does not suggest so.

aaroncblack · 2023-06-02T21:42:05Z

@adrienbernede thanks, I had been using -v, but it slipped my mind to use -d. I'll do that!

adrienbernede · 2023-06-05T09:47:24Z

@aaroncblack Let me know how it goes, I’d really like to avoid the duplication implied by this PR, and I wanna help.

aaroncblack added 2 commits February 21, 2023 16:39

Enable use as a spack repo

2246bd9

remove cross package imports.

f3d0a7d

remove more cross-imported functions.

59f1e1b

kab163 requested a review from adrienbernede February 22, 2023 15:14

kab163 requested review from davidbeckingsale and white238 February 22, 2023 15:20

aaroncblack commented Feb 22, 2023

View reviewed changes

rhornung67 approved these changes Feb 27, 2023

View reviewed changes

adrienbernede mentioned this pull request Mar 2, 2023

Woptim/update uberenv #56

Closed

Merge branch 'LLNL:main' into main

9338265

update

42dfdd3

aaroncblack closed this by deleting the head repository Jun 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable use as a spack repo #55

Enable use as a spack repo #55

aaroncblack commented Feb 22, 2023 •

edited

Loading

aaroncblack commented Feb 22, 2023

kab163 commented Feb 22, 2023 •

edited

Loading

adrienbernede commented Feb 22, 2023 •

edited

Loading

aaroncblack commented Feb 22, 2023

aaroncblack Feb 22, 2023 •

edited

Loading

white238 Feb 22, 2023

aaroncblack Feb 22, 2023

rhornung67 Feb 22, 2023 •

edited

Loading

aaroncblack Feb 22, 2023 •

edited

Loading

adrienbernede Feb 23, 2023

adrienbernede commented Feb 23, 2023 •

edited

Loading

aaroncblack commented Feb 23, 2023 •

edited

Loading

adrienbernede commented Feb 23, 2023

tgamblin commented Feb 23, 2023

tgamblin commented Feb 23, 2023

adrienbernede commented Feb 27, 2023 •

edited

Loading

adrienbernede commented Feb 27, 2023 •

edited

Loading

jonesholger commented Apr 4, 2023 •

edited

Loading

aaroncblack commented Apr 13, 2023

aaroncblack commented Jun 2, 2023

aaroncblack commented Jun 2, 2023

adrienbernede commented Jun 2, 2023

aaroncblack commented Jun 2, 2023

adrienbernede commented Jun 5, 2023

Enable use as a spack repo #55

Enable use as a spack repo #55

Conversation

aaroncblack commented Feb 22, 2023 • edited Loading

aaroncblack commented Feb 22, 2023

kab163 commented Feb 22, 2023 • edited Loading

adrienbernede commented Feb 22, 2023 • edited Loading

aaroncblack commented Feb 22, 2023

aaroncblack Feb 22, 2023 • edited Loading

Choose a reason for hiding this comment

white238 Feb 22, 2023

Choose a reason for hiding this comment

aaroncblack Feb 22, 2023

Choose a reason for hiding this comment

rhornung67 Feb 22, 2023 • edited Loading

Choose a reason for hiding this comment

aaroncblack Feb 22, 2023 • edited Loading

Choose a reason for hiding this comment

adrienbernede Feb 23, 2023

Choose a reason for hiding this comment

adrienbernede commented Feb 23, 2023 • edited Loading

aaroncblack commented Feb 23, 2023 • edited Loading

adrienbernede commented Feb 23, 2023

tgamblin commented Feb 23, 2023

tgamblin commented Feb 23, 2023

adrienbernede commented Feb 27, 2023 • edited Loading

adrienbernede commented Feb 27, 2023 • edited Loading

jonesholger commented Apr 4, 2023 • edited Loading

aaroncblack commented Apr 13, 2023

aaroncblack commented Jun 2, 2023

aaroncblack commented Jun 2, 2023

adrienbernede commented Jun 2, 2023

aaroncblack commented Jun 2, 2023

adrienbernede commented Jun 5, 2023

aaroncblack commented Feb 22, 2023 •

edited

Loading

kab163 commented Feb 22, 2023 •

edited

Loading

adrienbernede commented Feb 22, 2023 •

edited

Loading

aaroncblack Feb 22, 2023 •

edited

Loading

rhornung67 Feb 22, 2023 •

edited

Loading

aaroncblack Feb 22, 2023 •

edited

Loading

adrienbernede commented Feb 23, 2023 •

edited

Loading

aaroncblack commented Feb 23, 2023 •

edited

Loading

adrienbernede commented Feb 27, 2023 •

edited

Loading

adrienbernede commented Feb 27, 2023 •

edited

Loading

jonesholger commented Apr 4, 2023 •

edited

Loading