Skip to content

concretization: move spec concretization logic to spack.concretize#47971

Merged
haampie merged 1 commit intodevelopfrom
imports/spec-concretize-cycle
Jan 15, 2025
Merged

concretization: move spec concretization logic to spack.concretize#47971
haampie merged 1 commit intodevelopfrom
imports/spec-concretize-cycle

Conversation

@becker33
Copy link
Copy Markdown
Member

@becker33 becker33 commented Dec 7, 2024

This resolves a circular import issue between spack.spec and spack.concretize. It requires removing the Spec.concretize and Spec.concretized methods and updating all call-sites to use spack.concretize.concretized instead.

This will help with potential future efforts to separate AbstractSpec and ConcreteSpec classes.

New import relationship is spack.concretize imports from spack.spec, but not the other way around.

@haampie this got a bit bigger than I expected.

@spackbot-app spackbot-app bot added commands core PR affects Spack core functionality tests General test capability(ies) labels Dec 7, 2024
@becker33 becker33 force-pushed the imports/spec-concretize-cycle branch 2 times, most recently from 7eb4ae5 to 4efc7d9 Compare December 7, 2024 00:32
@haampie
Copy link
Copy Markdown
Member

haampie commented Dec 7, 2024 •
edited
Loading

Haha, I expected it would be big since it's a popular function call :p

I've taken the liberty to rebase 4efc7d9 on 05acd29 (which also drops a circular import), fixed a test and added a missing import. Curiously I need to do from x import y type of inline imports, cause otherwise flake8 complains and so does the python language server in my editor.

I also tried to move spack.concretize.CHECK_COMPILER_EXISTENCE into spack.solver.asp which would fix another circularity, but in practice the import-check action reported the same number of problematic imports. Since that global is removed in the compilers as deps branch, let's leave it.

Further, spack.solver.asp cannot be a top-level import in concretize.py due to this cycle:

concretize -> solver/asp -> binary_distribution -> user_environment -> build_environment -> subprocess_context -> environment/__init__ -> environment/environment -> concretize

of which the import-check action suggests to remove the subprocess_context -> environment edge. But this PR is already big enough...

haampie
haampie reviewed Dec 7, 2024
View reviewed changes
haampie
haampie reviewed Dec 7, 2024
View reviewed changes
@haampie haampie force-pushed the imports/spec-concretize-cycle branch 3 times, most recently from 9877aa8 to 5ca590c Compare December 7, 2024 21:35
@haampie haampie mentioned this pull request Dec 8, 2024
Closed
@haampie haampie force-pushed the imports/spec-concretize-cycle branch from ce4da25 to 74a6b61 Compare December 9, 2024 09:08
haampie
haampie previously approved these changes Dec 9, 2024
View reviewed changes
haampie
haampie reviewed Dec 11, 2024
View reviewed changes
@becker33 becker33 mentioned this pull request Dec 11, 2024
Merged
@becker33 becker33 force-pushed the imports/spec-concretize-cycle branch from cf3addc to aa50b0d Compare December 11, 2024 19:31
haampie
haampie reviewed Dec 12, 2024
View reviewed changes
@haampie
Copy link
Copy Markdown
Member

haampie commented Dec 16, 2024

@becker33 can you rebase?

tgamblin
tgamblin previously requested changes Dec 20, 2024
View reviewed changes
@haampie haampie force-pushed the imports/spec-concretize-cycle branch 2 times, most recently from fce788e to 608a2f5 Compare January 6, 2025 09:54
@haampie haampie requested a review from tgamblin January 6, 2025 13:18
@haampie haampie force-pushed the imports/spec-concretize-cycle branch 4 times, most recently from 4e11eb8 to 0e12749 Compare January 6, 2025 18:31
haampie
haampie previously approved these changes Jan 9, 2025
View reviewed changes
@haampie haampie force-pushed the imports/spec-concretize-cycle branch from 0e12749 to 4ab84ef Compare January 13, 2025 09:24
@spackbot-app spackbot-app bot added the documentation Improvements or additions to documentation label Jan 13, 2025
@haampie haampie force-pushed the imports/spec-concretize-cycle branch 2 times, most recently from 73a211f to 8589fdd Compare January 14, 2025 08:44
@becker33 @haampie
Deprecate Spec.concretize* methods
ac640c0 
This resolves a circular import issue between spack.spec and
spack.concretize. It requires removing the `Spec.concretize` and
`Spec.concretized` methods and updating all call-sites to use
`spack.concretize.concretized` instead.

This will help with potential future efforts to separate AbstractSpec
and ConcreteSpec classes.

New import relationship is `spack.concretize` imports from `spack.spec`,
but not the other way around.
@haampie haampie force-pushed the imports/spec-concretize-cycle branch from 8589fdd to ac640c0 Compare January 14, 2025 10:17
alalazo
alalazo approved these changes Jan 15, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@alalazo alalazo left a comment

Choose a reason for hiding this comment

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

I just skimmed through tests, since they seem to all have the same API change. I checked asp.py and spec.py more thoroughly and they LGTM.

@haampie haampie dismissed tgamblin’s stale review January 15, 2025 08:43

review from alalazo

@haampie haampie merged commit e7c591a into develop Jan 15, 2025
@haampie haampie deleted the imports/spec-concretize-cycle branch January 15, 2025 09:13
mtaillefumier pushed a commit to mtaillefumier/spack that referenced this pull request Jan 20, 2025
@becker33
Deprecate Spec.concretize/Spec.concretized in favor of spack.concreti…
c6eebe6 
…ze.concretize_one (spack#47971)

The methods spack.spec.Spec.concretize and spack.spec.Spec.concretized
are deprecated in favor of spack.concretize.concretize_one.

This will resolve a circular dependency between the spack.spec and
spack.concretize in the next Spack release.
teaguesterling pushed a commit to teaguesterling/spack that referenced this pull request Feb 5, 2025
@becker33 @teaguesterling
Deprecate Spec.concretize/Spec.concretized in favor of spack.concreti…
c47ad2e 
…ze.concretize_one (spack#47971)

The methods spack.spec.Spec.concretize and spack.spec.Spec.concretized
are deprecated in favor of spack.concretize.concretize_one.

This will resolve a circular dependency between the spack.spec and
spack.concretize in the next Spack release.
mrmundt pushed a commit to mrmundt/spack that referenced this pull request Feb 17, 2025
@becker33 @mrmundt
Deprecate Spec.concretize/Spec.concretized in favor of spack.concreti…
9a876ad 
…ze.concretize_one (spack#47971)

The methods spack.spec.Spec.concretize and spack.spec.Spec.concretized
are deprecated in favor of spack.concretize.concretize_one.

This will resolve a circular dependency between the spack.spec and
spack.concretize in the next Spack release.
@greenc-FNAL greenc-FNAL mentioned this pull request Feb 25, 2025
Merged
@haampie haampie mentioned this pull request Feb 12, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alalazo alalazo alalazo approved these changes

@haampie haampie haampie left review comments

@tgamblin tgamblin Awaiting requested review from tgamblin

Assignees

@haampie haampie

Labels

breaking-change commands core PR affects Spack core functionality documentation Improvements or additions to documentation tests General test capability(ies)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@becker33 @haampie @tgamblin @alalazo