Skip to content

[MC-1] NACHOS automated scoring engine - initial code migration#11

Open
CSR2017 wants to merge 9 commits into
mainfrom
MC-1
Open

[MC-1] NACHOS automated scoring engine - initial code migration#11
CSR2017 wants to merge 9 commits into
mainfrom
MC-1

Conversation

@CSR2017

@CSR2017 CSR2017 commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

No description provided.

Copilot AI review requested due to automatic review settings July 21, 2026 20:24

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review this pull request because it exceeds the maximum number of lines (20,000). Try reducing the number of changed lines and requesting a review from Copilot again.

@touchdownllc touchdownllc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Suganya — this is a solid port. The rename is thorough (no stray poc3 anywhere in code, compiles clean, ruff green), and the additions on top are genuinely useful: the rate-limit-aware retry, the Windows-safe cache paths, and the publish-stage progress rendering are all improvements over POC-3.

Because the diff is too large for GitHub to render, I reviewed it by diffing the branch against POC-3 main with the rename normalized out (~950 lines of real change), then ran it locally: installed the package, ran mc ingest az end to end (artifacts match POC-3 main's output), ran mc publish --dry-run, and ran POC-3's test suite against the branch with the renames applied. That last step is where most of the findings below come from: 2,201 of 2,220 tests pass, and all 19 failures trace to real changes in this PR.

Four things need to be fixed before this merges; the rest can follow.

Blocking

1. The test suite and CI didn't come over.
POC-3 main has 1,959 tests across 84 files plus a tests.yml workflow (Ubuntu and Windows). None of it is in this PR, but pyproject.toml still carries the full pytest configuration pointing at the missing tests/, and docstrings throughout cite test files as behavioral contracts. The suite ports almost mechanically — a rename pass got 2,201 passing on this branch — and it immediately caught the regression in item 2. Please bring tests/ and tests.yml over in this PR, or stack a PR that lands before anything else builds on main.

2. score run-all now fails without an API key before doing any work.
runner.py (line 360) calls build_runtime_client(model=model) at the top of run_all() just to resolve the model namespace, and the fallback branch constructs AnthropicClient, which raises when ANTHROPIC_API_KEY is unset. On POC-3 main a key was only needed when an LLM extract pair was actually reached; deterministic-facts and cached runs worked keyless. Repro:

env -u ANTHROPIC_API_KEY mc score run-all --state AZ --lens source --yes
# → raw RuntimeError traceback before any stage runs

This accounts for 13 of the 19 test failures (test_score_runner.py; the same tests pass 39/39 keyless on POC-3 main). Suggested fix: resolve the namespace from the env vars alone (the selector already inspects them) and construct the client lazily. Note the client built here is discarded anyway — each extract.run() builds its own.

3. The installed package is named src.
packages = ["src"] with from src.x import y everywhere means the wheel installs a top-level package literally called src — it will collide with any other package built the same way, and the distribution can never be published. Related: src/__main__.py documents python -m mc, but only python -m src works. Suggest renaming the package directory (e.g. src/mc/, or metadata_catalog) while keeping the mc console script. Cheap now; expensive after more code lands on these imports.

4. A fresh clone can't run ingest, and nothing documents the setup.
Comments in arizona.py and indiana.py say the source workbooks live under data/bootstrap/ "so mc ingest az works out of the box" — but data/bootstrap/ is gitignored and not in the repo, so a fresh clone fails with FileNotFoundError. The .gitignore also contradicts itself: line 20 ignores data/bootstrap/ while the comment at line 46 says those files "stay tracked". If the SEA workbooks can't be committed here (a reasonable licensing call), the README needs a setup section saying where to get them and where to place them — right now it has no install or run instructions at all.

Should fix

  • --model is silently ignored when Azure env vars are set. build_runtime_client() drops its model argument whenever an Azure pair is present; with the Sonnet vars set, an explicit request for claude-haiku-4-5 resolves to azure:claude-sonnet-4-6 (verified) — a 3x cost change with no signal. At minimum log which backend and model were selected at run start; better, make the selection explicit rather than env-presence-driven.
  • Batch and sync paths disagree on the cache namespace when Azure is active. run-all --batch submits under the plain Anthropic namespace, but the sync path with Azure envs reads and writes azure:*. Submit → collect → sync re-run finds nothing in cache and pays again on Azure. Refuse --batch when Azure selection is active, or resolve the namespace on both paths.
  • Removed test seams. extract.py and peer_gap.py no longer import AnthropicClient, which the suite monkeypatches (3 of the 19 failures). Keep the import or repoint the tests when they come over.
  • mypy fails on the new file. azure_client.py:103 and :111client takes the type of the first branch (AzureAnthropicClient) and the later assignments don't match. Annotate it client: LLMClient. POC-3's CI enforced mypy green, so this blocks restoring the lint lane.
  • Rename slips in generated report text. reviewer_comparison_summary.py now names data/out/{state}_scores_mc_gap.json — the artifact is {state}_scores_gap.json; the same file has a dropped word ("…no response category See the no_mc_row caveat…"); review_digest.py's docstring says key_server_override where the bucket is key_sever_override.
  • Artifact schema break should be stated. The poc3_*mc_* JSON keys, the no_poc3_rowno_mc_row bucket, and the *_with_mc_scores.xlsx filenames are a clean break from POC-3 artifacts. Fine for a fresh repo, but the PR description (currently empty) should say so, along with the POC-3 base commit and what was deliberately left behind (tests, docs, scripts, infra, bootstrap data).

Minor

mc --help still says "NACHOS POC-3 — ingestion-first pipeline"; 65+ docstring references point at docs/ files that don't exist in this repo; uv.lock was dropped (CI used it); no Apache/SPDX headers on the .py files — maintainers should confirm the repo convention; workbook fingerprint goldens will need regeneration when tests arrive (the rename changed text on four deliverable sheets, and scripts/ didn't come over either).

To save redoing the rename pass, the adapted test port is ready to merge as #12 (stacked on this branch — 137 test files plus tests.yml, provenance and per-failure triage in the commit message). It runs red on exactly the findings above and goes green as they're fixed.

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.

3 participants