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

environment.py: concretize together by default #29942

Closed

Conversation

haampie
Copy link
Member

@haampie haampie commented Apr 7, 2022

We have somewhat conflicting defaults, namely view: true and
concretization: separately.

This leads to post-install errors when merging different flavors of the
same package into the default view.

It makes more sense to either disable views or do consistent
concretization by default.

Our docs hint that concretization: separately is aimed at system
administrators, which is likely a small percentage of spack users.
By now many people use environments to just build stuff, given that
repeated concretization is slow, spack install x doesn't even show
what gets built, etc, all better when using environments. And for those
users consistent concretization makes more sense.

We have somewhat conflicting defaults, namely `view: true` and
`concretization: separately`.

This leads to post-install errors when merging different flavors of the
same package into the default view.

It makes more sense to either disable views or do consistent
concretization by default.

Our docs hint that `concretization: separately` is aimed at system
administrators, of which there are likely fewer. By now there are likely
more using environments to just build stuff, given that concretization
is slow and spack.lock's are nice.
@spackbot-app spackbot-app bot added documentation Improvements or additions to documentation environments labels Apr 7, 2022
@tldahlgren tldahlgren requested review from alalazo, becker33, tgamblin and scheibelp and removed request for alalazo and becker33 April 7, 2022 22:05
@tldahlgren
Copy link
Contributor

... spack install x doesn't even show what gets built, etc,...

That statement surprises me. What is the context where you don't see Installing, Successfully installed, and [+] messages for packages that get built?

@scheibelp scheibelp self-assigned this Apr 7, 2022
concretized. *By default specs are concretized separately*, one after
the other. This mode of operation permits to deploy a full
software stack where multiple configurations of the same package
need to be installed alongside each other. Central installations done
Copy link
Member

Choose a reason for hiding this comment

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

This reasoning might be poorly explained:

  • We've since added stacks to support this (while that isn't specifically a problem this PR needs to address, this docs section should probably point to the section on stacks for this specific use case)
  • Admins might not be the only people who benefit: there may be users who want to install separate tools in a single environment, and it may be that those tools cannot be concretized together (i.e. if each root provides a different top-level executable, but each top-level executable might depend on mutually exclusive library configurations)

Do you think it might be worth adding an option similar to --with-view? I think it would be OK to switch the default but IMO we should provide an option so that users can control this from the command line.

Copy link
Member

Choose a reason for hiding this comment

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

This reasoning might be poorly explained:

Sorry to clarify: I mean that the documentation, as originally written, is missing details.

Copy link
Member

@scheibelp scheibelp left a comment

Choose a reason for hiding this comment

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

I think it should be possible to control this default with a new option --concretize-separately (which is false by default - as desired in this PR); I think this would make it easier to maintain scripts which generate environments (when users want to concretize separately).

It also occurred to me we might want to add a temporary warning message to Environment.concretize() when doing concretize_together (only when it fails): right now the concretization: choice isn't explicitly dumped into the config, so users who were depending on that behavior will see errors (but wouldn't know why that started happening).

@haampie
Copy link
Member Author

haampie commented Apr 12, 2022

I don't think we should introduce tons of new features and warnings, that's just overwhelming and the opposite of reducing the steep learning curve for newcomers.

Alternatively and much simpler, we can make spack env create and spack env activate --temp set concretization: together explicitly by default, yet keep separately as a default when loading existing spack.yaml files that do not have concretization: ... set, to be backwards compatible.

@haampie
Copy link
Member Author

haampie commented Apr 12, 2022

@tamara somehow missed your message. But I meant to say: it does not print an overview ahead of time like with a spack add ..., spack concretize, spack install workflow, where the concretize step shows you what is already installed and what should still be installed. Without environments, the alternative is spac spec -I ..., spack install ..., but that has the downside of waiting for concretization twice. So, I think most people would want to use environments all the time, because you concretize just once, and see what gets installed ahead of time.

@scheibelp
Copy link
Member

Alternatively and much simpler, we can make spack env create and spack env activate --temp set concretization: together explicitly by default, yet keep separately as a default when loading existing spack.yaml files that do not have concretization: ... set, to be backwards compatible.

That sounds good to me for most cases, but my concern was with users who might be running a nightly CI script which automatically generates the environment. I think this isn't critical since they can instantiate from a simple template like:

spack:
  specs: []
  concretization: separately

so I could see the benefit in not adding more options to the spack env command. However, that still creates a problem where their scripts will suddenly fail without a clear indication of why, so I still think that a debug-level message would be useful in Environment.concretize()

@tgamblin tgamblin added this to In progress in Spack v0.18.0 release via automation Apr 12, 2022
@tgamblin
Copy link
Member

adding this to v0.18.0 -- we should do something with this but whether we switch to together or best-effort per #28941 is TBD

@haampie haampie added this to In progress in Spack v0.19.0 release via automation May 12, 2022
@haampie haampie removed this from In progress in Spack v0.18.0 release May 12, 2022
@haampie
Copy link
Member Author

haampie commented May 12, 2022

This can be 0.19 earliest since it's breaking, #30038 should be 0.18 so this is less breaking.

@haampie
Copy link
Member Author

haampie commented Jul 28, 2022

Closing this, will replace with a new PR that just changes the default of unify

@haampie haampie closed this Jul 28, 2022
Spack v0.19.0 release automation moved this from In progress to Done Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change documentation Improvements or additions to documentation environments
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants