Reorganise the repository around three layout rules

[12yuens2]

Repository organisation only. No behaviour changes to either pipeline — the one code change is path resolution in three analysis scripts that had to move.

Three rules

Rule	Where
All Python under src/	nothing executable at the root any more
All shell runners under scripts/	including the K* batch runners
All generated output under results/	git-ignored, one rule

Reference inputs go in configs/. The notebook goes in notebooks/.

What moved

K_star_analysis/*.py    -> src/analysis/k_star/
K_star_analysis/*.sh    -> scripts/k_star/
overlap-viz.py          -> src/analysis/overlap_viz.py
chosen-agents.py        -> src/analysis/agent_selection_sweep.py
orchestrator_test.py    -> src/analysis/orchestrator_pool_sweep.py
personas.json           -> configs/
Untitled.ipynb          -> notebooks/exploration.ipynb
viz/ csv/ task_plots/ root PNGs, root JSON dumps, overall-performance.pdf
                        -> results/

The three moved analysis scripts read and wrote cwd-relative paths, so they only worked from the root. They now resolve paths from __file__ and can be invoked from anywhere. The K* shell runners derived everything from their own location; they now compute a repo root, point at src/analysis/k_star/, and write to results/k_star/.

orchestrator_test.py is not a test — it is a standalone Azure probe carrying a second, divergent copy of the orchestrator system prompt, and pytest would have collected it by name and executed its module-level API calls. Renamed to say what it does.

The .gitignore fix matters more than it looks

The old rules matched *.csv, *.json and *.jsonl across the whole repository. HuggingFace save_to_disk writes dataset_info.json and state.json beside the arrow file, so:

data/tagged_dataset/ — the input every orchestrator run reads — could not be committed, and existed on one machine only.

The same patterns also hid configs/personas.json and .vscode/settings.json from git entirely. Rules are now scoped to the directories that actually hold output (out/, out-baseline/, results/, **/history/), and the tagged dataset is committed in this PR (699 questions, 370 KB).

Also in the ignore file: .DS_STORE corrected to .DS_Store, and env/ added explicitly — until now the 1.3 GB venv escaped git only because venv writes its own .gitignore inside itself.

Other fixes

• .vscode/settings.json pointed python.analysis.extraPaths at ./src/orchestation — missing the r. Editor import resolution had been silently broken. Now ./src.
• README gains a top section: what the repo holds, setup, and the three-stage orchestration pipeline (tag → build dataset → run loop), which was previously only discoverable by reading the source. The stale structure tree and K_star_analysis/ invocation paths are updated.
• The README's orchestrator section carries a status note that stage 3 is not yet ready for experiments.

Verified

• Core imports resolve from the repo root; --help works on main.py, tag_questions.py, train_orchestrator.py.
• src/analysis/overlap_viz.py runs end to end and writes into results/overlap/{csv,viz,png} — nothing leaks back to the root.
• data/tagged_dataset loads: 699 rows.
• git check-ignore confirms the dataset and configs/personas.json are no longer ignored, while all output directories still are.

Left alone deliberately

• out/ and out-baseline/ hold 182 MB of live debate-pipeline results and remain the --out_dir default, so they keep their location. results/ is the convention going forward.
• .ipynb_checkpoints/ under src/ contains autosaves that differ from the live files, including an 896-line model_utils_copy-checkpoint.py with no live counterpart. Not mine to delete — worth a look before you remove them.
• notebooks/exploration.ipynb is committed with its outputs (812 KB). Easy to drop from the commit if you would rather it stayed untracked.

Commits

1. Checkpoint in-progress orchestrator work — snapshots the 7 modified files that were uncommitted, so the reorganisation reads as a move-only diff rather than being tangled with unrelated source edits. No behaviour reviewed or corrected.
2. Reorganise the repository around three layout rules — this PR.

🤖 Generated with Claude Code

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[12yuens2]

why are there two different max_new_tokens numbers here? they should be the same and the default should be put as a global constant to remove the magic number

[12yuens2]

Fixed. The two were 2048 (when a persona config was present) and 512 (when not), so an agent's token budget depended on whether it had a persona at all. Both now come from MAX_NEW_TOKENS in the new src/defaults.py, which matches the --max_new_tokens argparse default get_agents was already using.

[12yuens2]

magic numbers here too on the temperature and top_p

[12yuens2]

Fixed — TEMPERATURE and TOP_P in src/defaults.py. Worth noting these had drifted too: engine fell back to 1.0, but get_agents used 0 for the same parameter, so the effective temperature depended on which code path set it. Both now use the constant.

