Skip to content

fix(evals): persist eval name on edit so renames actually stick - #1850

Open
onatozmenn wants to merge 2 commits into
future-agi:devfrom
onatozmenn:fix/eval-rename
Open

fix(evals): persist eval name on edit so renames actually stick#1850
onatozmenn wants to merge 2 commits into
future-agi:devfrom
onatozmenn:fix/eval-rename

Conversation

@onatozmenn

Copy link
Copy Markdown

Summary

An eval's name could not be changed once it was added to a dataset. EditAndRunUserEvalView accepts name in its request serializer and the frontend sends it on every edit, but the view never assigned it, so the request came back 200 OK while the name stayed the same. The Name field in the eval picker was also hard-disabled in edit mode. The only workaround was to delete the eval and add it again, which re-ran it over every row and threw away the existing results.

This makes the rename actually work, end to end.

Linked issues

Closes #1769
Linear:

Type of change

  • 🐛 Bug fix (non-breaking change which fixes an issue)
  • ✨ New feature (non-breaking change which adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • 📖 Documentation only
  • 🧹 Chore / refactor (no user-visible change)
  • 🚀 Performance improvement
  • 🧪 Test-only change

1) What changes were done

A. Backend: persist the name on edit

  • EditAndRunUserEvalView.post now assigns name to the UserEvalMetric when one is sent and it differs from the current name.
  • The new name goes through validate_eval_name, the same helper UserEvalSerializer.validate_name uses when an eval is created, so the rules do not differ between create and rename.
  • Uniqueness is checked with the same filter AddUserEvalView uses (name + organization + dataset, deleted=False), excluding the eval being edited, and returns the existing EVAL_NAME_EXISTS error on a collision.
  • A blank or unchanged name is a no-op rather than an error, since the drawer sends name on every save.
  • The save_as_template branch is untouched. There name is the name of the new EvalTemplate, not the instance.

B. Backend: keep the grid columns in sync

  • New _rename_eval_columns helper renames the eval column and its reason column, which both snapshot the eval name when they are created (see the Column.objects.create calls around lines 7619 and 8446). Without this the rename would land on the instance but the grid header would still show the old name.
  • Reason columns are {eval}-reason for datasets and {eval}-{column}-reason for experiments, so the helper swaps the name prefix instead of rebuilding the whole string.
  • The rename runs before the existing reason-column reconciliation. That block does a get_or_create keyed partly on f"{eval_metric.name}-reason", so renaming afterwards would have created a duplicate reason column under the new name.

C. Frontend: unblock the Name field

  • EvalPickerConfigFull had disabled={isEditMode} on the Name input with a comment saying the name belongs to the UserEvalMetric instance rather than the template. That is exactly the name this PR makes editable, so the flag is gone.
  • The 50-character validation and helper text now apply in edit mode too. They were suppressed with !isEditMode before because the field could not be typed in.
  • Nothing else changed. resolvedName already resolved to the picker's evalName state in edit mode, and EvaluationDrawer already put it in the POST payload.

2) Why the changes were done

  • Product/UX: renaming was silently dropped. The request succeeded, the drawer closed, and nothing changed, so there was no signal that the name had not been saved.
  • Technical: reusing validate_eval_name and the existing uniqueness filter keeps rename and create consistent. Adding a separate rule set for renames would let a name exist that could never have been created.
  • Architecture: the column rename lives in a small static helper on the view rather than a post_save signal on UserEvalMetric, because the eval column and reason column are already reconciled inside this same view and this keeps the whole rename in one transaction.

3) Tests written + scenarios each covers

model_hub/tests/test_evaluation_api.pyTestEditAndRunUserEvalView

  • test_edit_persists_new_name — a rename is written to the instance instead of being dropped.
  • test_edit_renames_eval_and_reason_columns — the eval column and its -reason column follow the rename.
  • test_edit_rejects_name_taken_by_another_eval — a collision with another eval on the same dataset returns 400 and leaves the name alone.
  • test_edit_rejects_invalid_name_format — a name with spaces is rejected, matching the create path.
  • test_edit_without_name_keeps_existing_name — an empty name in the payload does not blank the eval out.
  • test_edit_with_unchanged_name_is_not_a_collision — resending the eval's own name does not trip the uniqueness check against itself.

One existing test changed: test_edit_and_run_user_eval_success sent "name": "Updated Eval". That name has a space and a capital letter, so it could never have been created through add_user_eval in the first place, and it now hits the shared validator. It sends updated-eval instead. The assertions are unchanged.

Run result: 87 passed in model_hub/tests/test_evaluation_api.py, plus 146 passed in tfc/tests/test_model_hub_api_contract_debt.py. black --check reports no diff inside the changed blocks (both files have pre-existing formatting drift on dev, which I left alone). eslint on the changed frontend file is clean apart from a pre-existing react-hooks/exhaustive-deps warning on an effect I did not touch.


4) How to run / test

cd futureagi
bin/test model_hub/tests/test_evaluation_api.py -k "EditAndRunUserEval" -v

UI steps (manual)

  1. Open a dataset that has an eval column.
  2. Right-click the eval column header and pick Edit Eval.
  3. The Name field is editable now. Change it and save.
  4. The eval and its reason column pick up the new name. Saving a name that another eval on the same dataset already uses returns the "Eval Template name must be unique" error instead of silently succeeding.

5) Screenshots / recordings

I did not attach UI screenshots. I could not bring the full compose stack up locally, so the frontend side of this is a code change I reasoned through rather than one I clicked through. Happy to redo it with screenshots if you would rather see that before merging.


