refactor: move the SI rubric bundle into data/SI/ - #180
Conversation
f319a85 to
4c87d9c
Compare
a991b0c to
de29200
Compare
4c87d9c to
ae08314
Compare
de29200 to
777bb38
Compare
Everything currently in data/ (rubric.tsv, rubric_prompt_beginning.txt, question_prompt.txt, rubric_manifest.json, personas.tsv) is specific to the SI rubric -- once a second rubric type (e.g. PHQ9) exists, data/ needs a subfolder per rubric rather than one flat, implicitly-SI set of files. Move all five into data/SI/ and update every hardcoded default/reference across generate.py, judge.py, run_pipeline.py, judge/score.py, judge/score_utils.py, scripts/, README.md, docs, and tests.
--rubrics/--rubric-manifest require a full manifest path; there's no way to select a rubric by bare name (e.g. typing "SI" anywhere on the command line) until --target lands on the future vera.py CLI. Also documents generate.py --rubric-manifest, which wasn't in the README at all.
777bb38 to
1e7c70c
Compare
There was a problem hiding this comment.
Pull request overview
Refactors the repository’s “production” rubric bundle layout by moving the Suicide Ideation (SI) rubric assets under data/SI/, updating code/tests/docs so defaults and references point at the new rubric-bundle location. This supports adding additional rubric types (e.g., PHQ9) without overloading a single flat data/ directory.
Changes:
- Moves SI bundle files into
data/SI/(rubric TSV, prompts, manifest, personas) and updates defaults/paths across generation, judging, scoring, and scripts. - Updates CLI defaults and test expectations to use
data/SI/rubric_manifest.jsonanddata/SI/personas.tsv. - Updates documentation to reflect the new bundle layout and usage.
Reviewed changes
Copilot reviewed 22 out of 28 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
tests/unit/judge/test_score.py |
Updates prod personas TSV path to data/SI/personas.tsv for risk-level ordering test. |
tests/unit/judge/test_rubric_config.py |
Updates prod rubric TSV path to data/SI/rubric.tsv for column/dimension validation tests. |
tests/unit/judge/test_llm_judge.py |
Updates production rubric TSV path and related assertions/messages to data/SI/rubric.tsv. |
tests/unit/judge/test_judge_cli.py |
Updates judge.py default --rubrics manifest path to data/SI/rubric_manifest.json in unit tests. |
tests/unit/generate_conversations/test_generate_cli.py |
Updates generation defaults to data/SI/personas.tsv and data/SI/persona_context_template.txt. |
tests/test_question_navigator.py |
Updates RubricConfig.load(... rubric_folder=...) to data/SI. |
tests/integration/test_scoring.py |
Updates integration test environment validation to require data/SI/personas.tsv. |
tests/integration/test_pipeline.py |
Updates pipeline default rubric manifest/personas paths in integration tests. |
tests/integration/test_llm_judge_not_relevant_flow.py |
Updates rubric folder to data/SI for integration flow. |
tests/integration/test_conversation_runner.py |
Updates docstring reference for default personas path to data/SI/personas.tsv. |
scripts/summarize_results.py |
Updates default --rubric path and usage examples to data/SI/rubric.tsv. |
scripts/pool_vera_scores.py |
Updates CLI default --personas-tsv and docstrings to data/SI/personas.tsv. |
run_pipeline.py |
Updates pipeline CLI defaults/help for --rubrics manifest and --personas-tsv to the data/SI/ bundle. |
README.md |
Updates documentation references/links/examples for personas/rubric paths under data/SI/. |
judge/score.py |
Updates scoring CLI default personas path to data/SI/personas.tsv. |
judge/score_utils.py |
Updates rubric structure load path to data/SI/rubric.tsv. |
judge/llm_judge.py |
Updates inline comment reference to rubric TSV path under data/SI/. |
judge.py |
Updates --rubrics default manifest path to data/SI/rubric_manifest.json. |
generate.py |
Updates generation defaults/help text for personas + context template paths to data/SI/. |
generate_conversations/utils.py |
Updates default prompt_path to data/SI/personas.tsv. |
generate_conversations/runner.py |
Updates ConversationRunner default persona paths to data/SI/. |
docs/judge.md |
Updates documentation reference to rubric TSV path under data/SI/. |
data/SI/rubric.tsv |
Adds the SI rubric TSV in its new bundle location. |
data/SI/rubric_prompt_beginning.txt |
Adds rubric prompt preamble in its new bundle location. |
data/SI/question_prompt.txt |
Adds question prompt template in its new bundle location. |
data/SI/rubric_manifest.json |
Adds bundle manifest in its new bundle location (manifest-relative references). |
data/SI/personas.tsv |
Adds SI personas TSV in its new bundle location. |
data/SI/persona_context_template.txt |
Adds SI-specific persona context template in its new bundle location. |
Suppressed comments (1)
tests/integration/test_pipeline.py:548
--rubricsis documented/typed as a list of rubric bundle manifest paths (and defaults are nowdata/SI/rubric_manifest.json), but this test still uses legacy TSV paths (data/rubric.tsv). That makes the test inconsistent with the CLI contract and can mask real regressions if later code validates file existence or manifest structure.
def test_rubrics_passed_to_judge(self, pipeline_args):
"""Test that rubrics are correctly passed to judge args."""
# Set custom rubrics
pipeline_args.rubrics = ["data/rubric.tsv", "data/custom_rubric.tsv"]
# As done in main(): judge receives these rubrics
judge_args = argparse.Namespace(
rubrics=pipeline_args.rubrics,
)
assert judge_args.rubrics == ["data/rubric.tsv", "data/custom_rubric.tsv"]
assert len(judge_args.rubrics) == 2
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Load dimensions from rubric file | ||
| DIMENSIONS, _ = load_rubric_structure("data/rubric.tsv") | ||
| DIMENSIONS, _ = load_rubric_structure("data/SI/rubric.tsv") |
| personas_tsv: Personas file for risk-level analysis; defaults to | ||
| ``data/personas.tsv`` under the repo when None. | ||
| ``data/SI/personas.tsv`` under the repo when None. | ||
| skip_risk_analysis: When True, skip ``score_results_by_risk`` and risk charts. |
|
I think github copilot had a few good points we should address before merging... |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…allback judge/score_utils.py loaded the rubric at import time from a cwd-relative path, so importing the module from outside the repo root raised FileNotFoundError. Resolve it from __file__ instead. scripts/pool_vera_scores.py's programmatic fallback still pointed at the pre-move data/personas.tsv. Since the call site guards on is_file(), risk level analysis silently skipped for callers that did not pass personas_tsv. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Thank you, should be addressed now |
Summary
Stacked on #178 (base branch is that PR's branch, not
main).Everything currently in
data/(rubric.tsv,rubric_prompt_beginning.txt,question_prompt.txt,rubric_manifest.json,personas.tsv) is specific to the SI rubric. Once a second rubric type (e.g. PHQ9) exists,data/needs a subfolder per rubric rather than one flat, implicitly-SI set of files.data/SI/generate.py,judge.py,run_pipeline.py,judge/score.py,judge/score_utils.py,scripts/pool_vera_scores.py,scripts/summarize_results.py,README.md,docs/judge.md, and testsdata/SI/rubric_manifest.json'spersonasentry ("personas.tsv") still resolves correctly since it's manifest-relative (per feat: add RubricConfig.load_bundle() for rubric bundle manifests #177's fix) and the manifest andpersonas.tsvremain siblings after the moveTest plan
uv run pytest -m "not live"passes (976 passed)uv run ruff check/uv run ruff format --checkclean (same pre-existing, unrelated drift as base -- nothing new)uv run pyrightclean (same pre-existing, unrelated error count as base -- nothing new)