Manager: fixes from PR #36 review#38
Merged
Merged
Conversation
- Share one load_dotenv (agents/env.py) between main.py and the fine-tune script instead of two hand-rolled copies - Thread sample_size into build_pipeline explicitly; drop the agents.manager._defaults reach-in - Route the finalize pass straight to its node: decide loses both finalize special cases (iteration bump, history dedup) and the redundant LLM rationale call on a decision that cannot change - Rank best-iteration snapshots with report_score (accuracy, collapse-penalized) — one definition shared with the gate's class floor, so select_best can never restore a degenerate pass - Name classifier_history once (HISTORY_DIR), simplify _next_params index mapping, revert mock formatting churn, dedup pipeline test fakes and graph-build boilerplate
- Perturb candidates carry both knobs (a partial dict made _same() falsely skip untried schedule entries) and boost_factor is clamped at _BOOST_MAX - A collapse-forced first retune falls back to the schedule instead of accepting the proceed-proposal's empty suggested_params - clean_outputs runs after Aurora succeeds, so a run that dies on its inputs keeps the previous run's deliverables - retune_loop.md: fix stale fine-tune-vs-baseline claim (contradicted finetune_runs.md); document the zero-support-class collapse limit (needs per-class support from Sabina to fix properly)
The loop tuned thresholds against the same test rows the final report
was measured on — eight rounds of that overfits the test set. Now:
- Aurora: split gains 'val' (last 10% of training dates), same
date-based no-leak logic; test window unchanged.
- Nadi's generated classifier predicts val + test rows, carrying each
row's split value through (was: test only).
- Sabina: eval_split parameter ('val' | 'test') filters what she
scores, hard error when the requested rows are absent; the report
records eval_split. code_notes now flag class collapse (any class
recall < 0.05), and the LLM prompt asks for per-class judgement and
warns against chasing the aggregate number.
- Pipeline: loop evaluations run eval_split='val'; new evaluate_test
node scores the selected best classifier on test exactly once before
explain/finalize.
- Manager: explanation sample and finals filter to test rows (val rows
exist only for the loop's own scoring).
- Contract, mock_data (2 rows now val; report regenerated), and docs
updated together per the working agreement.
Cross-lane note: touches Aurora's and Nadi's modules at Jack's
direction — flagged for their review on the PR.
58 tests pass.
Evaluator: score retunes on a val split; test set touched once
e88f7e3 stripped --dataset-end from main.py/pipeline_graph.py, leaving Aurora's dataset_end param reachable only by calling ProcessingAgent directly. Full-pipeline runs since then silently test on whatever window falls out of the split, including the COVID crash - producing sub-random accuracy that looked like a model failure but was actually an evaluation bug.
Share handoff file rules
Force-added from gitignored outputs/ since these are the artifacts of a completed run worth preserving (decision, reports, predictions, explanations, finetune comparisons). Model weight dirs and the classifier.py run-copy stay untracked.
aouirora
approved these changes
Jul 5, 2026
aouirora
left a comment
aouirora
left a comment
Collaborator
There was a problem hiding this comment.
Good to me! Approving the 3-way split in processing agent (test / train /val)
Collaborator
|
It seems ok to me |
Test-row filtering lives in contracts._test_rows (used by build_final_results + write_explanation_sample). The copy in jack_manager had no callers after the contracts centralization — a stale second definition a future edit could wrongly trust.
Sabina: report per-class support
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Follow-up to #36 (merged before these landed). Two commits: my pre-merge self-review simplification pass, plus fixes for the findings of a full review of #36.
Correctness
select_bestcan no longer restore a class-collapsed pass. Best-iteration snapshots now rank onreport_score— accuracy penalized below any healthy score when a class sits under the recall floor — the same rule the gate uses. Previously a degenerate all-neutral pass (e.g. 0.65 with up-recall 0.0) that the collapse floor refused to ship could still be snapshotted as "best" and restored into the finals.finalizenode instead of re-runningdecideon the (possibly restored) report — which also removes the iteration-bump/history-dedup special cases and a wasted LLM rationale call.suggested_params; the gate previously "accepted"{}and re-ran Nadi's defaults, wasting an iteration and poisoningtried_params._same()falsely skip untried schedule entries.boost_factoris also clamped (_BOOST_MAX) — threshold was clamped, boost grew without bound.clean_outputs()runs after Aurora succeeds, not at run start — a run that dies on its inputs (typo'd--data-dir) no longer wipes the previous run's deliverables first.Cleanup
load_dotenvinagents/env.py(was copy-pasted intofinetune_finbert.py— @nadi, touches your file again, same footprint as before).sample_sizepassed intobuild_pipelineexplicitly; theagents.manager._defaultsprivate reach-in is gone.HISTORY_DIRnamed once;_next_paramsindex mapping simplified; mock formatting churn reverted; pipeline test fakes deduped.docs/retune_loop.mdno longer contradictsdocs/finetune_runs.mdon the fine-tune-vs-baseline verdict.Surfaced, not fixed (Sabina's lane)
class_accuracyis 0.0 for a label with zero test rows — indistinguishable from a collapse, so a window missing a label can never clear the target (documented inretune_loop.md). Needs per-class support inevaluation_report.json.Branches merged into this one
Since this PR was opened, two more branches landed on top of
feature/manager-adapt:fix/eval-val-split— retune loop now evaluates on avalsplit instead of the test split (test is scored exactly once, at the end). Touchesnadi_classifier.py,sabina_evaluator.py,jack_manager.py,finetune_finbert.py.feature/contracts-module— newagents/contracts.pycentralizes the handoff-file column lists/validation that were previously duplicated across agents. Also includes my own--dataset-endCLI fix (restores a flag that had been accidentally reverted, so full-pipeline runs can exclude the COVID window from the test split again).For my teammates (Freddi, Aurora, Sabina, Nadi)
The contracts centralization (#39) touches your agent files directly, not just mine. Please check the diff on your file specifically for behavior changes, don't assume it's purely mechanical:
agents/freddi_explanation.pyagents/aurora_processing.pyagents/sabina_evaluator.pyagents/nadi_classifier.pyI also modified the notebooks below as part of that same change — I updated them to match, but I have not fully re-verified them end to end, so please check your own before trusting it:
docs/explanation_agent_walkthrough.ipynb(Freddi)docs/nadi_classifier_guide.ipynb(Nadi)docs/sabina_evaluator_guide.ipynb(Sabina)Verification
uv run pytest tests/), including new regression tests: collapsed pass never outranks healthy, collapse-forced retune params, full-key perturbs, boost clamp, failed-start keeps previous outputs.