Per-persona temperature/top_p values inside the persona definitions are left as literals — those are persona data rather than defaults.

[12yuens2]

more magic numbers, should do a pass, if possible it should be added as an argument in the parseargs with a default

[12yuens2]

Done, and promoted to a flag. thinking_token_budget was hard-coded in three places with two different values (1024 in the agent engine, 2048 in the orchestrator and summariser). It is now --thinking_token_budget on main.py, tag_questions.py and train_orchestrator.py, reaching agents as an attribute set by get_agents and the orchestrator via its constructor. Precedence follows the other generation params: persona config → agent attribute → default.

[12yuens2]

more magic numbers to clean up

[12yuens2]

Fixed — SUMMARISER_MAX_TOKENS, SUMMARISER_TEMPERATURE and THINKING_TOKEN_BUDGET in src/defaults.py.

[12yuens2]

let's just delete this fallback printing and do a retry on the summariser (up to a number of times before actually failing)

[12yuens2]

Done — retries up to SUMMARISER_MAX_ATTEMPTS (3) and raises if all fail.

Worth flagging what the fallback was actually doing, because it was worse than dead code. md_content was stringified before the is None guard, so str(None) produced the four-character string "None", the guard could never fire, and the atomic write then replaced the scoreboard with it. A single transient API failure wiped the accumulated per-tag performance history — the file the orchestrator reads back as its only memory. On failure the previous version is now left untouched, and the temp file is unlinked rather than leaked. The unconditional dump of the whole prompt on every iteration is gone too.

[12yuens2]

print this but only on a debug flag being active

[12yuens2]

Done — real print behind args.debug, one agent per block rather than a single dict repr.

[12yuens2]

is this used for anything? if there's parts of the code that checks for personas based on dataset they should refer to this instead of hard-coded

[12yuens2]

Yes — src/analysis/overlap_viz.py reads it as the canonical persona set per task, to measure how far the LLM-selected teams overlap with it.

You are right that the mapping is duplicated: _build_enhanced_personas in model_utils.py branches on args.data and hard-codes the same dataset → persona grouping. I checked whether this file can just become the source of truth, and it is close but not quite — three things block it, and each one changes experimental results, so I have not made the swap in this PR.

1. Six of seven datasets match exactly. gsm8k, pro_medicine, formal_logic, humaneval, arc, winogrande — identical name sets.

2. truthfulqa lists 10 names here, but the code only ever uses 5. The extra five are the second elif args.data in ['truthfulqa'] branch at model_utils.py:1195, which is unreachable — the branch at :1156 catches it first. So this file records an intent the code has never executed. Which set of five is the one the published TruthfulQA numbers used?

3. piqa is missing from this file but has a persona set in the code.

4. Elimination_Specialist is a genuine name collision. It is defined twice with different content — the arc version (style: elimination_science, top_p: 0.84) and the winogrande version (style: elimination, top_p: 0.9, a longer prompt). The flat bank in _build_chosen_personas keeps only one of them, so --chosen_personas Elimination_Specialist on winogrande silently gets the arc persona today. Unifying means renaming one, which changes the persona name recorded in every output file.

Everything else in the two persona banks is identical — I diffed all 44 definitions field by field and that collision is the only difference.

So the unification is worth doing and mostly mechanical, but it needs your call on (2) and (4). Happy to take it as the next PR once you say which TruthfulQA set is correct and what the two Elimination_Specialist personas should be called.

[12yuens2]

All three resolved, and configs/personas.json now matches the code exactly for all seven datasets (verified name-by-name).

TruthfulQA → 10. The two elif args.data in ['truthfulqa'] branches are merged into one set. Their union was already exactly the 10 you had listed here, so the file was right and the code was short by five — those five had never executed, since the first branch always caught it.

Elimination_Specialist renamed. Based on the prompts: the ARC one is scientific option elimination ("find one clear flaw in each wrong option… prefer the one consistent with general scientific consensus"), the WinoGrande one is pronoun resolution ("for each candidate, list what must be true for it to be the pronoun referent"). It turned out the flat bank in _build_chosen_personas had already resolved this by calling the WinoGrande one Elimination_Based_Solver — byte-identical prompt — so I made _build_enhanced_personas match rather than invent a third name. ARC keeps Elimination_Specialist. This also fixes the silent substitution: --chosen_personas Elimination_Specialist on WinoGrande was getting the ARC persona.

PIQA is Physical Interaction QA — a physical-commonsense benchmark (which of two ways of achieving a goal actually works), the sort of thing the five personas there describe: simulating action sequences, mechanical stability, material interaction. It was never wired up, though: no src/data/piqa.py, no branch in data_utils.load_data, no case in main.py's evaluator dispatch. --data piqa could not run, so those personas were unreachable. I removed the branch. Nothing is lost — all five are still in the flat bank and reachable by name through --chosen_personas, and they are in git history if you ever add the loader.

With the file and the code now in agreement, the structural unification (deriving the per-dataset names from this file instead of duplicating them) is unblocked. One thing to know before doing it: only 5 of 45 personas in the flat bank carry nvidia_persona, and _add_nvidia_personas indexes that key unconditionally — so --persona_prompt currently only works for GSM8K and raises KeyError on every other dataset. That wants fixing as part of the same change.

[12yuens2]

i think this fits in the src dir, we want to have all our experimental scripts in one place at the top level, and analysis can contain the scripts for looking at data/visualisation

[12yuens2]

Moved. src/analysis/ now holds only what reads results and draws plots (k_star/, overlap_viz.py); both sweeps moved up to the top of src/.

Both also turned out to execute on import: importing agent_selection_sweep spawned its entire job queue through ThreadPoolExecutor (I hit this while verifying the move — the subprocesses died immediately because python on PATH has no torch, but it did fire), and orchestrator_pool_sweep built an OpenAI client at module scope, so it raised on import without credentials and --help did not work. Both are now behind a main guard with the client built after argument parsing.

orchestrator_pool_sweep was also writing its chosen-agents JSON to the working directory; it now writes to results/agent-selection/.

[12yuens2]

we should put settings like temperature and max tokens here as arguments if relevant and same in any argparse.

[12yuens2]

Added --temperature, --top_p, --max_completion_tokens and --repeats, and passed all four to the call. The temperature and top_p were not being set at all before, so the sweep was running at whatever the endpoint defaults to.

Same pass applied to agent_selection_sweep.py, which was pinning --max_new_tokens 1024, --num_agents 4, --data_size 100, --solver vote, --debate_rounds 0 and max_workers=10 directly into each main.py command line — all flags now. It also launched "python" rather than sys.executable, which is why its subprocesses died silently for me earlier; fixed.

[12yuens2]

magic number, put in arguments

[12yuens2]

Done — --max_completion_tokens, default 1024 from POOL_SWEEP_MAX_COMPLETION_TOKENS.

[12yuens2]

this 10 should also be an argument

[12yuens2]

Done — --repeats, default 10 from POOL_SWEEP_REPEATS.

[12yuens2]

we should be use args instead of global defaults, the global defaults should be the default if no arg is passed in the argparse

[12yuens2]

Understood, and applied throughout — src/defaults.py now exists only to supply argparse defaults, and its docstring says so.

save_evaluation_summary_with_llm takes max_tokens, temperature, max_attempts and thinking_token_budget as parameters (falling back to the orchestrator's own settings when not passed), and train_orchestrator supplies them from --summariser_max_tokens, --summariser_temperature, --summariser_max_attempts and --thinking_token_budget. OrchestratorAgent likewise takes temperature and top_p alongside max_tokens. Neither module imports a constant any more.

[12yuens2]

like this, this is good, do this instead of directly using the defaults constants in the code

[12yuens2]

Applied as the rule everywhere. train_orchestrator now has three groups of generation flags — the agents in the selected team, the selection call, and the scoreboard rewrite — plus --iterations to replace the hard-coded 10.

The agent-facing group closes a real gap rather than just tidying: team_evaluation builds its namespace by deep-copying these args, and it had no max_new_tokens/temperature/top_p on it, so the selected team ran on whatever get_agents fell back to no matter what you configured.

I also added argparse to tag_dataset.py and analysis/overlap_viz.py, which had none at all — input paths, the tag-frequency threshold and the model/description selectors were literals in the module body.

[sylvesterkaczmarek]

scripts/add.sh looks non-runnable after this change. GPU_LIST, GPU_COUNT, MAX_JOBS_PER_GPU, MAX_PARALLEL_JOBS, and the local GPU_ID / PHYS_GPU_ID assignments are commented out, but the scheduler and run_experiment() still dereference them.

With set -u, the script should fail on these unset variables before the experiments can run. Could the GPU definitions and local assignments be restored, or the remaining GPU scheduling / CUDA_VISIBLE_DEVICES logic be removed consistently?

Since this PR is described as an organisation-only change, this currently introduces a functional regression in the batch runner.