Skip to content

add log hint in modules create and add create containers to modules bump-versions#4374

Merged
mashehu merged 6 commits into
nf-core:devfrom
mashehu:add-container-create-info
Jul 15, 2026
Merged

add log hint in modules create and add create containers to modules bump-versions#4374
mashehu merged 6 commits into
nf-core:devfrom
mashehu:add-container-create-info

Conversation

@mashehu

@mashehu mashehu commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Partially taken from #4362 (everything except of running the container creation during modules creation)

@mashehu
mashehu requested a review from JulianFlesch July 6, 2026 10:08
@codecov

codecov Bot commented Jul 6, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 80.35714% with 33 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.81%. Comparing base (e3dbb4f) to head (40fe733).
⚠️ Report is 1 commits behind head on dev.

Files with missing lines Patch % Lines
nf_core/modules/bump_versions.py 83.90% 14 Missing ⚠️
nf_core/commands_modules.py 0.00% 13 Missing ⚠️
nf_core/modules/containers.py 90.90% 6 Missing ⚠️

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mashehu mashehu added this to the 4.1.0 milestone Jul 9, 2026
log.info("Created following files:\n " + "\n ".join(new_files))

if self.component_type == "modules":
log.info(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this the correct log level we want? Does this always show?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes, should always hint to use that after the creation of the module for now. later we will actually do this step here.

Comment thread nf_core/modules/containers.py Outdated
"""Build Seqera/Wave containers for ``components`` with a live progress display.

Shows an overall module bar plus, per module, the per-build spinners
(docker/singularity × platform) that :meth:`ModuleContainers.create` renders when a

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
(docker/singularity × platform) that :meth:`ModuleContainers.create` renders when a
(docker/singularity x platform) that :meth:`ModuleContainers.create` renders when a

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

\cross is the right symbol here 🤓

Comment thread nf_core/modules/containers.py Outdated
raise RuntimeError(f"{error_context}: could not parse JSON response") from e


def build_containers_with_progress(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am not sure if I like these static functions when they are heavily parametrized and not helping to instantiate objects, but rather expecting them as input.
They are obviously useful to simplify one specific complex flow, but here we expect a manager, components and then also the Path.

Can we redesign this a bit such that it e.g. only requires a Path and a Module name (str)?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

build_containers_with_progress lives in the same module as ModuleContainers so it is fair to assume knowledge about its instantiation. This would reduce that coupling in more distant logic that lives in command_modules etc.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

good idea

Comment thread nf_core/modules/bump_versions.py Outdated
components=modules,
)
ModuleContainers.check_tower_token()
failed_modules = build_containers_with_progress(manager, modules, self.directory)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We never pass force when building containers from this class. Should we resurface this?

@mashehu
mashehu force-pushed the add-container-create-info branch from 153052c to ff65f30 Compare July 14, 2026 13:27
@codspeed-hq

codspeed-hq Bot commented Jul 14, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

✅ 8 untouched benchmarks


Comparing mashehu:add-container-create-info (40fe733) with dev (facc035)

Open in CodSpeed

@mashehu
mashehu force-pushed the add-container-create-info branch from 6497a8a to 007bdde Compare July 14, 2026 19:25
@mashehu
mashehu force-pushed the add-container-create-info branch from 007bdde to 40fe733 Compare July 15, 2026 12:02
@mashehu
mashehu merged commit 4513429 into nf-core:dev Jul 15, 2026
122 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants