-
Notifications
You must be signed in to change notification settings - Fork 10
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
Feature/spack env #18
Conversation
# copy in "defaults" config.yaml | ||
config_yaml = os.path.abspath(pjoin(self.uberenv_path,"spack_configs","config.yaml")) | ||
sexe("cp {} {}/".format(config_yaml, spack_etc_defaults_dir ), echo=True) | ||
self.spack_cmd = "{} -e {}".format(self.spack_cmd,self.config_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.
Is there a reason you went with -e instead of loading the environment? My concern is if you need to debug your build you would need to remember to add the -e to your spack commands.
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.
Unfortunately there is a reason:
I tried first with loading the environment, but loading from a script is not reliable/persistent in my experience. It seems to be because it would load the environment in a subprocess and then kill it (the subprocess).
I asked @becker33. The other solution would be to use "spack python" as an interpreter, but it’s not compatible with the fact uberenv clones spack.
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 the "-e" method isn’t stupid though, the way I coded it I mean. Maybe we could make it more explicit what is the environment and the command to use to reproduce the behavior.
Like an output
"Uberenv: Now using spack environment scripts/uberenv/spack-config/toss3...
"
"Ubernev: load environment with : "
"Uberenv: > spack env activate scripts/uberenv/spack-config/toss3...
"
"Uberenv: or"
"Uberenv: > spack -e scripts/uberenv/spack-config/toss3... <action>
"
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.
As long as there is a reason, this is the whole purpose of uberenv is to encapsulate what we feel is the best way to use spack for us. If loading the environment doesn't work right now then we have to go with the -e option. Just make sure that it prints out the extra options so we can copy and paste the command. Maybe when it errors out re-print the command to the screen so it is easy to find.
I would try to get them to fix the env command while moving forward with this solution though.
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.
What do you mean by "fix the env command"?
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.
@cyrush I’m not sure I understand the question.
be persistent beyond the shell instance
Would you like the "activation" or the "configuration" to persist ?
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.
yes, our current setup modifies spack so that the compiler and package settings persist.
Any further spack calls (driven by uberenv or manually) will respect those changes without having to know to invoke extra commands.
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.
Now I understand. And it’s true that it is not the case with the present PR.
What is the exact usage currently, because you still need a way to call Spack:
- always using the binary (uberenv-libs/spack/bin/spack) ?
- loading spack shell setup : . ubernev-libs/spack/share/spack/setup.sh ?
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.
@cyrush -- maybe we could generate a small script run_spack
in the uberenv_libs
directory that runs spack with the desired persistent
parameters.
This is similar to what axom
does in its config_build
script to use the same version of cmake that was used to configure axom:
https://github.com/LLNL/axom/blob/61cd97c5aad8c96fd5d3defe16f5f72667ee1b99/config-build.py#L217-L230
A side benefit would be that you no longer need to run spack/bin/spack
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.
After a bit more thought, I think it is crucial to export a run_spack
script (and document this) to help users know how to setup their spack environment, as per my previous comment.
usually I haven't used spack shell much b/c there was a tricky bug with an alias to a shell command in my environment that prevented me from using for years (until we figured it out :-), but old habits are hard to get rid of) |
Let’s summarize: Need:
Root problem:
First way to solve it:
This method was OK. I think the patch was a bit "violent" but it’s working. The root problem remains. Another way with environments?Now, with environments we can do something somewhat equivalent without patching Spack, but the configuration is not forced anymore, the goal of environments being to easily switch from one to another. We could imagine ways to force the use of a configuration, like a script to call the local Spack binary with the adequate environment, could even be encapsulated in Uberenv, but it gets a bit twisted IMO. However, when using environment, we begin to envision a new possibility: using the same Spack instance to manage multiple projects. In fact, each project provides the desired configuration. Everything gets installed in the same place. And I think that’s what Spack is aiming at. Then, "you wanna work on conduit? Just load conduit environment." (currently we use anonymous environment, but we could name them). If the goal is to get rid of patching of Spack and not to use a single Spack instance, maybe environment is not the perfect answer precisely because we loose the "enforced configuration". @becker33 I’m just mentioning you so that you get an idea of what we need. |
471b6bf
to
657ea71
Compare
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.
Thanks @adrienbernede
I think this branch is almost there, but needs a bit more work before we can merge it.
-
We need to make sure the script won't crash if a user does not provide a spack config dir. E.g. if they are setting this up on their laptop and are happy with the default environment.
-
We should document the change in behavior in the sphinx docs, since this fix, while elegant will take the user from "it just works" (using the spack environment hotcopy), to "it works if I pass in the right parameter (and btw, there is no error message to let you know that this parameter is missing)"
-
We need to export a simple file, named something like
run_spack
which they can use to run uberenv's spack with the given environment. This script should have permissions for the user to execute it (e.g.chmod u+x
).
Some other notes about consistency:
-
For consistency with the rest of uberenv, please add explicit indices to the
String.format
lines
I.e. prefer"{0} find -p {1}".format(...)
to"{} find -p {}".format(...)
-
Please also move the entire command into the interpolated string.
I.e. prefer something likesexe("{0} find -p {1}".format(spack_cmd, pkg_name)
to something likesexe("{0} find -p ".format(spack_cmd) + pkg_name)
uberenv.py
Outdated
@@ -386,82 +389,44 @@ def config_dir(self): | |||
return spack_config_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.
When the spack-config
option is not provided, this will return None
, right?
# copy in "defaults" config.yaml | ||
config_yaml = os.path.abspath(pjoin(self.uberenv_path,"spack_configs","config.yaml")) | ||
sexe("cp {} {}/".format(config_yaml, spack_etc_defaults_dir ), echo=True) | ||
self.spack_cmd = "{} -e {}".format(self.spack_cmd,self.config_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.
What happens if there is no spack config dir? Will this error out?
Perhaps we need to only add the -e <config-dir>
option when it is available?
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.
@kennyweiss an environment is treated the same way config dir were.
It means that Uberenv will try to retrieve the environment corresponding to the current OS/system type.
The behavior when no environment is found is then consistent with what it was before: it throws and error.
This means projects must add a new environment (spack.yaml file) in each config dir.
Adding a basic one is fairly straightforward though, and consists in:
spack:
include:
- ../config.yaml
- compilers.yaml
- packages.yaml
specs: []
view: false
concretization: separately
and in compilers.yaml and packages.yaml, changing respectively:
compilers:
->compilers::
packages:
->packages::
In order to override and scoping (can be done for config too, but I think we will need to discuss the config case separately: overriding scopes in config is not safe).
# copy in "defaults" config.yaml | ||
config_yaml = os.path.abspath(pjoin(self.uberenv_path,"spack_configs","config.yaml")) | ||
sexe("cp {} {}/".format(config_yaml, spack_etc_defaults_dir ), echo=True) | ||
self.spack_cmd = "{} -e {}".format(self.spack_cmd,self.config_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.
After a bit more thought, I think it is crucial to export a run_spack
script (and document this) to help users know how to setup their spack environment, as per my previous comment.
657ea71
to
5b804cd
Compare
@kennyweiss I added a very simple script generation to encapsulate the spack command. This branch is working fine on Umpire CI. |
I should have merged instead of force pushing the rebased branch. Old habits. |
@cyrush @kennyweiss @white238 what do you guys want for this branch? Please express your requirements. |
Since I have been using envs a bit more, I have a few general suggestions:
|
Goal:
Here is an attempt to use Spack environments to enforce the configuration.
Benefits:
No need to patch Spack, no need to override Spack configuration.
Painful points: