Conversation
QPX_OPENMSCONSENSUS called build_mudata() + mdata.write() directly. That
bypasses qpx's own writer, which checks
missing = required_modalities - set(mdata.mod)
and refuses a MuData missing a required quantification modality.
build_mudata catches per-modality failures, logs them and continues, so a
modality that failed to build was simply absent: the module wrote an
INCOMPLETE h5mu and the task exited 0. On MSV000085836 (TMT) that shipped
a view with proteins and no precursors, from a pyarrow offset overflow
(bigbio/qpx#316).
Also make *.h5mu an optional output. It was made required, which inverted
qpx's contract: the parquet views are the dataset's source of truth and
the MuData view is best-effort, so a view that cannot be built must not
destroy an otherwise complete run. Combined with the writer's guard the
behaviour is now: complete view, or no view and a warning - never a
silently partial one.
write_dataset_mudata also writes via a temporary file and removes a stale
h5mu when the build fails, which the inline heredoc did not.
Requires qpx 1.1.4: write_dataset_mudata was extracted in bigbio/qpx#312
and the overflow fixed in bigbio/qpx#316. Container and conda pins bumped.
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
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.
The problem
QPX_OPENMSCONSENSUScallsbuild_mudata()+mdata.write()directly, which bypasses qpx's own writer.write_dataset_mudatachecks:build_mudatacatches per-modality build failures, logs them, and continues — so a modality that fails to build is simply absent from the result. The module then wrote that incomplete MuData and the task exited 0.This is not theoretical. Validating the 1.1.4 container against MSV000085836 (TMT, 552 runs x 10 channels) produced:
A 451 MB h5mu with no precursors, reported as success. The underlying overflow is fixed in bigbio/qpx#316 (that dataset now yields
['precursors', 'proteins'], 988 MB), but the module would have shipped the partial view for any future modality failure — so the guard matters independently of that bug.Changes
write_dataset_mudata. Gets the modality guard, plus a tmp-file write and removal of a stale h5mu when the build fails — neither of which the inline heredoc did.*.h5muoptional. It had been made a required output, which inverted qpx's contract: the parquet views are the dataset's source of truth and the MuData view is explicitly best-effort. A view that cannot be built should not destroy an otherwise complete run — which is exactly what happened on MSV000085836, where a ~1h19 conversion succeeded and the run died afterwards on the h5mu step.Net behaviour: a complete view, or no view plus a warning — never a silently partial one.
Scope check
out.mudatais consumed only by this module's own nf-test (assert process.out.mudata.size() == 1), which exercises the stub and still passes; no workflow consumes the channel, sooptional: truechanges nothing downstream.