Skip to content

Budget the trade CLI tests for the engine import their CLI pays - #784

Merged
juaristi22 merged 1 commit into
mainfrom
fix-trade-cli-timeout
Aug 26, 2026
Merged

Budget the trade CLI tests for the engine import their CLI pays#784
juaristi22 merged 1 commit into
mainfrom
fix-trade-cli-timeout

Conversation

@juaristi22

@juaristi22 juaristi22 commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes the red main run after #766 merged (run 32910539480). One test failed — test_us_trade_entries_cli.py::test_cli_fails_when_margins_missing_from_window — with subprocess.TimeoutExpired after 300 s.

The test spawns the CLI over a two-row parquet and expects an immediate exit 1. It passed on #766's final run, which tested a merge ref with the same content: nothing landed between that run (21:36) and the merge (23:23). Same code, same group, 380 passed there against 379 passed, 1 failed here, and near-identical job wall clock (1281 s vs 1263 s).

Why the margin was thin

The CLI pays for machinery the trade code never touches. tools/build_us_import_entries.py imports us_runtime.us_trade.cbp_entry_stats, which executes us_runtime/__init__, which pulls in spine_agreement and through it the whole policyengine_us system.

Measured on the failing test's own fixture:

environment time to exit 1
engines installed (the engine-us lane) 35.9 s
engine-free 5.4 s

-X importtime attributes it: us_runtime 32.9 s → spine_agreement 30.9 s → policyengine_us 8.7 s. build_us_import_entry_margins.py (used by test_us_trade_imdb_bulk.py) carries the identical 33.0 s chain — checked rather than assumed, which is why its four spawn sites are in this change too.

That import is parameter-tree file I/O, so it stretches with runner disk contention. A 300 s budget left under 10× headroom on a call whose useful work is milliseconds.

The change

CLI_TIMEOUT_SECONDS = 900 at the six full-CLI spawn sites across both trade test files, with the measurements recorded next to the number so the next reader knows what the budget is for. The lightweight -c spawns keep their 60 s — they do not import the chain.

What this does not do

It buys headroom; it does not remove the cost. The cost disappears by deferring the engine import in us_runtime/__init__, which would cut ~30 s from every one of these spawns and speed up any other tool importing a us_runtime submodule. That is US source with unclear load-bearing behavior, so it belongs to its owners as a follow-up rather than a hotfix — flagging it here rather than filing separately, per the current preference not to open US-side issues.

Verification

  • Both affected files pass against the change in an engine environment: 91 passed in 10m15s.
  • ruff check clean on both files.
  • A re-run of the unfixed job on main was started to confirm non-determinism; result noted in a comment.

🤖 Generated with Claude Code

`test_cli_fails_when_margins_missing_from_window` timed out on main
(run 32910539480) after passing on #766's final run, which tested a merge ref
with the same content — nothing landed between that run and the merge. Same
code, same group, 380 passed there against 379 plus one timeout here, so the
break is marginal timing rather than logic.

The margin is thin because the CLI pays for machinery it never uses. Importing
`us_runtime.us_trade.*` executes `us_runtime/__init__`, which pulls in
spine_agreement and through it the whole policyengine-us system. Measured on
the failing test's own fixture — a CLI that reads a two-row parquet and exits
1 — that is 35.9 s with the engine installed against 5.4 s without, and
`build_us_import_entry_margins.py` carries the identical 33.0 s chain. The
import is parameter-tree file I/O, so it stretches with runner disk contention;
300 s left under 10x headroom on a call whose useful work is milliseconds.

Name the budget and raise it to 900 s at the six full-CLI spawn sites across
both trade test files, with the measurements recorded where the number lives.
The lightweight `-c` spawns keep their 60 s: they do not import the chain.

This buys headroom; it does not remove the cost. The cost disappears by
deferring the engine import in `us_runtime/__init__`, which would also cut
~30 s from each of these spawns — US source with unclear load-bearing
behavior, so it is filed for its owners rather than attempted in a hotfix.

Verified: both files pass against the change, 91 passed in 10m15s in an
engine environment.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@juaristi22

Copy link
Copy Markdown
Collaborator Author

Non-determinism confirmed — main is green again

I re-ran the failed job on main with no code change. It passed:

run of engine-us (3.13, us-not) on 2263df36 result wall clock
original 1 failed, 379 passed, 6 skipped 1263 s
re-run, identical commit 380 passed, 6 skipped 1415 s

Run 32910539480 is now success across all 23 jobs, so main is unblocked and this PR is not urgent — it removes the fragility rather than unsticking anything.

Note the re-run took 1415 s against 1263 s for the failing run, a 12% swing on the same commit. That is the runner variance the 300 s budget was sitting inside: when the group's work stretches, so does the 33 s engine import each CLI spawn pays, and the spawn nearest the edge is the one that tips.

This also rules out the alternative explanations I was holding open — it is not something that landed in #743 or #747 (both merged before #766's final green run), and not a deterministic break from the regrouping, since the identical grouping passes on re-run.

One consequence worth stating plainly: the same fragility exists on main today at five other spawn sites that happened not to tip — the two in test_us_trade_entries_cli.py and four in test_us_trade_imdb_bulk.py, all paying the same 33 s chain against the same 300 s budget. That is the argument for landing this rather than treating a green re-run as the end of it.

@juaristi22
juaristi22 merged commit 10bfa17 into main Aug 26, 2026
22 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.

1 participant