-
Notifications
You must be signed in to change notification settings - Fork 9
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
Spack Environments #96
Conversation
… into feature/chapman39/spack_env
uberenv.py
Outdated
@@ -828,38 +852,6 @@ def patch(self): | |||
self.disable_spack_config_scopes(spack_dir) | |||
spack_etc_defaults_dir = pjoin(spack_dir,"etc","spack","defaults") | |||
|
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 just want you to be aware that this will no longer hot-copy any files into spack's repo. All of these things can be handled inside of the environment file.
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.
In that case, do we need to add safeguards/options our environment files to not use the user's home directory (e.g. ~/.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'm confused by this. Can this section be removed, then (in another 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.
The disabling of the user scope config (inside your home directory) is handled by the function disable_spack_config_scopes
, which is still there.
Lines 815 to 840 in 096edec
def disable_spack_config_scopes(self,spack_dir): | |
# disables all config scopes except "defaults", which we will | |
# force our settings into | |
spack_lib_config = pjoin(spack_dir,"lib","spack","spack","config.py") | |
print("[disabling config scope (except defaults) in: {0}]".format(spack_lib_config)) | |
cfg_script = open(spack_lib_config).read() | |
# | |
# For newer versions of spack, we can use the SPACK_DISABLE_LOCAL_CONFIG | |
# env var plumbing. We patch it to True to make a permanent change. | |
# | |
# Note: This path does not disable the 'site' config, but disabling 'user' config | |
# is our primary goal. | |
# | |
spack_disable_env_stmt = 'disable_local_config = "SPACK_DISABLE_LOCAL_CONFIG" in os.environ' | |
spack_disable_env_stmt_perm = "disable_local_config = True" | |
if cfg_script.count(spack_disable_env_stmt) > 0: | |
cfg_script = cfg_script.replace(spack_disable_env_stmt, | |
spack_disable_env_stmt_perm) | |
# path for older versions of spack | |
elif cfg_script.count(spack_disable_env_stmt_perm) == 0: | |
for cfg_scope_stmt in ["('system', os.path.join(spack.paths.system_etc_path, 'spack')),", | |
"('site', os.path.join(spack.paths.etc_path, 'spack')),", | |
"('user', spack.paths.user_config_path)"]: | |
cfg_script = cfg_script.replace(cfg_scope_stmt, | |
"#DISABLED BY UBERENV: " + cfg_scope_stmt) | |
open(spack_lib_config,"w").write(cfg_script) |
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.
this made me realize that spack_etc_defaults_dir
was no longer being used, so i removed 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.
Looks great. Sorry for the minor nitpicking at the end.
Thanks, @chapman39 !
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 I’m late for the party...
uberenv.py
Outdated
# Create Spack Environment | ||
if not is_windows(): | ||
env.create_spack_env() |
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.
In "create_spack_env" the environment is also concretized. It means that it’s concretized before the mirrors and upstreams are set, so the concretization cannot use them. Both could contain already installed packages, especially when using --reuse
.
Setting up the mirrors and upstreams must also be done before looking for already installed spec in show info, for the same reason.
In the end, I think both concretization and checking for already installed package should be done right before the install. That does not apply to "creating the environment" nor "showing information" by the way.
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.
So you're saying setting up mirrors and upstreams should be done first, and then concretize?
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, move concretize after, or move mirrors and upstreams before, that’s the same :)
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 be fixed in the following commit
@white238 thoughts on creating a mirror CI check in the future?
uberenv.py
Outdated
sexe("cp {0} {1}/".format(packages_yaml, spack_etc_defaults_dir ), echo=True) | ||
else: | ||
# let spack try to auto find compilers | ||
sexe("{0} compiler find".format(self.spack_exe_path()), echo=True) | ||
|
||
# hot-copy our packages into spack | ||
if len(self.packages_paths) > 0: |
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.
This section should be augmented to only copy if the packages directory is not a spack repo. If it is a spack repo it should call spack repo add /path
.
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 was keeping that one for a later issue... 😔
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.
im having to move this from patch() and into a new function after the spack environment has been created
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.
to determine if there is a spack package repo, i scan each package path in spack_packages_path
and check if there is a repo.yaml
in the previous directory.
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 @white238 I’m really thinking that this should be addressed in a subsequent PR: we don’t have that "feature" yet in Uberenv, and we will only delay the merge of this one. Additionally, my personal take on this is that we could/should require that custom packages be defined in a legit spack repo and definitively stop hot copying packages.
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 this works seamlessly. How confident are we that wont open up a new surface of confusion?
Hot copying is dead simple to understand and one of the reasons that uberenv has been flexible despite a host of changes in spack proper.
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 personally think it is only an improvement. I recommend we remove the hot-copying and add an error check for correctness and tells you how to fix it. For example, if there isnt a repo.yaml
in the correct spot it errors immediately and tells you to create one there.
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 trust you, but that's also not the question I asked :-).
It would be good to know what the new failure modes are due to this change.
Documenting those is important b/c it is a big change.
For example:
We never needed to write a repo.yaml
before. (More yaml files for the yaml overloads :-))
I am fine with moving forward, but we need be prepared to help folks understand how to move and fix things when things go off the rails b/c we know they will.
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.
@chapman39 can you add a section in the documentation on what you need inside of a packages directory to make it a spack package repository?
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.
Documenting those is important b/c it is a big change.
I agree. I will add a section in the documentation for this
I enabled the documentation here: https://uberenv.readthedocs.io/en/feature-chapman39-spack_env/ |
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 got this to work without much trouble in Umpire CI. Thank you @chapman39 !
So am I missing something, or does this change mean that all of the configs in https://github.com/LLNL/radiuss-spack-configs need to be changed to spack environment syntax, i.e. combine the compilers.yaml and packages.yaml files? In sundials we point |
I believe so - if you update to the latest uberenv, yes. If you aren't ready for the change (which will take some time with respect to configs), then you should stay on the older version. |
We tagged the pre-spack environments version here: https://github.com/LLNL/uberenv/tree/v1.0.0 It is also included (but won't be updated) as |
We should add a documentation section with some guidance on what to do to convert from the old split file way to the new environment file. It isn't a lot of work but may be confusing. |
@balos1 I’m always glad to hear someone is using This is for testing purpose: we are still making changes to radiuss-spack-configs non-env version, so I need to think about the migration process first. Maintaining both is not satisfying. |
Changes that will affect users
compilers.yaml
andpackages.yaml
, they're both put into a singlespack.yaml
--spack-config-dir
option is replaced by--spack-env-file
, which is the path to aspack.yaml
orspack.lock
--spack-env-name
is a new option that names the environment (which is always placed in the destination dir)spack-activate
is no longer an option and is replaced by viewsFile Organization Changes
.ci/test-project/spack_configs
directory with differentspack.yaml
's based on system.ci/test-project/uberenv_configs
directory for each different mode to test (install, dev-build, uberenv-pkg)deprecated_uberenv.py
, which is the originaluberenv.py
before spack environmentsUberenv.py Changes
--spack-env-file
is not specified,uberenv.py
will try to find it based on thespack_config_path
and platformSpackEnv
classspack_exe_path()
withspack_exe()
, which has an option to include Spack's environment-e
option (default).SpackEnv
has a new function,create_spack_env()
, that creates a spack environment, adds the package to the environment, and concretizes the environmentpkg_name_with_spec
variable toSpackEnv
class, which is the combination of package name and specSpackEnv
'sinstall()
. Every build mode now usesspack install
. Removesspack activate
sections.Fixes #93
Fixes #98