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
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?
|
Unfortuantely not |
Add missing date in CHANGELOG.md
erikrikarddaniel
left a comment
erikrikarddaniel
left a comment
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."
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 is correct. This is what Claude does not get: The module was renamed into importsegmentation because of nf-core/modules standards. Yet, the actual function from xenium ranger is import-segmentation. Thus the meta.ymls reference correctly that the module runs import-segmentation. |
This was tackled now with #202 |
My mistake again; I know the rules. |
I just approved that. |
Changes to tackle revision for hotfix 1.0.1
1.0.1 - Hotfix to solve some intitial bugs and template update
Tackling Issues #189 and #186
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.