ASP-based solver: don't error for type mismatch on preferences by alalazo · Pull Request #41138 · spack/spack

alalazo · 2023-11-17T14:42:10Z

This PR discards type mismatches or failures to validate a package preference during concretization. The values discarded are logged as debug level messages. It also adds a config audit to help users spot misconfigurations in packages.yaml preferences.

alalazo · 2023-11-17T14:45:16Z

As an example audit, with:

packages:
  llvm:
    variants: ["+openmp+foo", "clang=absolutely"]

  zlib:
    variants: "+openmp +foo clang=absolutely"

we get the following warnings:

haampie · 2023-11-20T09:23:00Z

lib/spack/spack/solver/asp.py

-            spec.update_variant_validate(variant_name, values)
+            try:
+                spec.update_variant_validate(variant_name, values)
+            except spack.error.SpackError:


Do we have a more specific exception for this? Like VariantValueError or whatever?

Also, what about requirements?

Also, what about requirements?

Requirements are fine already. They will error, unless the requirement comes from the all: section in packages.yaml:

spack/lib/spack/spack/solver/asp.py

Lines 1710 to 1715 in 868a3c4

except Exception as e:

if rule.kind != RequirementKind.DEFAULT:

raise RuntimeError(

"cannot emit requirements for the solver: " + str(e)

) from e

continue

The overall idea there is that, since requirements are stricter, we shouldn't silently skip them if they cannot be applied.¹

Footnotes

Think of a typo like +opnmp. It would be better to error, than to skip and concretize to the opposite value. ↩

Why guard against a typo +opnmp if +openmp errors because the vast majority of the package don't have that variant. It's guarding against typos so in return you can do ... nothing?

I'll repeat. For requirements all: is permissive, specific packages are not.

See comment below

fixes spack#41134

haampie · 2023-11-22T14:27:11Z

To me the sensible thing to do is make requirements behave the same:

packages:
  all:
    require: +cuda

should require pkg +cuda for all pkg defining this variant.

That's the only way in which all-type variant requirements can possibly be useful, cause no variant exists (except for build_system) that is shared across all packages, making it impossible to use by definition.

alalazo · 2023-11-22T15:21:01Z

@haampie Reconstructed what is happening here. This dates back to #35037 We have all the semantic we need and an awful syntax. My proposal is to add syntactic sugar for the all: case in a separate PR. The idea is to map any simple string in the requirement to the infamous any_of: [..., "@:"].

haampie · 2023-11-23T07:26:11Z

The any_of concept is not exactly equivalent though, cause you're giving the concretizer the option to ignore it.

It's equivalent to a preference that's stronger than reuse?

alalazo · 2023-11-23T08:22:52Z

The any_of concept is not exactly equivalent though, cause you're giving the concretizer the option to ignore it.

Yes. It will be mostly equivalent due to the priority we give to requirements, but not exactly equivalent to a requirement specified on the package.

The fact that we are a bit more permissive with all: in the enforcement of the requirement is in line with favoring usability. With:

spack:
  zlib:
    require: +shared

users cannot concretize or solve anything that explicitly needs ~shared:

$ spack spec zlib~shared
==> Error: concretization failed for the following reasons:

   1. cannot satisfy a requirement for package 'zlib'.

With:

spack:
  all:
    require: 
    - any_of: ["+shared", "@:"]

explicits request are possible:

$ spack spec zlib~shared
Input spec
--------------------------------
 -   zlib~shared

Concretized
--------------------------------
 -   [email protected]%[email protected]+optimize+pic~shared build_system=makefile arch=linux-ubuntu20.04-icelake
 -       ^[email protected]%[email protected]~guile build_system=generic arch=linux-ubuntu20.04-icelake

We can also have the same semantic for the two cases, which favors consistency, but in that case the occasional concretization of ~shared requires changing the config. Whatever we decide to do, I'd do that in a following PR and document the behavior, since I think there will still be edge cases that need the other solution.

haampie · 2023-11-23T09:40:18Z

Variants aren't the only requirement you can add in all, would be weird if %gcc or target=... were ignored.

