Skip to content

v26 usability: fix P0 (invalid track) + 4 P1s (CLI noise, bare exceptions, MCP debug)#43

Closed
lucapinello wants to merge 1 commit intomainfrom
fix/2026-04-23-v26-usability
Closed

v26 usability: fix P0 (invalid track) + 4 P1s (CLI noise, bare exceptions, MCP debug)#43
lucapinello wants to merge 1 commit intomainfrom
fix/2026-04-23-v26-usability

Conversation

@lucapinello
Copy link
Copy Markdown
Contributor

Summary

Picks up the actionable items from v26 usability audit. One code-correctness bug, four UX polish fixes.

P0 #9 — invalid track ID silently returned predictions for track 0

Both chorus/oracles/enformer.py:346 and chorus/oracles/borzoi.py:300 logged a warning and appended index 0 as a fallback when get_track_by_identifier() returned None. So:

oracle.predict(('chr1', 0, 100_000), ['ENCFF999BADID'])
# → warning "Identifier 'ENCFF999BADID' not found in metadata"
# → silently returns predictions for track 0 (an arbitrary other track)

User output corrupted with no recovery path.

Fix: raise InvalidAssayError naming the missing ID and pointing at oracle.get_track_info(). Also upgraded the env-runner subprocess templates (enformer_source/templates/predict_template.py, borzoi_source/…/predict_template.py) from bare raise Exception to a descriptive ValueError (v26 P1 #12 also).

Regression test: tests/test_prediction_methods.py::test_unknown_track_id_gives_actionable_error. Covers both the ENCFF-ID and description-search branches with a stubbed metadata object.

P1 #4 — leading INFO noise on every CLI invocation

Before:

$ chorus list
2026-04-23 20:21:55,540 - chorus.core.environment.manager - INFO - Found mamba via MAMBA_EXE: /Users/…/miniforge3/bin/mamba
2026-04-23 20:21:55,540 - chorus.core.platform - INFO - Detected platform: Darwin arm64 (key=macos_arm64, cuda=False)
Available oracle environments:

After:

$ chorus list
Available oracle environments:

Demoted both to logger.debug in core/environment/manager.py (3 lines) and core/platform.py (1 line). Diagnostic, not user-facing.

P1 #1chorus setup --oracle fakeoracle lists valid names

Before: Environment file not found: environments/chorus-fakeoracle.yml. No hint of valid names.

After: No environment file for oracle 'fakeoracle'. Expected …. Valid oracle names: ['alphagenome','borzoi','chrombpnet','enformer','legnet','sei'].

P1 #13 — bare raise Exception('') in interval.py

CIGAR parser fallback raised an empty Exception. Replaced with IntervalException naming the bad symbol and valid set ('=MIDX').

P1 #18 — MCP _safe_tool hides traceback

The decorator returns {"error", "error_type", "tool"} dicts on failure — MCP clients can't see which line raised.

Fix: opt-in CHORUS_MCP_DEBUG=1 env var includes "traceback" in the error payload. Default behaviour unchanged.

Not addressed here (separate PRs)

Test plan

  • pytest tests/ --ignore=tests/test_smoke_predict.py -q340 passed / 1 skipped (+1 new test)
  • chorus --help + chorus list → no leading INFO noise
  • chorus setup --oracle fakeoracle → lists valid oracle names
  • Manual: oracle.predict(..., ['ENCFF999BADID'])InvalidAssayError with get_track_info() pointer (both use_environment=True/False paths)
  • CHORUS_MCP_DEBUG=1 chorus-mcp preserves existing behaviour; unset behaves identically to main

🤖 Generated with Claude Code

…on, MCP debug)

Picks up the v26 usability audit's actionable items. One code correctness
bug, four UX polish fixes.

## P0 #9 — invalid track ID silently returned wrong predictions

Both chorus/oracles/enformer.py:346 and chorus/oracles/borzoi.py:300
logged a warning when get_track_by_identifier() returned None and
appended index 0 as a fallback, so oracle.predict(..., ['ENCFF999BAD'])
silently returned predictions for track 0 (an arbitrary other track).
User output corrupted with no error.

Replaced the fallback with:
  raise InvalidAssayError(
      f"<oracle> track identifier '{assay_id}' not found in metadata. "
      f"Use oracle.get_track_info() to list valid identifiers..."
  )

