-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Add --os option to containerize #30385
base: develop
Are you sure you want to change the base?
Conversation
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 have a comment on error-handling. Note that with respect to being able to automatically generate a spack.yaml
file, this PR covers only a small amount of ground. Any customization needed e.g. to install a run-time library like OpenMP from the corresponding linux distribution still needs to be handled by manually editing the spack.yaml
file.
if args.os: | ||
config['spack']['container']['images']['os'] = args.os |
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 need to:
- Validate the
args.os
argument to ensure it's among the allowed values - Handle the case where a conflicting option is already present in the
spack.yaml
and e.g. inform the user about it
For 2. in particular, there are currently different ways to select the build image. If a spack.yaml
already defines custom images we should probably error and report a comprehensible message pointing to the docs.
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.
@philregier Apologies, since I remember we briefly discussed that a few weeks ago during the meeting, but can you remind me which use case is covered by this PR?
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.
Sorry, @alalazo, I was getting ready for travel when you made these comments and I didn't see them when I got back; that was my fault. To your comments:
- Due to the placement,
args.os
gets treated exactly like text inspack.yaml
does; if I specifywhatever
in either location, I get the same error:
==> Error: invalid operating system name "whatever". [Allowed values are alpine:3, amazonlinux:2, centos:stream, centos:7, opensuse/leap:15, nvidia/cuda:11.2.1, ubuntu:22.04, ubuntu:20.04, ubuntu:18.04]
- I based my suggestion on the documentation, and am proposing this as a level-7 override to the level-6
spack.yaml
(see below for use cases). I don't really get what this has to do with custom images, but if having an entry in spack:container:images:os would presently error and report a comprehensible message pointing to the docs, then specifying--os
probably will as well, unless aforementioned error takes place inspack.container.validate
, in which case I would definitely need to revisit my proposal.
As for use cases, the basic ones include (1) troubleshooting: if a spack.yaml with a defined os
runs into problems, one could override it on the command line to see if the recipe produced with an alternate --os
override had the same issue, (2) automation, so a distribution vendor can do CI/CD in a parameterized fashion, testing the same spack.yaml
against multiple targets (without adding another dimension to the required matrix of spack.yaml files) to make sure all OS releases are suitable for use as spack targets, and (3) for documentation purposes, to simplify demonstration of exporting the same functioning spack environment to different target containers (and EDIT: to this last point, if there's something dangerous about --os
, then it's intrinsically dangerous for spack:container:images:os
as well, in which case documenting hand edits is just as treacherous as specifying --os
on the command line!).
This last gets to my other concern, which isn't quite covered by your questions so far, which is that containerize
strikes me very much as an operation on an environment, not a fixed set of properties. I don't see any reason why if Alice, Bob, and Charlie are using the same environment on their institutional cluster, why Alice might not be perfectly content to run locally, Bob might want to export container images from their environment to Amazon, and Charlie might want to build CentOS images for use on GCP. I don't get why the final export step should mean they have to all have forks of their shared environment, since (again) the containerize
step seems to be an export operation as opposed to a property of the instantiated environment.
Does any of this make sense? I don't presently want to change what spack does in any of these cases, just add a switch to access its current unmodified behavior from an automated and/or parameterized context.
spack containerize
already takes a--list-os
but currently requires that eachspack.yaml
be modified individually to build against anything but Spack's hardcoded distribution preference. This patch introduces a corresponding--os
option to select any valid listing fromimages.json
which contains the profiles listed by--list-os
.I maintain that the lack of this feature introduces unnecessary complexity into any affected documentation and/or automation; the former is reasonably low-impact, but not being able to automate generation of recipes in a repeatable and unattended fashion is somewhat more troublesome. This being my first PR, I am perfectly content to move this to an issue for further discussion and/or to rework or extend it as needed, but since the patch proved much more trivial than I expected I thought I'd begin by simply presenting the code.