alalazo · 2023-11-23T09:46:10Z

Variants aren't the only requirement you can add in all, would be weird if %gcc or target=... were ignored.

They will be ignored when either explicitly set or required in a spec literal. That said, I also pointed out the other option and the consequence:

We can also have the same semantic for the two cases, which favors consistency, but in that case the occasional concretization of ~shared requires changing the config.

I don't have a preferred option, and both are doable.

haampie · 2023-11-23T10:00:07Z

I feel like "enforce when applicable" is the least surprising interpretation of requirements under all.

all:require:%gcc currently can be applied uniformly and people are already familiar with its semantics. To make the transition to non-binary packages non-breaking it can only mean "require gcc if the package needs a compiler".
all:require:+cuda can follow the same rule: enforce +cuda if the package defines the variant.

This leaves room for people to relax from strong requirement to "strong preference" with that any_of construct (for which we can always still improve syntax).

alalazo · 2023-11-23T10:01:56Z

So let's get this bug fix in and work on the improvement in the next PR?

…#41138) This commit discards type mismatches or failures to validate a package preference during concretization. The values discarded are logged as debug level messages. It also adds a config audit to help users spot misconfigurations in packages.yaml preferences.

This commit discards type mismatches or failures to validate a package preference during concretization. The values discarded are logged as debug level messages. It also adds a config audit to help users spot misconfigurations in packages.yaml preferences.

…#41138) This commit discards type mismatches or failures to validate a package preference during concretization. The values discarded are logged as debug level messages. It also adds a config audit to help users spot misconfigurations in packages.yaml preferences.

spackbot-app bot added core PR affects Spack core functionality stand-alone-tests Stand-alone (or smoke) tests for installed packages tests General test capability(ies) labels Nov 17, 2023

alalazo mentioned this pull request Nov 17, 2023

Backports v0.21.1 #41063

Merged

36 tasks

haampie reviewed Nov 20, 2023

View reviewed changes

alalazo added 4 commits November 20, 2023 22:26

ASP-based solver: don't error for type mismatch on preferences

715acac

fixes spack#41134

Add a config audit to spot issues with variant names

393e065

Add test marker

b1f435e

Be more specific with the exceptions that could be raised

645ee8d

alalazo force-pushed the bugfix/preference-not-error branch from 847311f to 645ee8d Compare November 20, 2023 21:26

alalazo requested a review from haampie November 21, 2023 08:43

haampie approved these changes Nov 23, 2023

View reviewed changes

haampie merged commit ee0d3a3 into spack:develop Nov 23, 2023

alalazo deleted the bugfix/preference-not-error branch November 23, 2023 10:31

	except Exception as e:
	if rule.kind != RequirementKind.DEFAULT:
	raise RuntimeError(
	"cannot emit requirements for the solver: " + str(e)
	) from e
	continue

Conversation

alalazo commented Nov 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alalazo commented Nov 17, 2023

Uh oh!

haampie Nov 20, 2023

Choose a reason for hiding this comment

Uh oh!

haampie Nov 20, 2023

Choose a reason for hiding this comment

Uh oh!

alalazo Nov 20, 2023

Choose a reason for hiding this comment

Footnotes

Uh oh!

haampie Nov 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alalazo Nov 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haampie Nov 22, 2023

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

haampie commented Nov 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alalazo commented Nov 22, 2023

Uh oh!

haampie commented Nov 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alalazo commented Nov 23, 2023

Uh oh!

haampie commented Nov 23, 2023

Uh oh!

alalazo commented Nov 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haampie commented Nov 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alalazo commented Nov 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

alalazo commented Nov 17, 2023 •

edited

Loading

haampie Nov 22, 2023 •

edited

Loading

alalazo Nov 22, 2023 •

edited

Loading

haampie commented Nov 22, 2023 •

edited

Loading

haampie commented Nov 23, 2023 •

edited

Loading

alalazo commented Nov 23, 2023 •

edited

Loading

haampie commented Nov 23, 2023 •

edited

Loading