Conversation
CodeGat
added
type:enhancement
…find the manifest
…dated manifest at end
|
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 |
|
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 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 |
👍
True. As long as The nice thing about using the name-space is that it would avoid above, and if you can't get |
|
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. |
I guess not.
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. |
Co-authored-by: Aidan Heerdegen <[email protected]> Signed-off-by: Tommy Gatti <[email protected]>
|
There is potential to need to move |
CodeGat
added
version:MAJOR
…t remove version information
* Injection of Modules over Checks (#295) * Add projection 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 * Run injection.projection and injection.prerelease modules * deploy-1-setup: Remove projection checks * Remove unneeded mergedeep requirement * Add module inits to folders * Add step for installing dependencies, update names for checkout steps * Used cd module prefix rather than custom working-directory so we can find the manifest * Added required input for prerelease modification, always print out updated manifest at end * Add back EOT * Update non-root-spec projections to namespaced `{name}/prX-Y/VERSION` format * Updated tests for namespaced non-root-spec projections * Update prerelease module argument from --root-spec-version to --version * Pass in sys.argv to parse_args, add tests * Refactor workflow to split generation of root spec and packages * Rename projections script to modules * Add inject_includes function * For test inputs, comment what is expected * Reordered functions * Updated tests after code updates * Apply suggestions from code review 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 alphabetically sorting the includelist since it always comes first * Updated output of manifest * Ran black * Added getter module for accessing various parts of a spack manifest * Update `[test_]modules.py` to use new getter module * Update `[test_]prerelease.py` to use getter module, update deploy-2-start to remove --root-spec arg * Ran black again * Raise NotImplementedErrors, add comments for successful matches * Remove comments of old code * Apply suggestions from code review Co-authored-by: Aidan Heerdegen <[email protected]> Signed-off-by: Tommy Gatti <[email protected]> * Add example to multi-target spec to test function * Parameterized tests * Fixes following more test cases * Converted manifest with no packages to fixture * Improved from_file classmethod tests * Parameterized valid and mistakenly invalid tests * Remove `root-spec` arg and instead get from manifest directly, update tests * Add new prerelease script flag `--keep-root-spec-intact`, that doesn't remove version information --------- Signed-off-by: Tommy Gatti <[email protected]> Co-authored-by: Aidan Heerdegen <[email protected]> * `vars` to File-Based Special Packages (#305) * Validate MDR config/packages.json * Pass info from MDR config/packages.json to module injection script * Update variable name to `vars.CONFIG_PACKAGES_SCHEMA_VERSION` * Change cwd relative to build-cd rather than model for scripts * Add output of packages to be injected/added to provenance DB * Update prerelease projection to `ROOT_SPEC/prX-Y/{name}/VERSION` * Update tests for new prerelease injection logic * Replace all internal refs with v6 * Dump quoted strings for prerelease projections, updated expected output * Schema Versions as Entrypoint Inputs Over `vars` (#310) * Change schema vars to required inputs, add optional spack manifest schema path for potentially different schemas * Add required schema version inputs, and optional spack manifest schema paths to entrypoint workflows * Get packages for provenance from an earlier step * Update prerelease injection to the form `ROOT_SPEC/.dependencies/PRX-Y/VERSION-{hash:7}` * Fixed up an unused variable * Remove `.` from `dependencies directory --------- Signed-off-by: Tommy Gatti <[email protected]> Co-authored-by: Aidan Heerdegen <[email protected]>
2 participants
Closes #169
References #276
Background
Having to update versions of packages in multiple places (
spack.packages.*.require[0]andspack.modules.tcl.default.projectionsmostly) 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
Testing
Python Unit Tests
I've added tests for all the important functions in the
scripts.spack_manifest.injection.modulesandscripts.spack_manifest.injection.prereleasescripts.These can be invoked with
python3 -m pytestto run the full suite ofbuild-cdtests, orpython3 -m pytest tests/scripts/spack_manifestfor 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.
E2E Workflow Tests
For E2E tests, I used an ephemeral branch
169-inject-projections_TESTthat used the same refs for all intra-pipeline workflow calls, and tested in onACCESS-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.