Same fix applied to the description-search branch. The env-runner
subprocess templates at enformer_source/templates/predict_template.py
and borzoi_source/.../predict_template.py previously raised
`Exception("Some assay IDs not found in metadata: [...]")` — upgraded
to `ValueError` with the same list_tracks pointer (v26 P1 #12).

Added regression test tests/test_prediction_methods.py::
test_unknown_track_id_gives_actionable_error.

## P1 #4 — leading INFO noise on every CLI invocation

`chorus --help` / `chorus list` / etc. used to lead with:
  INFO - Found mamba via MAMBA_EXE: ...
  INFO - Detected platform: Darwin arm64 ...

Demoted both to `logger.debug` — they're diagnostic, not user-facing.
Sites: core/environment/manager.py (3 lines), core/platform.py (1
line). Pytest still passes; tests don't depend on these being INFO.

## P1 #1 — `chorus setup --oracle fakeoracle` names valid oracles

Previously errored with "Environment file not found:
environments/chorus-fakeoracle.yml" — no hint of what names are
valid. Now reads:
  No environment file for oracle 'fakeoracle'. Expected ....
  Valid oracle names: ['alphagenome','borzoi','chrombpnet',
                       'enformer','legnet','sei']

## P1 #13 — bare `raise Exception('')` in interval.py

CIGAR parser fallback raised an empty Exception. Replaced with
IntervalException naming the bad symbol + valid set ('=MIDX').

## P1 #18 — MCP _safe_tool hides traceback

@_safe_tool returned {"error", "error_type", "tool"} dicts, so MCP
clients debugging a failing tool couldn't see which line raised.

Added opt-in: set CHORUS_MCP_DEBUG=1 to include a "traceback" field
in the error dict. Default behaviour unchanged.

## Not addressed in this PR

- P1 #10 (TF/absl startup noise): needs env-activation-script
  TF_CPP_MIN_LOG_LEVEL=3 — invasive, separate PR
- P1 #11 (env swallow silent fail): needs new exception type +
  flag-on-later-use — separate PR
- P2 items: polish pass after these land

## Tests

340 passed / 1 skipped in 8m 31s (was 339 — +1 regression test).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@lucapinello
Copy link
Copy Markdown
Contributor Author

Superseded by 96fc28d which landed 4 of my 9 files (the track-ID guards + INFO→DEBUG demotion). Opening a follow-up PR with just the 5 files that didn't land: template fixes (P1 #12), interval.py bare Exception (P1 #13), MCP CHORUS_MCP_DEBUG opt-in (P1 #18), and the regression test for the P0.

lucapinello added a commit that referenced this pull request Apr 24, 2026
…egression test (#44)

Picks up the parts of v26 my PR #43 fixed that didn't land in the
parallel 96fc28d commit. 96fc28d landed the track-ID _validate_assay_ids
override + INFO→DEBUG demotion (4 files). These 5 files close the
remaining items from v26's list.

## P1 #12 — subprocess templates raise bare Exception

enformer_source/templates/predict_template.py and
borzoi_source/templates/predict_template.py had

  raise Exception(f"Some assay IDs not found in metadata: {...}")

The env-runner wraps subprocess failures in RuntimeError, so the
user saw a generic traceback prefix instead of a recognisable
exception type. Upgraded to ValueError with a `get_track_info()`
pointer and a specific missing-IDs list (vs dumping the whole
assay_ids arg).

## P1 #13 — interval.py bare Exception('')

CIGAR parser fallback at interval.py:673 raised an empty Exception('')
— meaningless if a user ever hit it. Replaced with IntervalException
naming the bad symbol + valid set ('=MIDX').

## P1 #18 — MCP _safe_tool hides traceback

The decorator returns {"error", "error_type", "tool"} dicts — MCP
clients debugging a failing tool couldn't see which line raised.

Opt-in: CHORUS_MCP_DEBUG=1 includes a "traceback" field in the error
payload. Default behaviour unchanged.

## P0 #9 regression test

tests/test_prediction_methods.py::test_unknown_track_id_gives_actionable_error
exercises the EnformerOracle._validate_assay_ids path (96fc28d added
this override, but no regression test for it landed). Covers:
- unknown ENCFF ID → InvalidAssayError naming list_tracks/search_tracks
- bad description → same
- None/[] input → no-op (doesn't raise)

Stubs enformer_metadata.get_metadata so the test runs in the base
chorus env without TF.

## Tests

340 passed / 1 skipped in 9m 17s (+1 regression test from 339).

Co-authored-by: lp698 <lp698@dimm2fv07n65x.partners.org>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.

1 participant