Skip to content

fix(ui): a failed mass tolerance reload puts the old tolerance back - #305

Merged
Jack-Coutts merged 4 commits into
mainfrom
fix/209-failed-reload-restores-tolerance
Nov 10, 2026
Merged

Jack-Coutts merged 4 commits into
mainfrom
fix/209-failed-reload-restores-tolerance

Conversation

@Jack-Coutts

@Jack-Coutts Jack-Coutts commented Nov 10, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #209. Re-applies #227 onto main.

The bug

Saving a new mass tolerance stored the value before the EIC reload ran. When the reload failed, the error box appeared but the stored tolerance stayed at the new value. Settings, the export changelog and the session file then reported a tolerance that no stored EIC was built with.

The fix

apply_mass_tolerance remembers the tolerance in force before the reload. _mass_tolerance_reload_failed puts it back, but only when no EIC row can have been committed at the new tolerance. Exports and session files read self.mass_tolerance when they are written, and the app does not persist it anywhere else, so that one assignment reaches every place.

A reload commits each sample as it goes, so a worker exception does not mean nothing was rebuilt. regenerate_all_eics_with_mass_tolerance now raises NothingToRegenerateError (a RuntimeError) for its two checks that run before the sample loop: no active samples, and no compounds. The worker's failed signal carries a second value, eics_replaced, which is false only for that error. Any other exception may follow a commit, so the handler keeps the new value, as main does, and says so.

Why this signal. The importer already raises these two errors, so giving them a type adds no new control flow and catches nothing. The progress callback was the other candidate, but the importer does not call it after a commit, so it cannot tell "a sample was committed" from "a sample was skipped". An exception inside the loop before the first commit is treated as "may have committed". That keeps the new value in a case where reverting would also have been right, which is the safe direction.

The stored tolerance in each case:

  • Full success: the new value.
  • Partial failure (some samples rebuilt, others unreadable): the new value. The "Regeneration Incomplete" box ends with "The mass tolerance setting is now X Da."
  • Total failure (no sample could be read): the old value. A "Regeneration Failed" box lists the samples and says "The mass tolerance stays at X Da."
  • Exception before any commit (the reload cannot start, or there are no samples or no compounds): the old value, with the same "Regeneration Failed" box.
  • Exception after a commit may have happened (any other error from the worker): the new value. The "Regeneration Failed" box says "The mass tolerance setting is now X Da. Some samples may still hold EICs built at Y Da."

A failure in the natural abundance correction step after the EICs are rebuilt does not reach this handler. The importer already catches it and logs a warning, so the reload completes and keeps the new value.

The user guide's Mass Tolerance section says what happens in each of these cases.

Checks

  • tests/test_failed_reload_restores_tolerance.py saves a new tolerance through the Settings window and waits for the real reload thread. Without the change three cases fail: no samples (assert 0.35 == 0.2), the reload cannot start (assert 0.35 == 0.2) and no CDF readable (assert ['warning'] == ['critical']). The fourth case, one CDF readable, passes both ways and guards that a partial failure keeps the new value.
  • test_reload_that_stops_after_rebuilding_a_sample_keeps_the_new_tolerance uses two real CDFs. The second one's folder is unreadable, so cdf_path.exists() raises PermissionError after the first sample was committed at 0.35 Da. It asserts that the first sample's EIC holds [20.0, 20.0, 20.0] (signal that only 0.35 Da matches) and that the stored tolerance is 0.35. On 653b149 it fails with assert 0.2 == 0.35.
  • uv run python -m pytest -q: 676 passed.
  • bench/reference.py check: exported results unchanged.

Left out of #227

Its test patched QThread and called the worker by hand. The new tests use the real thread and real CDF files instead.

A worker exception can follow a sample whose EICs were already replaced
at the new tolerance. Revert only when the importer raised
NothingToRegenerateError before its sample loop, the reload could not
start, or no sample was rebuilt.
@Jack-Coutts

Copy link
Copy Markdown
Collaborator Author

Review verdict for head 22514ff: CLEAN after one fix round. Reviewed by GPT-6.1 Sol.

Round 1, at 653b149:

  • Blocker: a worker exception does not mean nothing was rebuilt. Sol reproduced it with real CDFs and the real reload thread. Two samples were loaded at 0.2 Da and the tolerance was changed to 0.35. Sample 1 committed EICs at 0.35, then a PermissionError on sample 2's CDF path reached the failure handler. The handler restored 0.2 Da, so exports and changelogs would label sample 1 with the wrong tolerance. main keeps 0.35 in this case.
  • What Sol ran: the full suite (675 passed), the new tests against main's source (3 failed, 1 passed), and probes of the start-failure path. That path closes the progress dialog and re-enables the window once.

Fix, 653b149..22514ff: the importer's two checks that run before its sample loop now raise NothingToRegenerateError, and the worker's failed signal reports whether EICs may already have been replaced. The tolerance goes back only when nothing can have been committed. After a later error it keeps the new value and says that some samples may still hold EICs at the old one. Sol's case was added as a test.

Round 2, the fix diff only:

  • Sol's probe now keeps 0.35 and shows the warning that names both tolerances.
  • The new test fails against 653b149's source with assert 0.2 == 0.35 and passes on the head.
  • Full suite: 676 passed.
  • The case where one sample rebuilds with zero rows while another fails does not leave a mismatch. The rows rebuilt at 0.2 Da are identical.
  • One emitter and two connections use the changed signal, and both are compatible.

The partial-reload exception on main is tracked in #307. Its marking half is being fixed in #310.

Jack-Coutts added a commit that referenced this pull request Nov 10, 2026
The rebuild bypasses apply_mass_tolerance, so #305's rollback would restore a
tolerance from an earlier change, or raise AttributeError when there was none.
The rebuild now records the current tolerance as the one to fall back to.
@Jack-Coutts
Jack-Coutts merged commit 5ea33eb into main Nov 10, 2026
10 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.

A failed mass tolerance reload keeps the new tolerance

1 participant