Conversation
Co-authored-by: khersameesh24 <58815690+khersameesh24@users.noreply.github.com>
This reverts commit 7c26481.
dev branch version bump to 1.1.0dev
Important! Template update for nf-core/tools v4.0.2
…e-4.0.2 Revert "Important! Template update for nf-core/tools v4.0.2"
Template merge - `nf-core v4.0.2`
Bundle validation built child paths from `Path.toString()`, which drops the URI scheme (e.g. `s3://`) for remote filesystems. On object-storage work dirs (S3 on AWS Batch) the scheme-less path was resolved on the local filesystem, so every child-file check failed with `Xenium bundle does not exist` / `NoSuchFileException`, even though the UNTAR step succeeded. Use scheme-preserving `Path.resolve()` / `toUriString()` instead of `toString()` at all bundle-path construction sites. This fixes both the `-profile test` staging path (UNTAR output) and production runs where the samplesheet supplies an object-storage bundle. No change on local / shared-filesystem executors. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
xeniumranger import-segmentation was given --expansion-distance on every invocation, but the tool only accepts it for nuclei-based imports (it expands nuclei into cell boundaries). Transcript-assignment imports (proseg/baysor/segger) and cells-only imports fail with `ERROR: --expansion-distance requires --nuclei`. Gate the flag on the module's `nuclei` input so it is emitted only when a nuclei segmentation is being imported. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ndle-paths Preserve URI scheme when resolving Xenium bundle child paths
…lei-only Only pass --expansion-distance for nuclei-based xeniumranger imports
implement matthias recom, update to snaptshots and version bump.
Merged
|
Warning Newer version of the nf-core template is available. Your pipeline is using an old version of the nf-core template: 4.0.3. For more documentation on how to update your pipeline, please see the Synchronisation documentation. |
Merging from dev to 1.0.1
Fixing a mistake in solving the merge conflict
Co-authored-by: Matthias Hörtenhuber <mashehu@users.noreply.github.com>
Important! Template update for nf-core/tools v4.0.3
|
just minor change to CHANGELOG.md
FriederikeHanssen
left a comment
FriederikeHanssen
left a comment
There was a problem hiding this comment.
looks good to me. We can't test on AWS at the moment (see #pipeline-maintainers) channel. Do you have an AWS CE available to test your fix on?
Collaborator
Author
|
Unfortuantely not |
Add missing date in CHANGELOG.md
erikrikarddaniel
left a comment
erikrikarddaniel
left a comment
Member
There was a problem hiding this comment.
No test seems to cover the bug you fixed. In the words of my coworker Claude (modules/nf-core/xeniumranger/importsegmentation/tests/main.nf.test, lines 28–61): "Both test cases here (nuclei + stub) set nuclei to bundle + "/cells.zarr.zip" and expansion_distance to 5 — so they only exercise the branch in main.nf:37 that already worked before this fix (nuclei truthy → include --expansion-distance).
Neither case covers the actual bug: nuclei empty/falsy (transcript-assignment or cells-only import) with expansion_distance still set, which is exactly the scenario that used to error out with --expansion-distance requires --nuclei. Could you add a third case with nuclei: [] and expansion_distance: 5 that asserts the resulting command does not include --expansion-distance? As it stands, this fix could regress silently and the test suite wouldn't catch it."
And another note on tests from the same source (workflows/spatialaxe.nf, around line 148): "This fix (and the toUriString() calls at lines 227/244/264/350) is specifically about object-storage work dirs — the commit message itself notes "No change on local / shared-filesystem executors." Since our test suite runs on local storage, toString() and toUriString() produce identical results there, so the existing tests can't actually distinguish the buggy behavior from the fixed one — this could regress again without CI noticing.
Was this verified against a real S3 (or other object-storage) work directory, e.g. via an AWS full-test run on Seqera Platform? Not asking for a new automated test necessarily (that'd need real cloud infra in CI), just want to confirm it's been exercised somewhere before we ship it as a fix, and maybe note in the PR how it was checked."
Claude also discovered some meta.yml mistakes that should be easy to fix: "8 subworkflows still reference the old module name in their meta.yml (xeniumranger/import-segmentation) after the module was renamed to xeniumranger/importsegmentation — a mechanical follow-up that got missed: baysor_run_prior_segmentation_mask, baysor_run_transcripts_parquet, cellpose_baysor_import_segmentation, cellpose_resolift_morphology_ome_tif, proseg_preset_proseg2baysor, segger_create_train_predict, xeniumranger_import_segmentation_redefine_bundle, xeniumranger_resegment_morphology_ome_tif."
| include { BAYSOR_PREPROCESS_TRANSCRIPTS } from '../../../modules/local/baysor/preprocess/main' | ||
| include { BAYSOR_RUN } from '../../../modules/local/baysor/run/main' | ||
| include { XENIUMRANGER_IMPORT_SEGMENTATION } from '../../../modules/nf-core/xeniumranger/import-segmentation/main' | ||
| include { XENIUMRANGER_IMPORTSEGMENTATION } from '../../../modules/nf-core/xeniumranger/importsegmentation/main' |
Member
There was a problem hiding this comment.
Suggested change
| include { XENIUMRANGER_IMPORTSEGMENTATION } from '../../../modules/nf-core/xeniumranger/importsegmentation/main' | |
| include { XENIUMRANGER_IMPORTSEGMENTATION } from '../../../modules/nf-core/xeniumranger/importsegmentation/main' |
| include { XENIUM_PATCH_STITCH } from '../../../modules/local/xenium_patch/stitch/main' | ||
| include { RECONSTRUCT_PATCHES } from '../../../modules/local/utility/reconstruct_patches/main' | ||
| include { XENIUMRANGER_IMPORT_SEGMENTATION } from '../../../modules/nf-core/xeniumranger/import-segmentation/main' | ||
| include { XENIUMRANGER_IMPORTSEGMENTATION } from '../../../modules/nf-core/xeniumranger/importsegmentation/main' |
Member
There was a problem hiding this comment.
Suggested change
| include { XENIUMRANGER_IMPORTSEGMENTATION } from '../../../modules/nf-core/xeniumranger/importsegmentation/main' | |
| include { XENIUMRANGER_IMPORTSEGMENTATION } from '../../../modules/nf-core/xeniumranger/importsegmentation/main' |
| include { BAYSOR_RUN } from '../../../modules/local/baysor/run/main' | ||
| include { XENIUM_PATCH_STITCH } from '../../../modules/local/xenium_patch/stitch/main' | ||
| include { XENIUMRANGER_IMPORT_SEGMENTATION } from '../../../modules/nf-core/xeniumranger/import-segmentation/main' | ||
| include { XENIUMRANGER_IMPORTSEGMENTATION } from '../../../modules/nf-core/xeniumranger/importsegmentation/main' |
Member
There was a problem hiding this comment.
Suggested change
| include { XENIUMRANGER_IMPORTSEGMENTATION } from '../../../modules/nf-core/xeniumranger/importsegmentation/main' | |
| include { XENIUMRANGER_IMPORTSEGMENTATION } from '../../../modules/nf-core/xeniumranger/importsegmentation/main' |
| include { RESIZE_TIF } from '../../../modules/local/utility/resize_tif/main' | ||
| include { GET_TRANSCRIPTS_COORDINATES } from '../../../modules/local/utility/get_coordinates/main' | ||
| include { XENIUMRANGER_IMPORT_SEGMENTATION } from '../../../modules/nf-core/xeniumranger/import-segmentation/main' | ||
| include { XENIUMRANGER_IMPORTSEGMENTATION } from '../../../modules/nf-core/xeniumranger/importsegmentation/main' |
Member
There was a problem hiding this comment.
Suggested change
| include { XENIUMRANGER_IMPORTSEGMENTATION } from '../../../modules/nf-core/xeniumranger/importsegmentation/main' | |
| include { XENIUMRANGER_IMPORTSEGMENTATION } from '../../../modules/nf-core/xeniumranger/importsegmentation/main' |
| include { STARDIST as STARDIST_NUCLEI } from '../../../modules/nf-core/stardist/main' | ||
| include { CONVERT_MASK_UINT32 } from '../../../modules/local/utility/convert_mask_uint32/main' | ||
| include { XENIUMRANGER_IMPORT_SEGMENTATION } from '../../../modules/nf-core/xeniumranger/import-segmentation/main' | ||
| include { XENIUMRANGER_IMPORTSEGMENTATION } from '../../../modules/nf-core/xeniumranger/importsegmentation/main' |
Member
There was a problem hiding this comment.
Suggested change
| include { XENIUMRANGER_IMPORTSEGMENTATION } from '../../../modules/nf-core/xeniumranger/importsegmentation/main' | |
| include { XENIUMRANGER_IMPORTSEGMENTATION } from '../../../modules/nf-core/xeniumranger/importsegmentation/main' |
| include { PROSEG2BAYSOR } from '../../../modules/local/proseg/proseg2baysor/main' | ||
| include { XENIUM_PATCH_STITCH } from '../../../modules/local/xenium_patch/stitch/main' | ||
| include { XENIUMRANGER_IMPORT_SEGMENTATION } from '../../../modules/nf-core/xeniumranger/import-segmentation/main' | ||
| include { XENIUMRANGER_IMPORTSEGMENTATION } from '../../../modules/nf-core/xeniumranger/importsegmentation/main' |
Member
There was a problem hiding this comment.
Suggested change
| include { XENIUMRANGER_IMPORTSEGMENTATION } from '../../../modules/nf-core/xeniumranger/importsegmentation/main' | |
| include { XENIUMRANGER_IMPORTSEGMENTATION } from '../../../modules/nf-core/xeniumranger/importsegmentation/main' |
| include { SEGGER_PREDICT } from '../../../modules/local/segger/predict/main' | ||
| include { SEGGER_CREATE_DATASET } from '../../../modules/local/segger/create_dataset/main' | ||
| include { XENIUMRANGER_IMPORT_SEGMENTATION } from '../../../modules/nf-core/xeniumranger/import-segmentation/main' | ||
| include { XENIUMRANGER_IMPORTSEGMENTATION } from '../../../modules/nf-core/xeniumranger/importsegmentation/main' |
Member
There was a problem hiding this comment.
Suggested change
| include { XENIUMRANGER_IMPORTSEGMENTATION } from '../../../modules/nf-core/xeniumranger/importsegmentation/main' | |
| include { XENIUMRANGER_IMPORTSEGMENTATION } from '../../../modules/nf-core/xeniumranger/importsegmentation/main' |
| include { STARDIST as STARDIST_NUCLEI } from '../../../modules/nf-core/stardist/main' | ||
| include { CONVERT_MASK_UINT32 } from '../../../modules/local/utility/convert_mask_uint32/main' | ||
| include { XENIUMRANGER_IMPORT_SEGMENTATION } from '../../../modules/nf-core/xeniumranger/import-segmentation/main' | ||
| include { XENIUMRANGER_IMPORTSEGMENTATION } from '../../../modules/nf-core/xeniumranger/importsegmentation/main' |
Member
There was a problem hiding this comment.
Suggested change
| include { XENIUMRANGER_IMPORTSEGMENTATION } from '../../../modules/nf-core/xeniumranger/importsegmentation/main' | |
| include { XENIUMRANGER_IMPORTSEGMENTATION } from '../../../modules/nf-core/xeniumranger/importsegmentation/main' |
|
|
||
| include { XENIUMRANGER_RESEGMENT } from '../../../modules/nf-core/xeniumranger/resegment/main' | ||
| include { XENIUMRANGER_IMPORT_SEGMENTATION } from '../../../modules/nf-core/xeniumranger/import-segmentation/main' | ||
| include { XENIUMRANGER_IMPORTSEGMENTATION } from '../../../modules/nf-core/xeniumranger/importsegmentation/main' |
Member
There was a problem hiding this comment.
Suggested change
| include { XENIUMRANGER_IMPORTSEGMENTATION } from '../../../modules/nf-core/xeniumranger/importsegmentation/main' | |
| include { XENIUMRANGER_IMPORTSEGMENTATION } from '../../../modules/nf-core/xeniumranger/importsegmentation/main' |
| "meta": { | ||
| "nf-test": "0.9.5", | ||
| "nextflow": "25.10.4" | ||
| "nextflow": "26.04.6" |
Member
There was a problem hiding this comment.
Is this needed for the pipeline or an accident? I try to avoid upgrading this unless it's needed. Just close the comment if it's needed.
Collaborator
Author
It is beeing tested. The fix which is already implement was defintily tested by seqera on their platform. Yet all the new implementation as whole (inclduing template updates) were not tested on seqara platform or similar. Will be done! Coming back to that with an update to this comment. |
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
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.
1.0.1 - Hotfix to solve some intitial bugs and template update
Added
Fixed
--expansion-distancetoxeniumranger import-segmentationfor nuclei-based imports. It was applied to every import, but xeniumranger rejects it for transcript-assignment imports (proseg/baysor/segger) and cells-only imports withERROR: --expansion-distance requires --nuclei.s3://) when building Xenium bundle child paths, so bundle validation works when the work directory is on object storage (S3/GCS/Azure). PreviouslyPath.toString()dropped the scheme and the resulting path was resolved on the local filesystem, causingXenium bundle does not exist/NoSuchFileExceptionfailures on AWS Batch.Dependencies
None
Deprecated
None
PR checklist
nf-core pipelines lint).nextflow run . -profile test,docker --outdir <OUTDIR>).nextflow run . -profile debug,test,docker --outdir <OUTDIR>).CHANGELOG.mdis updated.