6) Edge cases & considerations

  • Legacy names that would fail validation today. Existing evals may carry names created before validate_eval_name was enforced. Only the incoming name is validated, so those evals can still be renamed to something valid. They are just not silently rewritten.
  • Renaming to your own name. The uniqueness query excludes the eval being edited, so resending the current name is not a collision.
  • Empty name. The serializer allows allow_blank=True and the drawer always sends name, so a blank value keeps the current name rather than wiping it.
  • save_as_template. Skipped entirely, otherwise saving a template would rename the instance as a side effect.
  • Experiment scope. Experiment evals put the eval id in a -sourceid-{id} suffix and name reason columns {eval}-{column}-reason, so the helper filters and rewrites for that shape as well as the dataset one.
  • Reason column created later. If the eval was added with run=false and never ran, there is no column to rename yet. The filters just match nothing and the instance rename still applies.

7) Pre-existing issues (NOT introduced by this PR)

  • black --check and isort --check-only already fail on model_hub/views/develop_dataset.py and model_hub/tests/test_evaluation_api.py on dev. I matched the surrounding style rather than reformatting the files, since that would have buried the change in a few hundred unrelated lines.
  • eslint reports a react-hooks/exhaustive-deps warning on the effect at EvalPickerConfigFull.jsx:272. It is on dev too and is unrelated to the lines I touched.

8) Architectural / important decisions

  • Reuse validate_eval_name instead of a rename-specific rule set: a renamed eval should not be able to hold a name that add_user_eval would have rejected.
  • Rename columns in the view rather than a model signal: this view already reconciles those columns, and doing it here keeps the rename inside the existing transaction.atomic() block.
  • Rename before the reason-column reconciliation: that block's get_or_create matches on the reason column name, so ordering it the other way would create a duplicate column.
  • Left "Edit Column Name" hidden on eval columns: the guard comment says eval column names are changed by editing the eval, which is now true. Adding a second rename entry point would give two ways to set the same value.

Checklist

  • My code follows the style guide
  • I've added tests that prove my fix is effective or that my feature works
  • bin/test passes locally (backend)
  • yarn test:run passes locally (frontend)
  • yarn contracts:check passes if API surface changed
  • I've updated the documentation where relevant
  • No hardcoded secrets, URLs, or PII
  • I've signed the CLA
  • PR title follows Conventional Commits
  • Linear issue is linked and updated with status

Two notes on the boxes I left unchecked. I could not run the frontend suite or contracts:check on Windows: yarn install dies building the native canvas dependency, so I only linted the file I changed. The API surface is unchanged, no serializer fields were added or removed, so I do not think contracts need regenerating, but say the word if you want me to confirm. I do not have a Linear account, so I could not link the issue there. I will sign the CLA when the bot prompts.

Investigated with AI assistance.

EditAndRunUserEvalView accepted a
ame in the request serializer but never
assigned it, so the only way to change an eval's name was to delete it and
add it again, which re-ran the eval over every row.

The rename now goes through validate_eval_name and the same per-dataset
uniqueness check AddUserEvalView uses, and it carries over to the eval column
and its reason column, which both snapshot the name at creation time. The
save_as_template branch is untouched since
ame means the new template
there. Blank or unchanged names are no-ops.

Frontend: the Name field in the eval picker was hard-disabled in edit mode.
It is editable now, with the existing length validation applied in both modes.

Closes future-agi#1769
@onatozmenn

Copy link
Copy Markdown
Author

Two process questions, neither blocking on my side.

The CLA bot never showed up on this PR, so I left that box unchecked. I looked for it and could not find a CLA workflow in .github/workflows/ or any signing link in the repo, only the mention in CONTRIBUTING.md. If there is somewhere I should sign, point me at it and I will.

Also, the only check that ran here is label. The backend and frontend suites did not trigger, which I assume is a fork PR thing waiting on a maintainer to approve the run. Let me know if there is anything I need to do to get them going.

@onatozmenn

Copy link
Copy Markdown
Author

Good catch, you were right. I moved the rename validation and uniqueness check ahead of all transactional writes, including version creation. I also strengthened the collision regression test to verify that both the version count and pinned version remain unchanged after a rejected edit. The full TestEditAndRunUserEvalView suite passes (11 tests). Fixed in 439c0bd.

@Kirtofu

Kirtofu commented Aug 15, 2026

Copy link
Copy Markdown

Verified the new commit (439c0bd) against the diff — this addresses the transactional leak correctly:

  • The rename validation + uniqueness check now runs right after the column_deleted guard, before save_as_template's template write and before maybe_pin_new_version — i.e., before the first write in the atomic block. The comment there ("Returning a 400 from transaction.atomic() does not trigger a rollback") captures the exact reasoning.
  • The rename execution stays after the version pin and before the reason-column reconciliation, so the get_or_create still matches the renamed column.
  • test_edit_rejects_name_taken_by_another_eval now sets eval_template.owner = USER (so maybe_pin_new_version would genuinely attempt a version creation), and asserts pinned_version_id unchanged plus EvalTemplateVersion count unchanged after the 400. That's the right regression shape.

One tiny nit (non-blocking): if new_name and new_name != eval_metric.name: — a rename to the empty string is already impossible (blank name skips validation), so this is fine as-is; just noting the truthiness check means a hypothetical falsy-but-valid name would be skipped, which can't occur with the current validator.

Nothing further from my side — the fix looks mergeable pending maintainer review. Thanks for the quick turnaround!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants