Injection of Modules over Checks #295

CodeGat · 2025-07-09T04:13:03Z

Closes #169
References #276

Background

Having to update versions of packages in multiple places (spack.packages.*.require[0] and spack.modules.tcl.default.projections mostly) has long been a source of errors and annoyance for users.

Rather than checking user manifests and reporting errors, we will now inject the relevant projections and includes into the manifest, and not override existing user projections.

As a little bonus, I've also added a module version of the prerelease modifications that are done in CD, too, so it's independently testable.

The PR

Add modules injection module
Add requirements for injection modules
Add tests for injection projection module, and inputs/requirements
Add prerelease injection module
Add tests for prerelease injection module
deploy-2-start: Run injection.modules and injection.prerelease modules
deploy-1-setup: Remove projection checks

Testing

Python Unit Tests

I've added tests for all the important functions in the scripts.spack_manifest.injection.modules and scripts.spack_manifest.injection.prerelease scripts.

These can be invoked with python3 -m pytest to run the full suite of build-cd tests, or python3 -m pytest tests/scripts/spack_manifest for just the new ones.

Local Module Runthrough

Can be done via the following commands. They will be most representative if the projections section is removed, or using your own manifests.

# For testing projection injection into the manifest
python3 -m scripts.spack_manifest.injection.modules --manifest tests/scripts/spack_manifest/injection/inputs/spack.yaml --root-spec access-om2 --packages "mom5 cice5 oasis3-mct libaccessom2"`

# For testing prerelease modifications to the manifest
python3 -m scripts.spack_manifest.injection.prerelease --manifest tests/scripts/spack_manifest/injection/inputs/prerelease.spack.yaml --root-spec access-om2 --version pr12-12

E2E Workflow Tests

For E2E tests, I used an ephemeral branch 169-inject-projections_TEST that used the same refs for all intra-pipeline workflow calls, and tested in on ACCESS-TEST. See ACCESS-NRI/ACCESS-TEST#48.

Inject Projections As Prerelease

In this run, you can see the original manifest both after projections are injected (done both in Prerelease and Release) and then after prerelease modifications are made. The injections are also visible in the artifacts generated by the run, see at the bottom of the summary in the metadata artifact - https://github.com/ACCESS-NRI/ACCESS-TEST/actions/runs/16161252034?pr=48

Inject Projections Where They Already Exist

I also tested injecting projections where they were already defined, and it succeeded - see ACCESS-NRI/ACCESS-ESM1.5#38. This could be for the case where users haven't got around to deleting the projections section.

…find the manifest

…dated manifest at end

aidanheerdegen · 2025-07-09T23:42:10Z

In the test run the pre-release injection results in the following change to projections:

         projections:
-          access-test: '{name}/2025.06.001'
+          access-test: '{name}/pr48-7'
           access-test-component: '{name}/main-{hash:7}'

So only the main spec is modified. Is that how we're currently doing it? Do we not get module namespace collisions because the components have their spack hash included, so if they're the same then it's all cool? What about when we delete the environment, do the module files get cleaned up too?

I'm just wondering if we're doing this, maybe we should add the PR namespace to the component as well, e.g.

access-test-component: '{name}/pr48-7/main-{hash:7}'

Then we could cleanly remove all module files within the pr48-* directories.

CodeGat · 2025-07-09T23:47:43Z

Yep, that's how we're currently doing it. And yeah, we don't get collisions because of the spack hash.

When we delete the environment, the modulefiles are not cleaned up, unfortunately. See #155 - still finding a way to clean them up. I've tried a bunch of spacks ways to remove the modulefiles (spack module tcl rm, etc) but it seems to mess with existing modulefiles as well. Maybe doing a spack -e ENV module tcl rm?

And yes it could be good to put the PR namespace into the components - I'll investigate that. If we can do an environment-aware module tcl rm we may not need to, though.

aidanheerdegen · 2025-07-09T23:57:40Z

And yes it could be good to put the PR namespace into the components - I'll investigate that.

👍

If we can do an environment-aware module tcl rm we may not need to, though.

True. As long as spack knows which environments are using a tcl module file. If there are multiple, and it doesn't know this, then removing for one environment could break another.

The nice thing about using the name-space is that it would avoid above, and if you can't get spack to do what you want, then it's simple to remove them manually.

CodeGat · 2025-07-10T00:42:12Z

Should this forced namespacing be the case for projections supplied by the user?

If so, this goes against the rationale of not overriding user projections.
And if not, we don't really have a way of knowing what projections are specified by the user in the prerelease module (since projection injection happens first). .

aidanheerdegen · 2025-07-10T01:12:32Z

Should this forced namespacing be the case for projections supplied by the user?

I guess not.

If so, this goes against the rationale of not overriding user projections. And if not, we don't really have a way of knowing what projections are specified by the user in the prerelease module (since projection injection happens first). .

It's going to be vanishingly rare for people to bother specifying their own projections. So in that case we live with there being some funky modules sitting around I suppose. Can always clean up anything older than, say, 12 months.

aidanheerdegen

I'll submit this review (half finished) now so I can pull in the most recent commits and I don't get confused between what I've done already and what is new.

scripts/spack_manifest/injection/projections.py

aidanheerdegen

Looks really good, thanks. I do have some suggestions still though.

scripts/spack_manifest/injection/modules.py

Co-authored-by: Aidan Heerdegen <[email protected]> Signed-off-by: Tommy Gatti <[email protected]>

…y sorting the includelist since it always comes first

…tart to remove --root-spec arg

aidanheerdegen

I have reviewed some ... still some more to look through.

Edit: not sure actually, so I'll leave it as-is and let me know if you need another review.

scripts/spack_manifest/getter.py

aidanheerdegen · 2025-07-17T02:53:10Z

scripts/spack_manifest/getter.py

+    # def get_spack_version(self) -> str:
+    #     pass
+
+    ## Get variant information
+
+    # def get_variants(self) -> list[str]:
+    #     pass
+
+    ## Get compiler information
+
+    # def get_compiler(self) -> str:
+    #     pass
+
+    # def get_compiler_name(self) -> str:
+    #     pass
+
+    # def get_compiler_version(self) -> str:
+    #     pass
+
+    ## Get architecture information
+
+    # def get_arch(self) -> str:
+    #     pass


Is the intention to provide these methods at a later date?

If so, would it be better to not have them commented, but instead return a NotImplemented exception?

Done in 9ac842d

aidanheerdegen · 2025-07-17T02:54:31Z

scripts/spack_manifest/getter.py

+    # def get_full_version(self) -> str:
+    #     pass
+
+    def get_ref(self) -> str:


Would be good to have a comment with an example or two of specs it would match against

Also done in 9ac842d

aidanheerdegen · 2025-07-17T03:39:08Z

scripts/spack_manifest/getter.py

+        if requirements == []:
+            raise NoSectionComponentError(f"Package component 'version' could not be extracted from the package requirements string for '{name}'.")
+
+        match = re.match(r"@(?:git\.)?([^+~=% ]+)", requirements[0])


Would be good to have a comment illustrating the uses cases and the expected match.

Done in 9ac842d

aidanheerdegen · 2025-07-17T03:40:56Z

scripts/spack_manifest/injection/modules.py

+    # defined_projections: dict[str, str] = _get_defined_projections(manifest)
+    # defined_projections_set: set[str] = set(defined_projections.keys())


I guess the intention is to remove the commented code?

tests/scripts/spack_manifest/test_getter.py

aidanheerdegen · 2025-07-17T04:41:35Z

tests/scripts/spack_manifest/test_getter.py

+            }
+        }
+
+        expected = ["root-spec", "package2", "package3"]


As above, could just compare directly to manifest['spack']['modules']['default']['tcl']['include'], or defined expected first and use that to populate manifest.

Fair enough, done via 804e577

tests/scripts/spack_manifest/test_getter.py

Co-authored-by: Aidan Heerdegen <[email protected]> Signed-off-by: Tommy Gatti <[email protected]>

CodeGat added 7 commits July 9, 2025 13:54

Add projection injection module

4212ec5

Add requirements for injection modules

6178077

Add tests for injection projection module, and inputs/requirements

c49ffae

Add prerelease injection module

cfb3fbd

Add tests for prerelease injection module

4ec0fda

Run injection.projection and injection.prerelease modules

9499ca4

deploy-1-setup: Remove projection checks

a2531b4

CodeGat added type:enhancement Improvements to existing features priority:high version:MINOR Doesn't require update to Model Deployment Repositories for:v5 Applies to `v5` labels Jul 9, 2025

CodeGat self-assigned this Jul 9, 2025

CodeGat added this to Model Release Jul 9, 2025

github-project-automation bot moved this to New Issues 🌅 in Model Release Jul 9, 2025

CodeGat added 6 commits July 9, 2025 14:15

Remove unneeded mergedeep requirement

671c2ee

Add module inits to folders

dc9611b

Add step for installing dependencies, update names for checkout steps

f9e668b

Used cd module prefix rather than custom working-directory so we can …

0abcacf

…find the manifest

Added required input for prerelease modification, always print out up…

1d2f2ee

…dated manifest at end

Add back EOT

0de367c

CodeGat marked this pull request as ready for review July 9, 2025 05:47

CodeGat moved this from New Issues 🌅 to Review 👏 in Model Release Jul 9, 2025

This was referenced Jul 9, 2025

Remove references to the now-injected Projections ACCESS-NRI/model-deployment-template#30

Draft

Create a Prerelease: Remove reference to projections ACCESS-NRI/access-hive.org.au#960

Draft

CodeGat mentioned this pull request Jul 10, 2025

TEST: Projection Injection on Existing Projections ACCESS-NRI/ACCESS-ESM1.5#38

Closed

aidanheerdegen reviewed Jul 10, 2025

View reviewed changes

CodeGat added 7 commits July 11, 2025 11:20

Pass in sys.argv to parse_args, add tests

b034315

Refactor workflow to split generation of root spec and packages

1b90737

Rename projections script to modules

641336c

Add inject_includes function

2b5139f

For test inputs, comment what is expected

5723425

Reordered functions

ad12a1e

Updated tests after code updates

24ec2f2

CodeGat requested a review from aidanheerdegen July 14, 2025 05:15

CodeGat changed the title ~~Injection Projections over Checks~~ Injection of Modules over Checks Jul 14, 2025

aidanheerdegen reviewed Jul 15, 2025

View reviewed changes

CodeGat and others added 8 commits July 15, 2025 13:47

Apply suggestions from code review

3425d10

Co-authored-by: Aidan Heerdegen <[email protected]> Signed-off-by: Tommy Gatti <[email protected]>

Add root_spec to includes set, but remove it later when alphabeticall…

6d575f3

…y sorting the includelist since it always comes first

Updated output of manifest

1539551

Ran black

587c536

Added getter module for accessing various parts of a spack manifest

bcd64b8

Update [test_]modules.py to use new getter module

6bc53c6

Update [test_]prerelease.py to use getter module, update deploy-2-s…

68828df

…tart to remove --root-spec arg

Ran black again

a1222bd

CodeGat requested a review from aidanheerdegen July 17, 2025 01:53

aidanheerdegen requested changes Jul 17, 2025

View reviewed changes

CodeGat and others added 9 commits July 18, 2025 11:36

Raise NotImplementedErrors, add comments for successful matches

9ac842d

Remove comments of old code

bd61938

Apply suggestions from code review

cd5b4da

Co-authored-by: Aidan Heerdegen <[email protected]> Signed-off-by: Tommy Gatti <[email protected]>

Add example to multi-target spec to test function

6375b8a

Parameterized tests

804e577

Fixes following more test cases

d9ba4a1

Converted manifest with no packages to fixture

cac77e8

Improved from_file classmethod tests

a00418c

Parameterized valid and mistakenly invalid tests

1a0f1b8

		# defined_projections: dict[str, str] = _get_defined_projections(manifest)
		# defined_projections_set: set[str] = set(defined_projections.keys())

Injection of Modules over Checks #295

Are you sure you want to change the base?

Injection of Modules over Checks #295

Uh oh!

Conversation

CodeGat commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Background

The PR

Testing

Python Unit Tests

Local Module Runthrough

E2E Workflow Tests

Inject Projections As Prerelease

Inject Projections Where They Already Exist

Uh oh!

aidanheerdegen commented Jul 9, 2025

Uh oh!

CodeGat commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aidanheerdegen commented Jul 9, 2025

Uh oh!

CodeGat commented Jul 10, 2025

Uh oh!

aidanheerdegen commented Jul 10, 2025

Uh oh!

aidanheerdegen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aidanheerdegen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aidanheerdegen left a comment • 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CodeGat commented Jul 9, 2025 •

edited

Loading

CodeGat commented Jul 9, 2025 •

edited

Loading

aidanheerdegen left a comment •

edited

Loading