perf(components): regenerate container configs once per top-level install/update#4276
Closed
pinin4fjords wants to merge 3 commits into
Closed
perf(components): regenerate container configs once per top-level install/update#4276pinin4fjords wants to merge 3 commits into
pinin4fjords wants to merge 3 commits into
Conversation
…tall/update `install` and `update` were re-running `nextflow inspect` (via `try_generate_container_configs`) once for every transitive module dependency instead of once per command. Each call is JVM startup plus pipeline parse (~17 s), so a subworkflow with 18 module deps spent ~5 minutes in container-config regeneration alone. - `install.py`: gate the call on `not silent`. Recursive installs from `install_included_components` pass `silent=True`. Also fixes top-level subworkflow installs, which previously skipped regeneration entirely because of the `component_type == "modules"` guard. - `update.py`: hoist the call out of the per-component loop to after it, and skip when `--save-diff` is in effect (no changes are applied to the working tree). Measured against nf-core/rnaseq `pr-1680`, warm cache: subworkflows install --force bam_dedup_umi 5m27s → 39s subworkflows update --update-deps --no-preview ... 4m 4s → 44s Repeating "Could not find meta.yml" warning lines 36 → 2 Two new tests mock `try_generate_container_configs` and assert it is invoked exactly once for an install/update that touches multiple module dependencies. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Add a regression test for `nf-core modules install`: regen runs once for a single top-level module install. - Add a regression test for `--save-diff`: regen must be skipped during a dry-run update so the pipeline's containers_*.config files don't drift while the user is still reviewing the patch. - Shorten the in-source comments at the two call sites. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
|
Closing in favour of #4277, which removes the Leaving the tests and PR body here for reference in case any of the regression assertions (top-level subworkflow install/update regenerates configs once; |
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
1 participant
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.
Improvement
nf-core modules/subworkflows installandupdatewere re-runningnextflow inspect(viatry_generate_container_configs) once for every transitive module dependency rather than once per command. Each call is JVM startup plus pipeline parse (~17 s).Move the call out of the recursive path:
nf_core/components/install.py: gate onnot silent. Recursive installs passsilent=True. Also runs at the end of top-level subworkflow installs, which previously skipped regeneration entirely because of thecomponent_type == "modules"guard.nf_core/components/update.py: hoist out of the per-component loop to after it. Skip when--save-diffis set (no changes applied to the working tree).Behaviour notes (worth flagging in review)
subworkflows install/updatenow regenerates container configs. Previously skipped entirely because ofcomponent_type == "modules". Aligns with the original intent of Command to generate pipeline container config files #3955.--save-diffupdates explicitly do not regenerate (dry run).nf-core modules install/update: behaviour unchanged.Measured impact
nf-core/rnaseq
pr-1680, warm cache:subworkflows install --force bam_dedup_umisubworkflows update --update-deps --no-preview bam_dedup_umiCould not find meta.ymlwarning linesTests
tests/subworkflows/test_install.py::test_subworkflows_install_regenerates_container_configs_once— subworkflow with multiple module deps, regen exactly once.tests/subworkflows/test_update.py::test_subworkflow_update_regenerates_container_configs_once—--update-depscascade, regen exactly once.tests/subworkflows/test_update.py::test_subworkflow_update_save_diff_skips_container_configs—--save-diffdry run, regen zero times.tests/modules/test_install.py::test_modules_install_regenerates_container_configs_once— top-level module install regression, regen exactly once.Validation requested
nf-core subworkflows install <X>now regenerates container configs at the top level (previously skipped).nf-core modules install <X>still regenerates exactly once (unchanged).nf-core ... update --save-diff <X>does not regenerate.