Repository navigation
Conversation
…lect Without --only, og summary collect reads all three groups over one walk of the folder and runs the post pass: the readings that join sections, the cyp2d6 block, not_run, not_assessed, the database re-check and new_since. A part of the post pass that fails makes only its own section unreadable; the summary is still written. Bead: pgp-bs9u.44
Nine cases run og summary collect without --only against bin/collect_summary.py on folders that join the three groups' fixtures, with failed steps, an empty folder, kept runs in history/ and a database re-check. A telomere table with an infinite read length stopped the Python with an uncaught OverflowError; og keeps the summary with that section unreadable (the all-inf golden). Bead: pgp-bs9u.44
Bead: pgp-bs9u.44
not_run keeps the sections that are no step, a stale VNtyper reading counts as assessed, new_since takes the current run's date, and a panic in a post-pass part is no longer contained. The next commit removes them. Bead: pgp-bs9u.44
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 3 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
# Conflicts: # CHANGELOG.md
Reverts 888cd36, the red control of this PR. Bead: pgp-bs9u.44
The images check reads any image name in a tracked file as one that bypasses versions.env; the fixture's DEEPVARIANT_IMAGE rows now hold plain version text. Bead: pgp-bs9u.44
This branch has not been deployed
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.
When one reading that joins two sections of summary.json fails, the Python collector loses the whole file. These readings are the repeat readings, Locityper's primary calls, CNVpytor's chrY filter, the CYP2D6 block and what was not assessed. They run after the per-section error handling. So one bad table there leaves no summary.json, and both reports fail with it. A reader that stops on an error the Python does not catch has the same effect. A telomere table with an infinite read length does this today: the Python stops with an
OverflowErrorand writes nothing.The fix
og summary collectwithout--onlynow writes the whole summary.json. It reads all 48 sections over one walk of the sample folder. Groups a, b and c are read at the same time. Then it runs the post pass:not_run,not_assessed,recheckandnew_since.Each part of the post pass that changes a section runs on its own. When it fails, with an error or a panic, only that section becomes
unreadable. Its note names the file. The other sections stay, and the summary is written. A previous run whose summary cannot be read leavesnew_sinceout, as before. The comparison itself returns typed results, so a bug in it is no longer read as "no previous run".The post pass is its own module,
rust/crates/og-summary/src/collect/post.rs, so the next lane (the switch of the reports to og) builds on it without a merge. Groups a and b now read through a sharedFolder(one walk, the run status and the manifest), so the whole collect walks the folder once.Nothing calls the whole collect yet. The reports still read the summary that
bin/collect_summary.pywrites. No output file, key or column changes.Decisions:
--only,--outis optional and defaults tosummary.jsonin the sample folder, as the Python did.--onlystill needs--out, so a part of the sections never replaces the complete file.OverflowError, which loses the whole file in the Python.OverflowError: cannot round, the existing group c reader's note. It is unchanged here.What differs from the Python on purpose (also in CHANGELOG): a post-pass part that fails makes only its section unreadable; a reader error the Python did not catch keeps the summary. The
all-infgolden pins the second.How it was checked
not_runkept the sections that are no step, a stale VNtyper reading counted as assessed,new_sincetook the current run's date, and a panic in a post-pass part was no longer caught. On the merged control head d3ff258, Guard's rust job failed in og-summary only, 97 passed and 5 failed, each on a planted fault. Guard's parity job reported 7 of 4,210 items different, all in the new whole-file cases and all from the faults ($.not_runlengths and$.new_since.previous_utc). Commit af005e2 removes the faults. The other runs on that head were cancelled.summary-collectcases run og andbin/collect_summary.pyat the oracle commit on the same folders, compared as JSON with key order and onlygenerated_utcmasked. The new generatortests/parity/gen/summary-collect-full.shjoins the three groups' fixtures into one folder, so the post pass reads across groups. It adds failed steps, an empty folder, kept runs inhistory/(one half-copied, one that is no JSON), a database re-check, and the default output path. The oracle gets the realrun_delta.pyat the oracle commit. 8 cases agree;all-infholds the golden. Locally,ORACLE_RUNNER=local scripts/ci/oracle-diff.sh summary-collect roh-summary: all 295 items in 74 cases agree, including the 62 older subtree cases after the refactor.oracle-diff.sh --self-test: all 15 checks passed.cargo test -p og-summary --no-fail-fast, 102 passed. 13 are new, incollect/post/tests.rs: section order,not_runand its failed steps, eachnot_assessedline, containment of an error and a panic, the telomere case, the declared sex across groups,new_sincewith a half-copied folder and three unreadable previous runs, the re-check block, and the command line. rustfmt, clippy with-D warningsandcargo docwith-D warningspass.scripts/ci/check-images.sh,scripts/ci/check-personal-data.shand shellcheck on the changed scripts pass./usr/bin/time -l, three runs each, same box. On the joined folder with every section: Python 0.15 to 0.21 s and 42 to 44 MB peak RSS; og 0.01 to 0.03 s and 15 to 16 MB. The same folder with a synthetic 1,000,000-record VCF: Python 1.53 to 1.54 s and 42 to 44 MB; og 0.44 to 0.62 s and 17 to 20 MB. Both outputs are the same JSON, with onlygenerated_utcmasked.Files outside the package list, each needed for the parity proof or for docs that stay true:
rust/Cargo.lock(og-summary depends on og-runlog fornew_sinceandrecheck),rust/og/src/main.rs(one usage line),rust/README.mdanddocs/rust.md(one line each),tests/parity/cases.tsv(9 rows),tests/parity/gen/summary-collect-full.sh(new),tests/parity/golden/summary-collect/all-inf/(new) andtests/parity/inputs/summary-collect/oracle.sh(one comment). The branch merges main once, for a CHANGELOG conflict.Bead: pgp-bs9u.44. The T3 real-genome check is run by the orchestrator before landing.