-
Notifications
You must be signed in to change notification settings - Fork 31
Add metric tuning for custom metrics [feature branch] #2446
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
akwasigroch
wants to merge
9
commits into
main
Choose a base branch
from
feat/metric-tuning-test-sets
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
cc106ab
feat(backend): add metric_id to test_set and test
akwasigroch 97fd96e
feat(backend): add metric tuning cases
akwasigroch 5381556
feat(frontend): add experimental metric tuning tab
akwasigroch de25f97
style(backend): format metric tuning router
akwasigroch db633e7
feat(backend): store the whole case in the prompt content
akwasigroch 983938f
feat(backend): make the tuning case verdict optional
akwasigroch a9d17da
feat(frontend): mark tuning cases with no verdict yet
akwasigroch 528d5e9
docs(backend): drop the reserved metric seat claim
akwasigroch 1f7d939
Add tuning runs for custom metrics (#2470)
akwasigroch File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
90 changes: 90 additions & 0 deletions
90
...nd/src/rhesis/backend/alembic/versions/c7e2f4a91b83_add_metric_id_to_test_set_and_test.py
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,90 @@ | ||
| """Add metric_id to test_set and test | ||
|
|
||
| Marks a test set (and its tests) as owned by a single metric, for metric tuning: | ||
| the metric's own golden test set, hidden from the normal /test_sets and /tests | ||
| lists the same way explorer_row hides Explorer's rows. | ||
|
|
||
| Nullable with no server default, so this adds no DML at all -- unlike | ||
| 7dd69fe35db5 (explorer_row), which needed the FORCE ROW LEVEL SECURITY | ||
| disable/restore dance purely so its backfill UPDATE could see rows. Nothing to | ||
| backfill here means nothing to work around. | ||
|
|
||
| test is a hot table, so the foreign keys go on as NOT VALID first and are | ||
| validated in a second statement: ADD CONSTRAINT ... NOT VALID takes a brief | ||
| ACCESS EXCLUSIVE lock without scanning, and VALIDATE CONSTRAINT then scans under | ||
| a weaker SHARE UPDATE EXCLUSIVE lock that does not block reads or writes. All | ||
| existing rows have metric_id IS NULL, so validation cannot fail. | ||
|
|
||
| Revision ID: c7e2f4a91b83 | ||
| Revises: 82881df987af | ||
| Create Date: 2026-08-06 | ||
|
|
||
| """ | ||
|
|
||
| from typing import Sequence, Union | ||
|
|
||
| import sqlalchemy as sa | ||
| from alembic import op | ||
|
|
||
| from rhesis.backend.alembic.utils.idempotency import column_exists, fk_exists, index_exists | ||
| from rhesis.backend.app.models.guid import GUID | ||
|
|
||
| # revision identifiers, used by Alembic. | ||
| revision: str = "c7e2f4a91b83" | ||
| down_revision: Union[str, None] = "82881df987af" | ||
| branch_labels: Union[str, Sequence[str], None] = None | ||
| depends_on: Union[str, Sequence[str], None] = None | ||
|
|
||
|
|
||
| # (table, index name, fk constraint name) | ||
| # (table, index name, fk constraint name, one row per metric) | ||
| # test_set is unique: a metric owns at most one tuning test set, and without the | ||
| # constraint two concurrent first POSTs each create one, after which half the | ||
| # cases live in a set nothing reads. test is not: a metric owns many cases. | ||
| _TARGETS = ( | ||
| ("test_set", "ix_test_set_metric_id", "fk_test_set_metric_id", True), | ||
| ("test", "ix_test_metric_id", "fk_test_metric_id", False), | ||
| ) | ||
|
|
||
|
|
||
| def upgrade() -> None: | ||
| conn = op.get_bind() | ||
|
|
||
| for table, index_name, fk_name, unique in _TARGETS: | ||
| if not column_exists(conn, table, "metric_id"): | ||
| op.add_column( | ||
| table, | ||
| sa.Column("metric_id", GUID(), nullable=True), | ||
| ) | ||
|
|
||
| if not index_exists(conn, index_name): | ||
| # Partial: rows with no metric_id are the overwhelming majority and | ||
| # must not be forced unique against each other. | ||
| op.create_index( | ||
| index_name, | ||
| table, | ||
| ["metric_id"], | ||
| unique=unique, | ||
| postgresql_where=sa.text("metric_id IS NOT NULL"), | ||
| ) | ||
|
|
||
| if not fk_exists(conn, fk_name, table): | ||
| op.execute( | ||
| f"ALTER TABLE {table} ADD CONSTRAINT {fk_name} " | ||
| f"FOREIGN KEY (metric_id) REFERENCES metric (id) NOT VALID" | ||
| ) | ||
| op.execute(f"ALTER TABLE {table} VALIDATE CONSTRAINT {fk_name}") | ||
|
|
||
|
|
||
| def downgrade() -> None: | ||
| conn = op.get_bind() | ||
|
|
||
| for table, index_name, fk_name, _unique in _TARGETS: | ||
| if fk_exists(conn, fk_name, table): | ||
| op.drop_constraint(fk_name, table, type_="foreignkey") | ||
|
|
||
| if index_exists(conn, index_name): | ||
| op.drop_index(index_name, table_name=table) | ||
|
|
||
| if column_exists(conn, table, "metric_id"): | ||
| op.drop_column(table, "metric_id") |
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
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
251 changes: 251 additions & 0 deletions
251
apps/backend/src/rhesis/backend/app/crud/metric_tuning.py
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,251 @@ | ||
| """CRUD operations for metric tuning. | ||
|
|
||
| Part of the incremental split of the ``crud`` monolith: per-entity modules like | ||
| this one take over as the code around them is touched, and nothing new is added | ||
| to ``crud/__init__.py``. | ||
|
|
||
| Every function here flushes and never commits -- the request session owns the | ||
| commit (see ``get_db_with_tenant_variables`` in ``database.py``). | ||
|
|
||
| Tuning cases are written as a ``Prompt`` + ``Test`` pair directly, the way | ||
| ``crud/explorer.py::create_explorer_test`` does, rather than through | ||
| ``services/test.py::bulk_create_tests``. That service requires a behavior, a | ||
| category and a topic and ``get_or_create``s each one, which would file rows like | ||
| "Metric Tuning" into the organization's real taxonomy. | ||
| """ | ||
|
|
||
| import logging | ||
| import uuid | ||
| from typing import List, Optional | ||
|
|
||
| from sqlalchemy.orm import Session, joinedload | ||
|
|
||
| from rhesis.backend.app import models | ||
| from rhesis.backend.app.models.test import test_test_set_association | ||
| from rhesis.backend.app.schemas.metric_tuning_metadata import ( | ||
| MetricTuningCaseMetadata, | ||
| MetricTuningCaseResult, | ||
| MetricTuningRunSummary, | ||
| parse_metric_tuning_case_metadata, | ||
| parse_metric_tuning_run_summary, | ||
| ) | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| # Where the latest run's summary lives inside ``TestSet.attributes``. Nothing | ||
| # else writes a tuning set's attributes -- attribute regeneration early-returns | ||
| # for metric-owned sets -- but the key keeps it out of the way regardless. | ||
| RUN_SUMMARY_KEY = "tuning_run" | ||
|
|
||
|
|
||
| # --- Test sets --------------------------------------------------------------------- | ||
|
|
||
|
|
||
| def get_tuning_test_set( | ||
| db: Session, metric_id: uuid.UUID, organization_id: str | ||
| ) -> Optional[models.TestSet]: | ||
| """The test set owned by this metric, or None if it has no tuning set yet. | ||
|
|
||
| A metric owns at most one tuning set -- ``services/metric_tuning/test_sets.py`` | ||
| is the only writer of ``TestSet.metric_id`` and creates it once, lazily. | ||
| """ | ||
| return ( | ||
| db.query(models.TestSet) | ||
| .filter( | ||
| models.TestSet.metric_id == metric_id, | ||
| models.TestSet.organization_id == organization_id, | ||
| ) | ||
| .first() | ||
| ) | ||
|
|
||
|
|
||
| def mark_test_set_as_tuning( | ||
| db: Session, test_set: models.TestSet, metric_id: uuid.UUID | ||
| ) -> models.TestSet: | ||
| """Flag a test set as owned by ``metric_id`` via the ``metric_id`` column. | ||
|
|
||
| Kept off ``TestSetCreate`` deliberately: a client-settable ``metric_id`` | ||
| would let anyone hide a test set from the list, so the column is written | ||
| server-side only (mirrors ``mark_test_set_as_explorer``). | ||
| """ | ||
| test_set.metric_id = metric_id | ||
| db.flush() | ||
| db.refresh(test_set) | ||
| return test_set | ||
|
|
||
|
|
||
| def get_run_summary(test_set: models.TestSet) -> MetricTuningRunSummary: | ||
| """The latest run's summary. A set nobody has run reads as ``never_run``.""" | ||
| attributes = test_set.attributes or {} | ||
| return parse_metric_tuning_run_summary(attributes.get(RUN_SUMMARY_KEY)) | ||
|
|
||
|
|
||
| def set_run_summary( | ||
| db: Session, test_set: models.TestSet, summary: MetricTuningRunSummary | ||
| ) -> models.TestSet: | ||
| """Overwrite the latest run's summary. Only the latest is ever kept. | ||
|
|
||
| The whole ``attributes`` dict is reassigned rather than mutated in place: | ||
| SQLAlchemy does not track mutation inside a plain JSONB column, so an | ||
| in-place edit would flush nothing. | ||
| """ | ||
| attributes = dict(test_set.attributes or {}) | ||
| attributes[RUN_SUMMARY_KEY] = summary.model_dump(mode="json", exclude_none=True) | ||
| test_set.attributes = attributes | ||
| db.flush() | ||
| return test_set | ||
|
|
||
|
|
||
| # --- Tuning cases -------------------------------------------------------------------- | ||
|
|
||
|
|
||
| def get_tuning_cases( | ||
| db: Session, test_set_id: uuid.UUID, organization_id: str | ||
| ) -> List[models.Test]: | ||
| """Every tuning case in a tuning test set, oldest first. | ||
|
|
||
| The prompt is eager-loaded because callers always serialize it -- it holds | ||
| both the input and the human's expected verdict. | ||
| """ | ||
| return ( | ||
| db.query(models.Test) | ||
| .options(joinedload(models.Test.prompt)) | ||
| .join( | ||
| test_test_set_association, | ||
| models.Test.id == test_test_set_association.c.test_id, | ||
| ) | ||
| .filter( | ||
| test_test_set_association.c.test_set_id == test_set_id, | ||
| models.Test.organization_id == organization_id, | ||
| ) | ||
| .order_by(models.Test.created_at.asc()) | ||
| .all() | ||
| ) | ||
|
|
||
|
|
||
| def get_tuning_case( | ||
| db: Session, test_set_id: uuid.UUID, test_id: uuid.UUID, organization_id: str | ||
| ) -> Optional[models.Test]: | ||
| """Load one tuning case, but only if it belongs to the given tuning set. | ||
|
|
||
| The membership join is the point: it doubles as the authorization check for | ||
| the per-case endpoints, so a case id from another metric's set 404s instead | ||
| of being edited across metrics. Same approach as | ||
| ``crud/explorer.py::get_test_in_test_set``. | ||
| """ | ||
| return ( | ||
| db.query(models.Test) | ||
| .options(joinedload(models.Test.prompt)) | ||
| .join( | ||
| test_test_set_association, | ||
| models.Test.id == test_test_set_association.c.test_id, | ||
| ) | ||
| .filter( | ||
| models.Test.id == test_id, | ||
| test_test_set_association.c.test_set_id == test_set_id, | ||
| models.Test.organization_id == organization_id, | ||
| ) | ||
| .first() | ||
| ) | ||
|
|
||
|
|
||
| def create_tuning_case( | ||
| db: Session, | ||
| *, | ||
| organization_id: str, | ||
| user_id: str, | ||
| metric_id: uuid.UUID, | ||
| content: str, | ||
| expected: Optional[str], | ||
| metadata: MetricTuningCaseMetadata, | ||
| ) -> models.Test: | ||
| """Insert a tuning case: the prompt holding the payload + verdict, then the test. | ||
|
|
||
| ``content`` is the serialized case payload -- what the metric is shown -- | ||
| and ``expected`` is the verdict it should return, which is why they sit in | ||
| the prompt's two natural slots (ADR-0002, ADR-0003). | ||
|
|
||
| Associating the test with its test set is the caller's job -- that goes | ||
| through the shared ``create_test_set_associations`` service. | ||
| """ | ||
| db_prompt = models.Prompt( | ||
| content=content, | ||
| expected_response=expected, | ||
| organization_id=organization_id, | ||
| user_id=user_id, | ||
| ) | ||
| db.add(db_prompt) | ||
| db.flush() | ||
|
|
||
| db_test = models.Test( | ||
| prompt_id=db_prompt.id, | ||
| test_metadata=metadata.model_dump(mode="json", exclude_none=True), | ||
| organization_id=organization_id, | ||
| user_id=user_id, | ||
| metric_id=metric_id, | ||
| ) | ||
| db.add(db_test) | ||
| db.flush() | ||
| db.refresh(db_test) | ||
| return db_test | ||
|
|
||
|
|
||
| def remove_case_from_test_set(db: Session, test_set_id: uuid.UUID, test_id: uuid.UUID) -> None: | ||
| """Drop the association row linking a tuning case to its test set. | ||
|
|
||
| Detaches only -- soft-deleting the test itself is a separate | ||
| ``crud.delete_test`` call, and the order matters: ``delete_test`` reads the | ||
| association table to decide which test sets to recalculate, so a case | ||
| detached first is deliberately left out of that. | ||
| """ | ||
| db.execute( | ||
| test_test_set_association.delete().where( | ||
| test_test_set_association.c.test_id == test_id, | ||
| test_test_set_association.c.test_set_id == test_set_id, | ||
| ) | ||
| ) | ||
| db.flush() | ||
|
|
||
|
|
||
| def set_case_result( | ||
| db: Session, db_test: models.Test, result: MetricTuningCaseResult | ||
| ) -> models.Test: | ||
| """Record what the metric said about this case, overwriting the last run's. | ||
|
|
||
| Writes only the ``result`` key -- the case itself (its payload, its expected | ||
| verdict, its rationale) is what is being scored and is never touched by | ||
| scoring it. | ||
| """ | ||
| metadata = parse_metric_tuning_case_metadata(db_test.test_metadata) | ||
| metadata.result = result | ||
| db_test.test_metadata = metadata.model_dump(mode="json", exclude_none=True) | ||
| db.flush() | ||
| return db_test | ||
|
|
||
|
|
||
| def update_tuning_case( | ||
| db: Session, | ||
| db_test: models.Test, | ||
| *, | ||
| content: Optional[str] = None, | ||
| expected: Optional[str] = None, | ||
| metadata: Optional[MetricTuningCaseMetadata] = None, | ||
| ) -> models.Test: | ||
| """Apply a partial update to a tuning case. | ||
|
|
||
| Only non-None arguments are written, so a PUT that omits a field leaves it | ||
| alone. Callers that want to clear ``expected`` pass an empty string. | ||
| """ | ||
| if db_test.prompt is not None: | ||
| if content is not None: | ||
| db_test.prompt.content = content | ||
| if expected is not None: | ||
| # NULL, not "", so an unlabelled case looks the same however it got there. | ||
| db_test.prompt.expected_response = expected or None | ||
|
|
||
| if metadata is not None: | ||
| db_test.test_metadata = metadata.model_dump(mode="json", exclude_none=True) | ||
|
|
||
| db.flush() | ||
| db.refresh(db_test) | ||
| return db_test |
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
crud.get_test_sets()now filters out metric-owned tuning sets, but/test_sets'X-Total-Countheader is still computed with onlyexclude_explorer_rows=True.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still looks like
GET /test_setsβsX-Total-Countwill include metric-owned tuning sets:crud.get_test_sets()now filters them out, andwith_count_headernow supportscustom_filters, but@with_count_header(model=models.TestSet, exclude_explorer_rows=True)inrouters/test_set.pyhasnβt been updated to passcustom_filters=[exclude_metric_owned(models.TestSet)].There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still seems outstanding:
crud.get_test_sets()now filters out metric-owned sets, butGET /test_setsis still decorated with@with_count_header(model=models.TestSet, exclude_explorer_rows=True)and doesnβt passcustom_filters=[exclude_metric_owned(models.TestSet)].So
X-Total-Countwill likely include metric-owned tuning sets and disagree with the returned rows.