parse_specs: special case for concretizing lookups quickly#47556
Merged
parse_specs: special case for concretizing lookups quickly#47556
Conversation
tgamblin
requested changes
Nov 12, 2024
Member
tgamblin
left a comment
tgamblin
left a comment
There was a problem hiding this comment.
couple suggestions but otherwise LGTM
Member
|
@haampie this looks good to me at first glance -- see if this fixes the perf regressions you saw. |
Signed-off-by: Todd Gamblin <[email protected]>
Signed-off-by: Todd Gamblin <[email protected]>
tgamblin
approved these changes
Nov 12, 2024
tgamblin
previously requested changes
Nov 12, 2024
tgamblin
reviewed
Nov 12, 2024
Signed-off-by: Todd Gamblin <[email protected]>
tgamblin
force-pushed
the
bugfix/fast-spec-lookups
branch
from
4af1191 to
ff572fb
Compare
haampie
reviewed
Nov 12, 2024
haampie
reviewed
Nov 12, 2024
alalazo
reviewed
Nov 12, 2024
Signed-off-by: Todd Gamblin <[email protected]>
tgamblin
force-pushed
the
bugfix/fast-spec-lookups
branch
from
acd61d0 to
e032658
Compare
Member
|
Ok I think this is ready to go when the tests pass so feel free to approve if you agree. |
Member
|
This doesn't work in the one example I tried it on: specs = spack.cmd.parse_specs(args.specs, concretize=False)
specs = spack.cmd.matching_specs_from_env(specs) # <- concretizes |
This was referenced Nov 25, 2024
fryeguy52
pushed a commit
to fryeguy52/spack
that referenced
this pull request
Dec 17, 2024
We added unification semantics for parsing specs from the CLI, but there are a couple
of special cases in which we can avoid calls to the concretizer for speed when the
specs can all be resolved by lookups.
- [x] special case 1: solving a single spec
- [x] special case 2: all specs are either concrete (come from a file) or have an abstract
hash. In this case if concretizer:unify:true we need an additional check to confirm
the specs are compatible.
- [x] add a parameterized test for unifying on the CI
---------
Signed-off-by: Todd Gamblin <[email protected]>
Co-authored-by: Todd Gamblin <[email protected]>
This was referenced Jan 6, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#44843 added unification semantics for parsing specs from the CLI, but there are a couple special cases in which we can avoid calls to the concretizer for speed when the specs can all be resolved by lookups.
special case 1: solving a single spec
special case 2: all specs are either concrete (come from a file) or have an abstract hash. In this case if
concretizer:unify:truewe need an additional check to confirm the specs are compatible.