Skip to content

Correct the CI timing comments and test notes after #253 - #255

Merged
MaxGhenis merged 5 commits into
mainfrom
tidy-ci-test-wording
Nov 3, 2026
Merged

MaxGhenis merged 5 commits into
mainfrom
tidy-ci-test-wording

Conversation

@MaxGhenis

@MaxGhenis MaxGhenis commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

This PR corrects the CI timing comments in ci.yml and the test notes in CLAUDE.md:

  • The timeout-minutes comment compares each parallel run with a serial run on the same base. At b0c8f2e the serial job took 29.5 minutes, against 12.5 to 16.2 with -n auto. At f4c3c38 it took 22.3, against 12.3 and 15.7 with -n logical.
  • The --dist loadfile comment names the 4-vCPU VM and gives both modes' times.
  • In CLAUDE.md, the -n 4 line says the command runs the same tests as CI, which starts one worker per logical CPU.
  • CLAUDE.md lists what the tests mock (LiteLLM's model calls, the provider batch adapters and gh) and drops a PolicyEngine API that no test calls. It also names two calls that reach the network. LiteLLM fetches its model cost map from GitHub in every process that imports it, and some tests/test_scenarios.py tests download the certified US dataset from Hugging Face.

The PR changes comments and docs only, and ci.yml loads to the same workflow data as on main.

axiom: n/a: docs and CI comments only, no policy logic

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LwQq6FeG5zNPJXgBp8j1qj
@vercel

vercel Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
policybench-site Ready Ready Preview Nov 3, 2026 3:53am UTC

Request Review

@MaxGhenis

Copy link
Copy Markdown
Contributor Author

Ready for hub review

Head: df612c4d24524d3913f5f7bbd4b5b599c66f055e. This is the first and only commit, on branch tidy-ci-test-wording from origin/main at 8ac64ac (#253). main hasn't moved past 8ac64ac, so I applied every item to the text #253 merged. The push to tidy-ci-test-wording was accepted, so I didn't need a claude/ branch.

Items

  1. ci.yml, the comment above timeout-minutes: 30. Replaced with the brief's text, which gives both -n logical runs (12.3 and 15.7 min, runs 41526298328 and 41529016470).
  2. ci.yml, the --dist loadfile comment. Now reads "on a 4-vCPU VM it ran faster than --dist load (9.2 to 9.7 minutes, against 10.1)."
  3. CLAUDE.md, the -n 4 line. Now reads "to run the same tests on four workers (CI starts one per logical CPU)."
  4. CLAUDE.md, the two mocking claims. The "Testing" line now names the tests/test_scenarios.py tests that download the certified US dataset from Hugging Face. The "Quick start" comment now reads # Run tests (see "Testing" for the one download).

Checked against #253's record

  • Run the CI test job in parallel with pytest-xdist #253's description. Each number in items 1 and 2 matches the description's Times table:

    • b0c8f2e: 29.5 min (run 41486949933).
    • ec5413c: cancelled at the 30-minute limit on 2 Nov (run 41495271792).
    • -n auto: 12.7, 12.5 and 16.2 min, so "12.5 to 16.2" holds (run 41490222217).
    • -n logical: 12.3 min (41526298328) and 15.7 min (41529016470).
    • VM loadfile: 9:09 to 9:39. VM load: 10:08.
  • Actions API. I recomputed the job times from the API's start and completion stamps:

    • 41486949933: 29:32.
    • 41490222217: attempts 12:43, 12:30 and 16:14.
    • 41526298328: 12:15.
    • 41529016470: 15:41.

    ec5413c's run concluded cancelled. The test-job logs for 41526298328 and 41529016470 each print created: 4/4 workers.

  • Item 4, from the code. Only policybench/policyengine_runtime.py materialize_certified_us_dataset calls hf_hub_download. Its callers include make_us_microsimulation.

  • Item 4, empirically. This container's proxy refuses huggingface.co (403). I unshallowed the clone and ran uv run pytest -m "not slow" -n logical --dist loadfile on main + this change. Result: 27 failed, 7,038 passed, 89 skipped (8:53).

    • All 27 failures are in tests/test_scenarios.py, and each one is httpx.ProxyError: 403 Forbidden.
    • The proxy's failure log during the run lists only huggingface.co:443.
    • No other test file reached the network.
    • These are the same 27 tests review r1 listed on Run the CI test job in parallel with pytest-xdist #253.
  • Where the items came from. Run the CI test job in parallel with pytest-xdist #253's issue comments hold only review r1 (at 4491519), and Run the CI test job in parallel with pytest-xdist #253 has no PR reviews or line comments. The description mentions a review r2 at efcecfc, but I found no comment on GitHub that lists these four items. So I checked each item's facts against the description, the Actions API and the code, not against the review text itself.

Checks run

  • uv sync --locked --extra dev
  • uv run ruff check .: all checks passed.
  • uv run ruff format --check .: 191 files already formatted.
  • yaml.safe_load on .github/workflows/ci.yml: parses, and jobs.test.timeout-minutes is 30.
  • The local full suite above.

CI at df612c4

All four checks passed: lint, app, test and Vercel Preview Comments (run 41568839231). The test job started 4/4 workers and gave 7,064 passed, 90 skipped in 13:40 (job 14:07).

Judgment calls

  • I kept the brief's wording verbatim, since every fact held.
  • In the PR body I added the session link line after the Claude Code footer.
  • I left README.md:60, which also says to run plain pytest, alone, because the brief scoped this PR to ci.yml and CLAUDE.md.

The PR is still a draft. I haven't marked it ready, merged it or approved it.


Generated by Claude Code

The timeout comment compared main's serial test job at b0c8f2e with
parallel runs on merges with f4c3c38. It now gives each base's serial time
beside its parallel runs: 29.5 minutes at b0c8f2e against 12.5 to 16.2 with
-n auto, and 22.3 at f4c3c38 against 12.3 and 15.7 with -n logical.

CLAUDE.md listed a PolicyEngine API among the mocked calls, though no test
calls one. The list now names LiteLLM, provider batch adapters and gh, and
says tests that need policyengine-us run it in-process.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
LiteLLM fetches its model cost map from GitHub when a pytest process
imports it (litellm/__init__.py and get_model_cost_map at v1.95.0), so the
tests reach the network for more than the Hugging Face download. CLAUDE.md
now names both calls and the variable that skips LiteLLM's fetch, and the
quick start points to network use instead of one download.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The policybench.cli runs that some slow tests start import LiteLLM
too, so they fetch the cost map as well. Only some tests in
tests/test_scenarios.py download the certified US dataset.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Under pytest-xdist the controller collects no tests, so it never
imports LiteLLM; each worker does, as does a serial run's one process.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@MaxGhenis
MaxGhenis marked this pull request as ready for review November 3, 2026 04:04
@MaxGhenis
MaxGhenis merged commit 7bccd96 into main Nov 3, 2026
5 checks passed
@MaxGhenis

MaxGhenis commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor Author

The PolicyBench owner session (Claude Opus 5.5) squash-merged this at the reviewed head 1e2a4269 as 7bccd967.

  • An independent Claude Opus 5.5 reviewer approved 1e2a4269 with no blocking finding. It checked the new CLAUDE.md clause against pytest-xdist 3.8.0: the controller collects no tests, so only workers and a serial run's one process import LiteLLM. Its one optional note says a run limited to files that never import LiteLLM skips the fetch. The owner session kept the line, since every command the file documents collects the whole suite.
  • An earlier review's three optional wording points went into d978ddcd and 1e2a4269.
  • CI run 41574596805 passed at 1e2a4269, and GitHub reported the PR mergeable with no requested changes.

🤖 Generated with Claude Code

This branch was successfully deployed

1 active deployment
Preview — 1e2a4269 Deployed Nov 3, 2026 by vercel[bot]
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.

2 participants