Skip to content

fix: reject empty scorer prompt sets - #408

Open
Bortlesboat wants to merge 1 commit into
p-e-w:masterfrom
Bortlesboat:agent/reject-empty-scorer-prompts
Open

fix: reject empty scorer prompt sets#408
Bortlesboat wants to merge 1 commit into
p-e-w:masterfrom
Bortlesboat:agent/reject-empty-scorer-prompts

Conversation

@Bortlesboat

Copy link
Copy Markdown
Contributor

Summary

Empty scorer prompt sets now fail during scorer initialization with a clear error instead of producing a later scoring crash.

KeywordRate previously accepted 0 prompts and could later divide by zero. KLDivergence could proceed to baseline-logit collection with no prompts. Both built-in scorers now reject empty prompt sets immediately after loading them.

Validation

  • PYTHONPATH=src uv run python -m unittest discover -s tests -p test_scorers.py
  • uv run ruff format --check src/heretic/scorers/keyword_rate.py src/heretic/scorers/kl_divergence.py tests/test_scorers.py
  • uv run ruff check --extend-select I src/heretic/scorers/keyword_rate.py src/heretic/scorers/kl_divergence.py tests/test_scorers.py
  • uv run ty check src/heretic/scorers/keyword_rate.py src/heretic/scorers/kl_divergence.py tests/test_scorers.py
  • git diff --check

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request adds validation to ensure that both the KeywordRate and KLDivergence scorers are initialized with at least one prompt, raising a ValueError otherwise. It also introduces corresponding unit tests to verify this behavior. I have no feedback to provide.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

@red40maxxer

Copy link
Copy Markdown
Contributor

This also looks like a reasonable change to me, though I'm not sure if the unit tests are strictly necessary.

@Bortlesboat

Copy link
Copy Markdown
Contributor Author

They're cheap now that CI runs the tests directory, but I don't feel strongly — happy to drop them and keep just the validation if you'd rather keep the suite lean.

@Vinay-Umrethe

Copy link
Copy Markdown
Collaborator

This also looks like a reasonable change to me, though I'm not sure if the unit tests are strictly necessary.

yes I don't think that test is necessary, it did not changed anything affecting reproducibility. And tests are already passing for what cases we have as of now.

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