Skip to content
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

Deprecate spack:concretization over concretizer:unify #30038

Merged

Conversation

haampie
Copy link
Member

@haampie haampie commented Apr 13, 2022

Now that we have concretizer configuration, we can use it for environments.

  1. Introduce spack:concretizer:unify:true|false in favor of spack:concretization:together|separately.
  2. Add a spack env update path. This update makes environments incompatible with Spack 0.17, but forward compatible with 0.19 once we remove spack:concretization.
  3. Add a deprecation warning for concretization.
  4. Store the default value spack:concretizer:unify in new spack.yaml files to make it easier for us to change the default of unify: false to unify: true in 0.19. (Downside being that new environments in 0.18 are incompatible with 0.17 by default).
  5. Turn default_manifest_yaml into a function (should be private API?) so we can use spack.config in it.

Further changes:

  1. Silence deprecatedProperties by allowing return None from the callback to signal it's not necessary to print yet another deprecation warning.
  2. Spack refused to save old format spack.yaml saying it's forward incompatible, ironically this error is backwards incompatible, so I've changed it into a warning.

We don't really have to introduce yet another flag spack env create --unify=... because spack -e . config add concretizer:unify:... exists.

@spackbot-app spackbot-app bot added commands environments tests General test capability(ies) labels Apr 13, 2022
@haampie haampie force-pushed the feature/spack-env-create-concretization-type-arg branch from cfa766b to 4572964 Compare April 20, 2022 09:27
@haampie haampie force-pushed the feature/spack-env-create-concretization-type-arg branch from 4572964 to f3d0666 Compare April 27, 2022 11:28
@tgamblin tgamblin added this to In progress in Spack v0.18.0 release via automation Apr 27, 2022
@haampie haampie force-pushed the feature/spack-env-create-concretization-type-arg branch from f3d0666 to 9a2b651 Compare May 23, 2022 11:05
@spackbot-app spackbot-app bot added defaults documentation Improvements or additions to documentation gitlab Issues related to gitlab integration labels May 23, 2022
@haampie haampie changed the title spack env create --concretize-[together|separately] Deprecate spack:concretization over concretizer:unify May 23, 2022
@haampie haampie force-pushed the feature/spack-env-create-concretization-type-arg branch from 6ad2049 to d16ea02 Compare May 23, 2022 12:00
@haampie
Copy link
Member Author

haampie commented May 23, 2022

After this PR:

$ cat spack.yaml
spack:
  specs:
  - zlib
  view: true
  concretization: separately
$ spack -e . concretize -f
==> Warning: concretization:separately is deprecated and will be removed in Spack 0.19 in favor of the new concretizer:unify:false config option.
==> Starting concretization
==> Environment concretized in 1.14 seconds.
==> Concretized zlib
[+]  hort4xo  zlib@1.2.12%clang@11.0.0+optimize+pic+shared arch=linux-ubuntu20.04-zen2

==> Warning: The environment "/tmp/tmp.2oLRDN70cu" is written to disk in a deprecated format. Please update it using:

	spack env update /tmp/tmp.2oLRDN70cu

Note that versions of Spack older than 0.18 may not be able to use the updated configuration.
$ ls
spack.lock  spack.yaml
$ spack -e . find
==> Warning: concretization:separately is deprecated and will be removed in Spack 0.19 in favor of the new concretizer:unify:false config option.
==> In environment /tmp/tmp.2oLRDN70cu
==> Root specs
zlib

==> 1 installed package
-- linux-ubuntu20.04-zen2 / clang@11.0.0 ------------------------
zlib@1.2.12
$ spack env update .
==> The environment "." is going to be updated to the latest schema format.
If the environment is updated, versions of Spack that are older than this version may not be able to read it. Spack stores backups of the updated environment which can be retrieved with "spack env revert"
==> Do you want to proceed? [y/N] y
==> Environment "." has been updated [backup=/tmp/tmp.2oLRDN70cu/spack.yaml.bkp]
$ spack -e . concretize -f
==> Starting concretization
==> Environment concretized in 1.17 seconds.
==> Concretized zlib
[+]  hort4xo  zlib@1.2.12%clang@11.0.0+optimize+pic+shared arch=linux-ubuntu20.04-zen2
$ cat spack.yaml
spack:
  specs:
  - zlib
  view: true
  concretizer:
    unify: false

@@ -28,3 +28,9 @@ concretizer:
# instance concretize with target "icelake" while running on "haswell").
# If "true" only allow targets that are compatible with the host.
host_compatible: true
# When "true" concretize root specs of environments together, so that each unique
# package in an environment corresponds to one concrete spec. This ensures
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Future work, writing about it here since this comment made me think of it -- we should add a feature that this setting is applied to specs from the command line when multiple specs are provided (e.g. spack install foo bar baz could co-concretize).


is_error = deprecated['error']
if not is_error:
llnl.util.tty.warn(msg)
warnings.warn(msg)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the switch to warnings.warn here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warnings gives a better way to filter things, even locally, and showing the warning is delegated to tty.warn:

def send_warning_to_tty(message, *args):
"""Redirects messages to tty.warn."""
tty.warn(message)

lib/spack/docs/environments.rst Outdated Show resolved Hide resolved

is_error = deprecated['error']
if not is_error:
llnl.util.tty.warn(msg)
warnings.warn(msg)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warnings gives a better way to filter things, even locally, and showing the warning is delegated to tty.warn:

def send_warning_to_tty(message, *args):
"""Redirects messages to tty.warn."""
tty.warn(message)

Co-authored-by: Massimiliano Culpo <massimiliano.culpo@gmail.com>
@haampie
Copy link
Member Author

haampie commented May 23, 2022

Other candidate names for this option? We also refer to it as "consistent" concretization elsewhere. And the new type of concretization (not in this PR) is "maximally consistent"?

Spack v0.18.0 release automation moved this from In progress to Reviewer approved May 23, 2022
@becker33 becker33 merged commit f7258e2 into spack:develop May 23, 2022
Spack v0.18.0 release automation moved this from Reviewer approved to Done May 23, 2022
@haampie haampie deleted the feature/spack-env-create-concretization-type-arg branch May 23, 2022 20:49
iarspider pushed a commit to iarspider/spack that referenced this pull request May 30, 2022
* Introduce concretizer:unify option to replace spack:concretization

* Deprecate concretization

* Make spack:concretization overrule concretize:unify for now

* Add environment update logic to move from spack:concretization to spack:concretizer:reuse

* Migrate spack:concretization to spack:concretize:unify in all locations

* For new environments make concretizer:unify explicit, so that defaults can be changed in 0.19
bhatiaharsh pushed a commit to bhatiaharsh/spack that referenced this pull request Aug 8, 2022
* Introduce concretizer:unify option to replace spack:concretization

* Deprecate concretization

* Make spack:concretization overrule concretize:unify for now

* Add environment update logic to move from spack:concretization to spack:concretizer:reuse

* Migrate spack:concretization to spack:concretize:unify in all locations

* For new environments make concretizer:unify explicit, so that defaults can be changed in 0.19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commands defaults documentation Improvements or additions to documentation environments gitlab Issues related to gitlab integration tests General test capability(ies)
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants