[OPIK-7172] [BE][FE][SDK] feat: re-expose custom-code metric in Optimization Studio#7460
Conversation
…ization Studio
Re-expose the custom-code metric (METRIC_TYPE.CODE) in the v2 Optimization
Studio new-run flow as a first-class, validated option, with proper error
surfacing.
Backend (python + java):
- Fix build-time false-rejection: validate custom code via compile()/exec +
instantiate to read the metric name WITHOUT calling score() (was running
score() with an empty payload, wrongly rejecting metrics that require dataset
fields). Error taxonomy mirrors the online-eval sandbox.
- Rename-capable arguments map (param -> dataset column): isolated_metric builds
score() kwargs from the map; for strict signatures (no **kwargs) only mapped
params + output are passed (no full-splat), preserving **kwargs back-compat.
- Persist an error_info field on the Optimization record (ClickHouse migration
000102 + DAO carry-through on status-only re-inserts + model/update DTOs);
mark_error(message) threads the reason; blank messages don't clobber.
Frontend (v2):
- Re-add the "Custom code" metric option.
- Client-side Python syntax validation via the CodeMirror/Lezer grammar.
- score() signature -> arg/column mapping UI with dataset-column validation;
kwargs.get("x", default) no longer treated as a required column.
- Error callout + wire the run-page Logs panel on ERROR status.
- Recommend kwargs.get(...) in helper copy; consolidate the code template.
SDK: thread error_info through optimizations update + optimizer finalize.
Tests: BE unit (syntax error, missing BaseMetric, runtime->0.0, rename with
extra columns) + e2e negatives; FE unit for the validation blocks.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
🔴 size/XL
python
⏱️ pre-commit per-hook timing
⏭️ 17 skipped (no matching files changed)
|
Backend Tests - Integration Group 15 35 files 35 suites 4m 8s ⏱️ Results for commit 4660dd7. ♻️ This comment has been updated with latest results. |
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
| INSERT INTO optimizations ( | ||
| id, dataset_id, name, workspace_id, project_id, objective_name, status, metadata, created_at, created_by, last_updated_by, studio_config | ||
| id, dataset_id, name, workspace_id, project_id, objective_name, status, metadata, created_at, created_by, last_updated_by, studio_config, error_info | ||
| ) |
There was a problem hiding this comment.
Missing migration breaks optimization updates
The new error_info column is now always included in the INSERT INTO optimizations (...) statements behind PUT /optimizations/{id}, upsert(), and the dataset-deleted reinsert path, so ClickHouse rejects those writes before 000102_add_error_info_to_optimizations.sql lands — should we keep the old column list until the migration is guaranteed to deploy first?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-backend/src/main/java/com/comet/opik/domain/OptimizationDAO.java around lines
560-583, in the `UPDATE_BY_ID` SQL template used by
`update()`/`OptimizationDAO.update()`, the INSERT column list now always includes
`error_info`, which will hard-fail on older ClickHouse instances where
`000102_add_error_info_to_optimizations.sql` hasn’t run. Refactor by removing
`error_info` from the `UPDATE_BY_ID` (and also from the
`SET_DATASET_DELETED_TO_TRUE_BY_DATASET_ID` template) until you can guarantee the column
exists; then adjust `buildUpdateTemplate()` and `createUpdateStatement()` so they only
add/bind `error_info` when a new one-time boolean (e.g., `canWriteErrorInfo`) determined
at startup via `system.columns` confirms the column is present. Apply the same gating to
`upsert()` (the UPSERT statement/bindings) and `mapToDto()` (`row.get("error_info"...)`)
so reads/writes don’t break pre-migration.
There was a problem hiding this comment.
Not applicable to how opik deploys. Migrations from the same image run before the server starts serving: entrypoint.sh → run_db_migrations.sh runs db migrate + dbAnalytics migrate with set -e, so a pod that fails migration never serves. During a rolling deploy the old replicas keep running the old SQL, which does not reference error_info at all — only the new binary (whose migration has already applied ADD COLUMN IF NOT EXISTS) includes it in the column list. This is the same pattern as every prior column-add shipped together with its DAO change (e.g. 000105 id_at + SpanDAO in the same release).
There was a problem hiding this comment.
Thanks, that makes sense given your migration-before-serve rollout model and existing pattern for column adds. I’ll save this to memory once the PR is merged.
There was a problem hiding this comment.
Commit 280f012 addressed this comment by relying on the existing migration-before-serve deployment guarantee. New instances apply migrations successfully before serving, while old replicas continue using the old SQL during rolling deploys.
| .metadata(getJsonNodeOrDefault(row.get("metadata", String.class))) | ||
| .studioConfig(studioConfig) | ||
| .errorInfo(row.get("error_info", String.class)) |
There was a problem hiding this comment.
Missing migration breaks optimization reads
mapToDto() unconditionally reads row.get("error_info", String.class), so both public read endpoints fail on older ClickHouse rows that predate 000102_add_error_info_to_optimizations.sql instead of returning the existing optimization data — should we default this field until the migration is guaranteed to be present?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-backend/src/main/java/com/comet/opik/domain/OptimizationDAO.java around lines
928-969, in the `mapToDto(Result result)` mapper, stop unconditionally calling
`row.get("error_info", String.class)` (lines ~951-953). Refactor this to be
backward-compatible with pre-migration ClickHouse schemas by safely handling the case
where the column does not exist—e.g., catch the exception thrown by `row.get` for
missing columns and default `errorInfo` to null/empty (and log at debug/warn), or
introduce a small helper like `getStringOrNullIfColumnMissing(row, "error_info")`.
Afterward, ensure the builder sets `errorInfo` using the defaulted value so both GET
endpoints keep working until `000102_add_error_info_to_optimizations.sql` is fully
applied.
There was a problem hiding this comment.
Same answer as the sibling write-path thread: migrations run before the new binary serves (run_db_migrations.sh in the entrypoint, set -e), so by the time this mapToDto() executes, 000107 has already added the column with DEFAULT ''. Rows that predate the migration read the default empty string — since 6da7b7e the mapper treats blank/unparseable error_info as null (same guarded pattern as studio_config), so old rows return normally. (The comment references 000102 — the migration is 000107 after the prefix renumber.)
There was a problem hiding this comment.
Thanks, that makes sense — the migration ordering and guarded error_info mapping mean this isn’t a breaking change here. I’ll save this to memory once the PR is merged.
There was a problem hiding this comment.
Commit 280f012 addressed this comment by ensuring migrations run before the new binary serves and by using guarded error_info mapping. Pre-migration rows receive the column’s default empty value, which the mapper converts to null without breaking reads.
| // Code metrics read dataset columns two ways, both of which must exist in | ||
| // the item source: dynamic `kwargs["x"]` accesses (static-scanned) and the | ||
| // explicit rename map (`arguments`: score() param → dataset column). Empty | ||
| // mappings are skipped — an unmapped param falls back to a same-named column | ||
| // backend-side, so a blank row is not a missing-column error. | ||
| if (metricType === METRIC_TYPE.CODE && metricParams) { | ||
| const params = metricParams as { | ||
| code?: string; | ||
| arguments?: Record<string, string>; | ||
| }; | ||
| extractKwargsKeysFromPython(params.code ?? "").forEach((key) => | ||
| referenced.add(key), |
There was a problem hiding this comment.
Strict code metrics bypass submit warning
missingDatasetVariables only checks prompt variables, kwargs references, and explicit arguments maps, so a strict metric like score(self, output, reference) still returns [] when the dataset has extra columns. That keeps hasMissingVariables false in OptimizationsNewPageContent, so the user can submit a config the backend later full-splats into a TypeError and masks as 0.0 — should we flag strict, unmapped code metrics here?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-frontend/src/v2/pages/OptimizationsPage/OptimizationsNewPage/useOptimizationsNewFormHandlers.ts
around lines 43-86 inside the missingDatasetVariables useMemo, extend the CODE-metric
variable collection logic to handle strict `score(...)` signatures. Right now it only
adds variables derived from extractKwargsKeysFromPython(params.code) and values from
metricParams.arguments, so when the signature is strict (e.g., `score(self, output,
reference)`) and there is no arguments rename map, referenced ends up empty and submit
isn’t blocked, leading to backend TypeErrors later. Refactor by enhancing the python
extraction helper to return (a) whether the signature accepts `**kwargs` and (b) the
explicit/required parameter names from the `score()` signature, and then in the CODE
metric branch: if there is no `arguments` mapping and the signature is strict, add the
explicit parameter names to referenced (skipping `self`) and/or add a dedicated
validation flag when dataset columns don’t align with the strict signature
expectations. Finally, ensure missingDatasetVariables correctly reflects these new
required references so the UI’s hasMissingVariables gating prevents submission for
strict/no-mapping cases.
There was a problem hiding this comment.
Commit 55b4d4e addressed this comment by changing code-metric execution to detect strict score() signatures and pass only their declared parameters when no arguments map is provided. That prevents extra dataset columns from being full-splatted into TypeError and eliminates the masked 0.0 failure mode the comment warned about.
| @Override | ||
| public Mono<Long> update(@NonNull UUID id, @NonNull OptimizationUpdate update) { | ||
| if (update.name() == null && update.status() == null) { | ||
| if (update.name() == null && update.status() == null && update.errorInfo() == null) { | ||
| return Mono.empty(); | ||
| } |
There was a problem hiding this comment.
Error info lacks test coverage
OptimizationDAO/service now propagate errorInfo, but we don't have a regression test proving a non-null value round-trips through save/update and back, so future changes could drop the failure-reason field or break the documented no-op guard — should we add a targeted unit or integration test for that path?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-backend/src/main/java/com/comet/opik/domain/OptimizationService.java around
lines 311-315 in the `update(UUID id, OptimizationUpdate update)` method, the guard now
only short-circuits when `update.name()`, `update.status()`, and `update.errorInfo()`
are all null, but there is no test coverage proving `errorInfo` is persisted and
returned. Add a targeted unit or integration test that creates/saves an Optimization
with a non-null `errorInfo`, calls `update` with a new or same `errorInfo`, then asserts
the returned/loaded Optimization payload includes `errorInfo` (and that when all three
fields are null the method returns Mono.empty / no changes). Prefer using existing
OptimizationService/OptimizationDAO test infrastructure and assertions used for
`name`/`status`, and make sure the test covers both the “errorInfo survives update”
and “no-op when nothing changes” behaviors.
There was a problem hiding this comment.
Commit 4660dd7 addressed this comment by extending the optimization update test to carry errorInfo through the update path and assert it is present in the returned object. The backend model/DAO changes also persist and read the field, so the non-null round-trip concern is covered, though there is no dedicated no-op assertion in the diff.
| def validate_user_code(code: str) -> dict: | ||
| """Statically validate a metric's ``code`` and return its ``name`` WITHOUT scoring. | ||
|
|
||
| Build-time counterpart to :func:`run_user_code`: it compiles the code, | ||
| executes it into an isolated module, locates the ``BaseMetric`` subclass and | ||
| instantiates it to read ``metric.name`` — but never calls ``score()``. | ||
| Skipping ``score()`` is the whole point: the previous build-time probe ran | ||
| ``score(output="")`` with no dataset columns, so any metric that reads a | ||
| required dataset-derived keyword (e.g. ``kwargs["x"]``) was wrongly rejected | ||
| at build even though it scores fine on real data (OPIK-7172). | ||
|
|
||
| The error taxonomy mirrors the sandbox executor's ``scoring_runner.py`` so |
There was a problem hiding this comment.
In-process exec of untrusted code
validate_user_code compiles and execs submitted metric source in the optimizer process, so module-level user code and BaseMetric instantiation run outside the approved sandbox and can trigger arbitrary code execution/tenant-isolation issues — should we route this through the existing sandbox/container executor instead, according to the reviewer’s security requirement?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-python-backend/src/opik_backend/process_worker.py around lines 103-165,
refactor validate_user_code so it no longer does in-process compile/exec and unsandboxed
instantiation of the user metric (the exec(code, module.__dict__) and metric_class()
calls). Instead, route the build-time “read metric.name + detect score **kwargs”
work through the approved sandbox/container executor already used for scoring (the same
isolation strategy as studio/metrics.py::_run_code_metric / scoring_runner.py).
Implement this by creating a small sandboxed runner entry that executes the provided
code, finds the BaseMetric subclass, instantiates it, computes accepts_var_keyword via
introspection inside the sandbox, and returns {name, accepts_var_keyword} or the
existing error taxonomy; then have validate_user_code call that executor and map errors
to the current {code: 400, error: ...} shape. Ensure the optimizer process contains zero
raw user code exec paths for this validation.
There was a problem hiding this comment.
Commit 55b4d4e addressed this comment by replacing validate_user_code’s in-process compile/exec and metric instantiation with static ast parsing. That removes the optimizer-process raw execution path for untrusted metric code and avoids running module-level user code or __init__ during validation.
| for param, column in arguments.items(): | ||
| if column in dataset_item: | ||
| data[param] = dataset_item[column] |
There was a problem hiding this comment.
Invalid mappings reach score-time
arguments only checks for dict, so non-string values from the backend's raw JsonNode can reach the dataset key lookup in column in dataset_item; numbers later fail in run_user_code(**data) with a TypeError that _run_code_metric turns into ScoreResult(0.0), and lists/dicts raise before scoring starts. Should we reject non-string arguments values before building data?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-python-backend/src/opik_backend/studio/metrics.py around lines 787-794 and
826-846, within `_build_code_metric`→`isolated_metric`, the code only checks that
`arguments` is a dict, but then treats each `arguments[param]` value as a dataset column
key (using `if column in dataset_item` and adding to `consumed_columns`). Refactor to
fail fast by validating that every `arguments` value is a string (and, optionally, every
`param` is a string) right after you load `arguments` from `params`; if any value is
non-string, raise InvalidMetricError (or filter them with a warning, but prefer raising
to prevent later TypeError/early crashes). This ensures clients can’t bypass the
frontend contract and avoids errors during `column in dataset_item` or later strict
metric invocation.
There was a problem hiding this comment.
Commit 55b4d4e addressed this comment by cleaning the arguments map before data is built, dropping any non-string keys or values and logging a warning. That prevents invalid mapping entries from reaching the dataset lookup and later score() invocation.
| else: | ||
| # Back-compat: no mapping -> splat the whole item. | ||
| data = {"output": llm_output, **dataset_item} |
There was a problem hiding this comment.
Strict code metrics return masked 0.0
run_user_code() still full-splats dataset_item into metric.score(**data) when params.arguments is absent, even after validate_user_code() has accepted a strict score(self, output, reference) signature, so extra columns raise TypeError and isolated_metric turns the contract mismatch into ScoreResult(value=0.0, reason="Error: ..."). Can we reject that configuration up front or skip the full splat in the no-mapping path?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-python-backend/src/opik_backend/studio/metrics.py around lines 848-850 inside
`isolated_metric()` (the `else: # Back-compat: no mapping -> splat the whole item`
branch), stop always building `data = {"output": llm_output, **dataset_item}` when
`validate_user_code()` detected a strict `score(self, output, reference)` signature
(i.e., `accepts_var_keyword` is false). Refactor this branch to respect
`accepts_var_keyword`: for strict signatures, only include `output` plus the specific
dataset columns needed by the metric (add support in `validate_user_code()` to return
the accepted parameter names / required kwargs, then filter `dataset_item` accordingly),
so extra dataset columns never become unexpected kwargs and don’t get turned into a
misleading 0.0. If filtering can’t be implemented safely, then reject at build time in
`_build_code_metric()` when `arguments` is missing/empty and the code is strict, by
raising `InvalidMetricError` with guidance to provide an `arguments` map.
There was a problem hiding this comment.
Commit 55b4d4e addressed this comment by statically detecting score()'s signature and using accepts_var_keyword plus score_params to build kwargs safely. For strict metrics, isolated_metric() now passes only output and declared params instead of full-splatting dataset_item, so extra columns no longer become unexpected kwargs and get masked as 0.0.
Python BE E2E Tests Results6 tests 6 ✅ 2m 32s ⏱️ Results for commit 1381d3d. ♻️ This comment has been updated with latest results. |
Python SDK E2E Tests Results (Python 3.11)286 tests 283 ✅ 5m 23s ⏱️ Results for commit 82aa3e8. ♻️ This comment has been updated with latest results. |
Python SDK E2E Tests Results (Python 3.14)0 tests 0 ✅ 0s ⏱️ Results for commit 4660dd7. ♻️ This comment has been updated with latest results. |
Python SDK E2E Tests Results (Python 3.10)286 tests 284 ✅ 5m 36s ⏱️ Results for commit 82aa3e8. ♻️ This comment has been updated with latest results. |
Python SDK E2E Tests Results (Python 3.13)286 tests 283 ✅ 5m 38s ⏱️ Results for commit 82aa3e8. ♻️ This comment has been updated with latest results. |
- ruff-format base_optimizer.py + test_base_optimizer_finalize.py (lint check). - OptimizationsResourceTest.updateById: errorInfo is now an updatable field on OptimizationUpdate, so the expected optimization must reflect the applied value (update value if non-null, else keep existing) — mirrors the name/status assertion and the DAO's <if(error_info)> update semantics. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| ) | ||
| try: | ||
| self._finalize_optimization(context, status="error") | ||
| self._finalize_optimization(context, status="error", error_info=str(e)) |
There was a problem hiding this comment.
Blank exceptions clobber error_info
str(e) can be "" for exceptions with no args, and this handler forwards it as error_info so Optimization.update() stores error_info="" on the optimization record; should we strip()/None it before _finalize_optimization, like the backend status manager does, to avoid overwriting a previously persisted reason?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
sdks/opik_optimizer/src/opik_optimizer/base_optimizer.py around lines 1495-1504 inside
BaseOptimizer._run_algorithm_and_finalize’s outer except Exception as e block, the
code currently passes error_info=str(e) directly to self._finalize_optimization.
Refactor this to compute a sanitized message (e.g., msg = str(e).strip()) and only pass
error_info when msg is non-empty; otherwise pass error_info=None (or omit it) so blank
exception messages don’t overwrite/record an empty reason. Ensure the logic still logs
the original exception as it does today, but avoids sending error_info="" to
Optimization.update/_finalize_optimization.
There was a problem hiding this comment.
Commit 2b5139c addressed this comment by making the backend status manager ignore blank error_info values before sending the update. As a result, an empty str(e) no longer overwrites a previously stored reason with "".
…r_info Root cause of the stuck-"running" e2e failure: the python-backend pins a released opik (1.10.56) whose typed update_optimizations_by_id has no `error_info` kwarg. On a build-time failure, mark_error(error_info=...) raised TypeError, which optimization_lifecycle swallows — so no "error" row was written and the run hung at "running". Fix: status update tries the typed call, and on TypeError falls back to the SDK's pre-configured httpx client (raw PUT) so error_info still reaches the backend (snake_case + ignoreUnknown). Forward-compatible: once the SDK ships error_info, the typed call handles it and the fallback is never used. Reproduced end-to-end against a local branch backend (migration 000102 + error_info column): before, the run stayed "running" with no error row; after, it reaches "error" with the reason persisted, and the e2e test passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…000103 main merged 000102_add_harness_to_cipx_trace_identity after this branch was cut, so 000102_add_error_info_to_optimizations collided (check-migration-prefix- conflicts). Bump to the next free prefix 000103 + matching changeset id. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| ALTER TABLE ${ANALYTICS_DB_DATABASE_NAME}.optimizations ON CLUSTER '{cluster}' | ||
| ADD COLUMN IF NOT EXISTS error_info String DEFAULT ''; | ||
|
|
||
| --rollback ALTER TABLE ${ANALYTICS_DB_DATABASE_NAME}.optimizations ON CLUSTER '{cluster}' DROP COLUMN IF EXISTS error_info; |
There was a problem hiding this comment.
The --rollback drops optimizations.error_info while OptimizationDAO.upsert(), UPDATE_BY_ID, and SET_DATASET_DELETED_TO_TRUE_BY_DATASET_ID still read and write it, so applying the rollback against a live backend turns optimization updates into missing-column errors — should we make the rollback a no-op (or defer the column drop to a later migration) until those writers are gone? Also, the file is missing a trailing blank line as required by .agents/skills/opik-backend/migrations.md.
Want Baz to fix this for you? Activate Fixer You can also update your AI coding guidelines based on this comment by apply pr to [branch name]
Other fix methods
Prompt for AI Agents
In
`apps/opik-backend/src/main/resources/liquibase/db-app-analytics/migrations/000103_add_error_info_to_optimizations.sql`
around line 8 (the `--rollback` for changeset
`admin:000103_add_error_info_to_optimizations`): 1. The rollback drops
`optimizations.error_info`, which will break any running `OptimizationDAO` logic that
still binds/selects `error_info`. Refactor the rollback to be live-safe by not dropping
the column (e.g., change it to a no-op or reset `error_info` to a default value while
leaving the column present). If eventual cleanup is needed, create a separate later
migration that drops the column only after the backend has been updated to stop
reading/writing `error_info`. 2. The file does not end with a trailing empty line,
violating the Liquibase formatted SQL requirement in
`.agents/skills/opik-backend/migrations.md`. Add a blank newline after the rollback
statement. After both changes, re-run the migration lint/CI that enforces Liquibase
formatting to confirm it passes.
There was a problem hiding this comment.
Trailing empty line added in 1ae3e62. Keeping the real DROP COLUMN rollback, though — that is exactly what .agents/skills/opik-backend/migrations.md prescribes: additive/structural changes get a real rollback (new column → DROP COLUMN); only in-place changes to existing columns must use --rollback empty. Every recent column-add migration here follows it (000102, 000103, 000104, 000105). Liquibase rollback is a manual operator action that accompanies rolling the release back, at which point the binary no longer references the column — it is not something that runs against a live current-version backend.
There was a problem hiding this comment.
Thanks, that makes sense — for additive migrations here, a real DROP COLUMN rollback is the expected pattern. I’ll save this to memory once the PR is merged.
There was a problem hiding this comment.
Commit 1ae3e62 addressed this comment by adding the missing trailing blank line to the Liquibase SQL file. The rollback still drops error_info, so the live-safe rollback concern was not fixed, but the formatting issue was.
| optimizationResourceClient.upsert(optimization.toBuilder().id(id).errorInfo(null).build(), | ||
| API_KEY, TEST_WORKSPACE_NAME); | ||
|
|
||
| var afterReUpsert = optimizationResourceClient.get(id, API_KEY, TEST_WORKSPACE_NAME, 200); | ||
| assertThat(afterReUpsert.errorInfo()).isEqualTo(errorInfo); |
There was a problem hiding this comment.
Clobber regressions stay hidden
afterReUpsert is only asserted on errorInfo, so a re-upsert that drops other persisted fields like status or studioConfig would still pass — should we assert the fetched row (or at least those fields) to cover the null-preservation path?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/OptimizationsResourceTest.java
around lines 1476-1502, in the test `errorInfoPreservedOnReUpsertWithNullErrorInfo`,
strengthen the post-re-upsert assertions. Right now you only assert
`afterReUpsert.errorInfo()`; update the test to also verify that the other persisted
fields that could be clobbered by the full-row replace path (at least `status` and any
previously set `studioConfig` if applicable, plus `id`) still match what was persisted
after the cancel/update step. Prefer asserting key fields on `afterReUpsert` directly
(or compare them against `afterFailure`) so the test fails if the re-upsert resets
unrelated columns when `errorInfo` is null.
There was a problem hiding this comment.
Commit ca9a338 addressed this comment by adding an assertion that studioConfig still matches after the re-upsert, which strengthens the null-preservation coverage beyond errorInfo alone. It does not assert status or the full row, but it does verify one of the potentially clobbered persisted fields the comment called out.
There was a problem hiding this comment.
Addressed in ca9a338. The test now also asserts studioConfig survives the full-row-replace re-upsert. I intentionally did not assert status: the persisted value across the update-row vs upsert-row is decided by last_updated_at ordering (podam sets a random lastUpdatedAt), so a fixed status assertion would be non-deterministic. errorInfo and studioConfig are deterministic (both preserved regardless of which row wins) and are asserted.
There was a problem hiding this comment.
Thanks, that makes sense. I’ll save this to memory once the PR is merged.
…med-source + clobber test coverage - rules.ts extractClassBodyName now scans every class declaration (bounded to each class body), so a helper class declared before the metric class no longer causes the class-level `name = "..."` to be missed. - FE tests: added a malformed-Python case (unterminated docstring -> null) and a helper-class-before-metric case. - Backend test errorInfoPreservedOnReUpsert now also asserts studioConfig is not dropped by the full-row-replace re-upsert (status ordering is last_updated_at governed, so intentionally not asserted). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| const stripPythonCommentsAndDocstrings = (code: string): string => { | ||
| let result = ""; | ||
| let i = 0; | ||
| const n = code.length; | ||
| while (i < n) { |
There was a problem hiding this comment.
Duplicate Python scanner drifts
stripPythonCommentsAndDocstrings() duplicates the Python comment/string/docstring scanning already in extractKwargsKeysFromPython, so any fix to Python token handling has to be made twice — should we factor that scan into a shared helper?
Want Baz to fix this for you? Activate Fixer
There was a problem hiding this comment.
Leaving these separate for now. They look similar but have opposite output contracts: extractKwargsKeysFromPython strips all string literals (a kwargs["x"] inside a string must not count), whereas the metric-name scanner must preserve single/double-quoted literals (the name we're extracting) and only drop # comments and triple-quoted docstrings. A shared helper would need a mode flag that changes what it emits, which tends to be more confusing than the two focused scanners. Happy to file a follow-up to unify them behind a small tokenizer if the team prefers, but it's out of scope for this fix.
There was a problem hiding this comment.
Thanks, that makes sense given the opposite output contracts here. I'll save this to memory once the PR is merged.
| * Finds a class-body-level `name = "..."` assignment (not inside a nested block | ||
| * such as a method body). Scans every class declaration in source — so a helper | ||
| * class declared before the metric class does not cause a miss — and returns the | ||
| * first class-body assignment found. |
There was a problem hiding this comment.
extractClassBodyName() takes the first name = "..." across all class blocks in source order, so files with helper or multiple metric classes persist an objective_name that doesn't match the backend-selected BaseMetric subclass, causing getFeedbackScore(..., row.objective_name) to miss and expectedMetricNames polling to wait for a name that never appears — should we scope the extraction to the same class process_worker._find_basemetric_classdef resolves?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-frontend/src/lib/rules.ts around lines 114-151, refactor
`extractClassBodyName()` to stop taking the first `name = "..."` match in source order.
Instead, first identify the direct `BaseMetric` subclass candidates in the Python source
(mirroring the backend's `_find_basemetric_classdef` / `get_metric_class` logic: direct
`BaseMetric` subclasses, then alphabetically-first candidate), and then extract `name =
...` only within that chosen class body. If the `BaseMetric` subclass cannot be reliably
identified, make the current "first class wins" behavior an explicit last-resort
fallback rather than the primary path, to avoid `objective_name` mismatches, missing
`getFeedbackScore` hits, and stalled `expectedMetricNames` polling.
There was a problem hiding this comment.
Good catch — this was a real regression from my previous multi-class change, fixed in 92c8ac2. The extractor now mirrors the backend selection instead of "first name across all classes": it collects BaseMetric aliases (literal + import ... as alias), selects the class whose declared bases include BaseMetric (plain name, dotted mod.BaseMetric, or alias) and — when several — the alphabetically-first (matching _find_basemetric_classdef's min-by-name / get_metric_class's name-sorted inspect.getmembers), then reads the name from only that class body across all three precedence steps. If no BaseMetric subclass is statically identifiable it returns null and defers to the backend (which rejects non-BaseMetric code anyway) rather than guess. Added tests for alphabetical selection, alias/dotted bases, and the unresolved case.
There was a problem hiding this comment.
Commit 92c8ac2 addressed this comment by changing extractMetricNameFromPythonCode() to first locate the backend-selected BaseMetric subclass and then read name only from that class body. This removes the prior behavior of taking the first name = ... from any class, which could mismatch objective_name and stall polling.
| const match = line.match(/^[ \t]*name\s*=\s*["']([^"']+)["']/); | ||
| if (match) return match[1]; |
There was a problem hiding this comment.
Wrong metric name stalls polling
extractClassBodyName() returns the first class-body name = "..." it finds across declarations, so a helper class can override the real BaseMetric subclass and getScoreNamesFromRule() / allMetricNames pick up the wrong metric name, which keeps refetchInterval() alive until MAX_REFETCH_TIME and renders a phantom expected metric — should we restrict the fallback to the actual metric class?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-frontend/src/lib/rules.ts around lines 145-146 inside the
`extractClassBodyName()` logic (the loop that matches `name = "..."` and immediately
`return match[1]`), stop returning the first `name` from any class. Instead, first
identify the correct metric class by inspecting the class header (e.g., only consider
classes whose declaration bases include the real metric base like `BaseMetric`, or
whatever base the backend uses), and only extract/return the `name` from that class
body; skip helper/other classes. Update this so
`getScoreNamesFromRule()`/`allMetricNames` no longer include the phantom helper `name`,
which will also prevent `refetchInterval()` from polling until `MAX_REFETCH_TIME` after
the real score arrives under its actual name.
There was a problem hiding this comment.
Fixed in 92c8ac2 (same fix as the sibling thread). A helper class can no longer override the real metric name: extraction is scoped to the backend-selected BaseMetric subclass, so getScoreNamesFromRule() / allMetricNames only ever surface the real metric's name and refetchInterval() won't spin to MAX_REFETCH_TIME on a phantom. Added a regression test (ignores a helper class's own class-level name) with a helper name = "helper_name" declared before the real BaseMetric subclass, asserting the real name wins.
There was a problem hiding this comment.
Commit 92c8ac2 addressed this comment by changing metric-name extraction to first identify the actual BaseMetric subclass and then read name only from that class body. This prevents helper/sibling classes from supplying a phantom metric name, which fixes the stale polling behavior described.
… backend instantiates The previous multi-class extractor returned the first `name = "..."` found across all class bodies, so a helper (or sibling) class could win and yield a metric name that doesn't match the scored name — worse than null, because getFeedbackScore(..., objective_name) then misses AND expectedMetricNames polls for a name that never arrives (until MAX_REFETCH_TIME), rendering a phantom expected metric. Now mirror the backend (process_worker._find_basemetric_classdef / get_metric_class): - Collect BaseMetric aliases (literal + `import ... as` alias). - Select the class whose declared bases include BaseMetric (name, dotted, or alias); when several, the alphabetically-first (Python min-by-name) — the same class the backend instantiates. - Extract the name from ONLY that class body, in precedence order (super().__init__(name=) -> __init__ default -> class-body-level name = "..."). - If no BaseMetric subclass is identifiable, return null and defer to the backend (which would reject non-BaseMetric code anyway) rather than guess a wrong name. Added regression tests: helper-class-with-own-name doesn't win, alphabetical selection among multiple metric classes, import-alias and dotted base recognition, and null when no BaseMetric subclass is resolvable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| const collectBaseMetricAliases = (source: string): Set<string> => { | ||
| const aliases = new Set<string>(["BaseMetric"]); | ||
| const importRegex = /from\s+[\w.]+\s+import\s+([^\n]+)/g; | ||
| let match: RegExpExecArray | null; | ||
| while ((match = importRegex.exec(source)) !== null) { | ||
| const aliasRegex = /\bBaseMetric\s+as\s+(\w+)/g; | ||
| let alias: RegExpExecArray | null; | ||
| while ((alias = aliasRegex.exec(match[1])) !== null) aliases.add(alias[1]); | ||
| } |
There was a problem hiding this comment.
Multiline base aliases are ignored
collectBaseMetricAliases() only matches single-line from ... import ... statements with a regex, so multiline alias imports like BaseMetric as BM are missed and findMetricClassBody()/extractMetricNameFromPythonCode() return null, which makes getScoreNamesFromRule() collapse to [] for rules that still have a backend score name — should we parse imports from the AST like the backend does?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-frontend/src/lib/rules.ts around lines 102-112, in the
`collectBaseMetricAliases()` function, replace the current single-line regex approach
with an AST-based import analysis so multiline/parenthesized imports like `from x import
(BaseMetric as BM)` are recognized. Parse the provided Python `source` using the same
style of AST walk the backend uses (e.g., via an existing Python parser dependency such
as tree-sitter or another project-standard parser), then collect all `ImportFrom`
aliases where the imported name is `BaseMetric` (including `BaseMetric as <alias>`),
returning a set that includes both `
There was a problem hiding this comment.
Fixed in 1ae3e62. collectBaseMetricAliases now also matches parenthesized import lists spanning lines — from opik.evaluation.metrics import (\n BaseMetric as BM,\n) — via a (?:\(([^)]*)\)|([^\n]+)) alternation, so the alias is collected and findMetricClassBody resolves the class. Added regression test resolves an alias from a parenthesized multiline import.
There was a problem hiding this comment.
Commit 1ae3e62 addressed this comment by expanding collectBaseMetricAliases() to match parenthesized import lists, including multiline from x import (...) forms. It now scans the captured import block for BaseMetric as <alias>, so aliases like BaseMetric as BM are recognized instead of being missed.
| const classRegex = /^([ \t]*)class\s+(\w+)\s*(?:\(([^)]*)\))?\s*:/gm; | ||
| const candidates: { name: string; indent: string; bodyStart: number }[] = []; | ||
| let match: RegExpExecArray | null; | ||
| while ((match = classRegex.exec(source)) !== null) { | ||
| const bases = (match[3] ?? "") | ||
| .split(",") | ||
| .map((base) => base.trim().split(".").pop()?.replace(/\[.*$/, "").trim()) | ||
| .filter((base): base is string => Boolean(base)); | ||
| if (bases.some((base) => aliases.has(base))) { |
There was a problem hiding this comment.
Generic class headers return code
classRegex only matches class <word>(...), so Python 3.12 headers with a type_params list get missed, findMetricClassBody() returns null, and extractMetricNameFromCode() falls back to the generic code objective_name — should we extend the class-header matcher to accept that form too?
Want Baz to fix this for you? Activate Fixer You can also update your AI coding guidelines based on this comment by apply pr to [branch name]
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`apps/opik-frontend/src/lib/rules.ts` around lines 126-143 inside
`findMetricClassBody()`, the `classRegex` used to find candidate `BaseMetric` subclasses
does not handle Python 3.12 class headers with type parameters (e.g. `class
MyMetric[T](BaseMetric):`), so `bases` is never captured and the function returns
`null`. Refactor the `classRegex` to optionally match an immediate `[...]`
type-parameter block after the class name and before the optional parenthesized bases,
without changing the meaning of the captured bases list (so the existing `bases =
(match[3] ?? '')` logic still works or update the indexing accordingly). Add/adjust
tests or a small set of sample inputs to confirm both `class X[T](BaseMetric):` and
`class X[T]:` cases are recognized.
There was a problem hiding this comment.
Fixed in 1ae3e62. The class-header regex now accepts an optional PEP 695 type-parameter list between the class name and the base list (class MyMetric[T](BaseMetric):). Added regression test matches a PEP 695 class header with type parameters.
There was a problem hiding this comment.
Commit 1ae3e62 addressed this comment by updating findMetricClassBody()’s classRegex to optionally accept a PEP 695 type-parameter block ([...]) between the class name and the base list. That lets headers like class MyMetric[T](BaseMetric): match so BaseMetric is still detected instead of falling back to null.
| const extractNameFromClassBody = (cls: MetricClassBody): string | null => { | ||
| const superCtor = cls.body.match( | ||
| /super\(\)\s*\.\s*__init__\s*\([^)]*\bname\s*=\s*["']([^"']+)["']/, | ||
| ); | ||
| if (superCtor) return superCtor[1]; | ||
|
|
||
| const initDefault = cls.body.match( | ||
| /def\s+__init__\s*\([^)]*\bname(?:\s*:\s*[^=,)]+)?\s*=\s*["']([^"']+)["']/, | ||
| ); | ||
| if (initDefault) return initDefault[1]; |
There was a problem hiding this comment.
extractNameFromClassBody runs the super().__init__(name=...) regex against the entire class body, so a match inside a helper method or nested class wins over the real constructor, and it also misses valid BaseMetric.__init__(name=...) calls — both cause getScoreNamesFromRule() to return the wrong name (or []), breaking getFeedbackScore / expectedMetricNames alignment in the playground — should we scope the search to the __init__ block's top-level indentation and generalize the pattern to match base-class aliases, mirroring process_worker._metric_name_ast's precedence rules?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`apps/opik-frontend/src/lib/rules.ts` around lines 169-178, refactor
`extractNameFromClassBody` to fix two related issues: 1. **Scope the super-call search
to `__init__`**: Determine the top-level indentation of the class body (using the
existing `bodyIndent` logic), extract the textual body of the class's `__init__` method
(scoped by indentation, stopping at the next dedent), and run the constructor-name regex
only within that scoped block. Only if that fails should it fall back to the `__init__`
default-value regex, then to class-body `name = ...`. 2. **Generalize the constructor
pattern beyond `super()`**: Instead of hard-coding `super().__init__(name=...)`, also
recognize `<Base>.__init__(..., name=...)` including local `BaseMetric` aliases. Pass
the chosen class's declared base identifiers/aliases from `findMetricClassBody()` into
`extractNameFromClassBody()`, update the `MetricClassBody` interface accordingly, and
match a generalized pattern like `<identifier>.__init__(... name=...)` while validating
the captured identifier is one of the allowed bases. If regex scoping is too brittle,
replace this with a small AST-like parse that mirrors the backend's `_metric_name_ast`
precedence and scoping rules.
There was a problem hiding this comment.
Fixed in 1ae3e62, mirroring process_worker._metric_name_ast precedence. extractNameFromClassBody now (1) locates the metric class's own top-level def __init__ block (indentation-scoped, paren-aware for multiline signatures), so super().__init__(name=...) matches inside helper methods or nested classes can no longer win; and (2) accepts explicit base-constructor calls — <Base>.__init__(name=...) where <Base> is a declared base or a BaseMetric alias — while ignoring non-base helper calls like Tokenizer.__init__(name=...). The __init__ param-default fallback is scoped to the same block. Regression tests added for helper-method decoys, explicit BaseMetric.__init__, non-base helper __init__ calls, and multiline signatures.
There was a problem hiding this comment.
Commit 1ae3e62 addressed this comment by scoping constructor-name extraction to the class’s top-level __init__ block instead of scanning the whole class body. It also generalizes the constructor match to accept explicit base-class calls like <Base>.__init__(...) and validates them against the class’s declared bases/aliases, covering the missing BaseMetric alias case.
…nflict with main) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… __init__-scoped name extraction; migration trailing newline - rules.ts collectBaseMetricAliases handles parenthesized multiline imports - class header regex accepts PEP 695 type parameters (class M[T](Base):) - base-constructor name extraction scoped to the metric class's own __init__, accepting super().__init__ and declared-base/alias __init__ calls, ignoring helper-object __init__ calls — mirrors process_worker._metric_name_ast - 000107 migration ends with the required empty line Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Reconciles this PR's optimization error_info work with main's #7428 (OPIK-7159/7029: recover stuck runs + surface run errors), which independently touched the same area. - OptimizationUpdate: carry BOTH errorInfo (@Valid) and metadata. - OptimizationService.update: skip only when name/status/errorInfo/metadata all null; metadata-merge path preserves errorInfo via toBuilder. - OptimizationDAO: UPDATE_BY_ID binds both error_info and metadata (SQL already has both <if()> clauses); keep both template/bind branches. - studio/status_manager.py: unified update_status(status, error_info, metadata) — sends an enriched PUT via the raw client with STATUS_UPDATE_MAX_RETRIES and falls back to a typed status-only update so the run always transitions (OPIK-7159 backstop); keeps mark_error(error_info) and mark_completed(metadata). - studio/metrics.py: keep main's try/except wrapping builder errors as InvalidMetricError. - Tests: keep both the errorInfo-only updateById case (now also nulls metadata) and main's metadata-merge + typed-error tests; drop the stale cancelStudioOptimization__returnsNotImplemented (main implemented studio cancellation, covered by its new Redis-signal tests). - Generated SDK client/raw_client: expose both error_info and metadata params. - Migration stays 000107 (unique vs main's 000106s). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…args Address baz finding (rules.ts:262): the `super().__init__(...)` / base-ctor argument capture used `[^)]*`, which stops at the first `)`. A nested call before the name kwarg — e.g. `super().__init__(config=make_cfg(), name="foo")` — was truncated, so `name` wasn't found and the extractor fell back to "code", storing a wrong objective_name (getFeedbackScore misses, expectedMetricNames polls until MAX_REFETCH_TIME). Match only the ctor opening and capture the FULL balanced argument list (via a new balancedArgs helper that respects nested parens + string literals), then find the name kwarg in it. Added regression tests for a nested call before and after the name kwarg. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| depth -= 1; | ||
| if (depth === 0) return text.slice(start, i); | ||
| } | ||
| } | ||
| return text.slice(start); | ||
| }; |
There was a problem hiding this comment.
Malformed code corrupts objective_name
balancedArgs() returns text.slice(start) when the closing ) is missing, so extractNameFromClassBody() can match a later name = "..." and extractMetricNameFromCode() saves a wrong objective_name instead of null — should we signal failure there instead?
Want Baz to fix this for you? Activate Fixer You can also update your AI coding guidelines based on this comment by apply pr to [branch name]
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-frontend/src/lib/rules.ts around lines 225-230, the `balancedArgs(text,
start)` function returns `text.slice(start)` when it never finds a matching closing `)`;
instead, change it to signal failure (e.g., return null) when `depth` never reaches 0.
Then update the caller in the constructor `__init__` parsing logic (around where `const
args = balancedArgs(...)` is used to match `name = ...`) so it only runs the
`args.match(/\bname\s*=.../)` when balancedArgs succeeded, otherwise it returns
`null`/falls back safely. Finally, ensure `extractMetricNameFromPythonCode` /
`extractMetricNameFromCode` accepts only valid fully-balanced argument lists so
unterminated `super().__init(` can’t produce a partial or incorrect `objective_name`.
| // Capture the FULL (balanced) argument list so a nested call before the | ||
| // name kwarg — e.g. `super().__init__(config=make_cfg(), name="foo")` — | ||
| // doesn't get truncated at the first `)`. | ||
| const args = balancedArgs(init, ctorRegex.lastIndex); | ||
| const name = args.match(/\bname\s*=\s*["']([^"']+)["']/); |
There was a problem hiding this comment.
Wrong metric name extracted
extractMetricNameFromPythonCode() now scans the full __init__ arg list with args.match(/�name\s*=\s*["']([^"']+)["']/), so a nested name= can override the constructor's actual name and extract the wrong metric name — should we restrict the search to top-level kwargs or split the arg list first?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-frontend/src/lib/rules.ts around lines 296-300 within
`extractMetricNameFromPythonCode()`, the regex
`args.match(/\bname\s*=\s*["']([^"']+)["']/)` is too broad: it matches the first `name=`
anywhere inside the balanced `__init__` argument span, including nested calls like
`config=make_cfg(name="debug")`, which incorrectly returns `debug`. Refactor by either
splitting/parsing the top-level kwargs from the `args` string (respecting nested
parentheses and string literals) and then searching only those top-level segments for
`name=`, or enhance the scan to locate `name=` at depth 1 only (not inside nested
function calls). Ensure the function returns the constructor’s `name` kwarg from the
`super().__init__(...)` call, not from nested arguments.
| def update_status( | ||
| self, status: str, metadata: Optional[Dict[str, Any]] = None | ||
| self, | ||
| status: str, | ||
| error_info: Optional[dict] = None, | ||
| metadata: Optional[Dict[str, Any]] = None, | ||
| ) -> None: | ||
| """Update optimization status in backend. | ||
|
|
||
| Args: | ||
| status: New status ("running", "completed", "error", etc.) | ||
| error_info: Optional structured failure reason to persist alongside | ||
| an "error" status. A dict shaped like |
There was a problem hiding this comment.
Partial error_info now 400s
update_status(...) still accepts partial error_info payloads like {"message": ...} and forwards them into the PUT body, so the backend’s OptimizationUpdate.errorInfo @Valid ErrorInfo check now turns released callers into 400s. Original location to also check: sdks/python/src/opik/api_objects/optimization/optimization.py (Optimization.update() forwards error_info via extra); should we align the client shape first?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-python-backend/src/opik_backend/studio/status_manager.py around lines 58-124,
in `update_status(...)`, stop forwarding partial `error_info` to the backend. Refactor
this method to validate `error_info` against the backend’s `ErrorInfo` requirements
(at minimum `exception_type` and `traceback`); only include the `error_info` key in the
PUT body when the required fields are present, otherwise log a warning and omit
`error_info` (so the request won’t be enriched with invalid data) or fall back to the
status-only typed update. Recompute the `enriched` condition after sanitizing the body
so you don’t attempt the raw client PUT with a bad `error_info`. Also check
sdks/python/src/opik/api_objects/optimization/optimization.py where
`Optimization.update()` forwards `error_info` via `extra`, and ensure that path either
constructs a complete `error_info` or similarly drops/does not send invalid partial
payloads.
| except Exception as update_error: | ||
| logger.warning( | ||
| f"Failed to send status '{status}' with metadata for " | ||
| f"optimization {self.optimization_id} ({metadata_error}); " | ||
| f"Failed to send status '{status}' with error_info/metadata for " | ||
| f"optimization {self.optimization_id} ({update_error}); " | ||
| "retrying status-only so the run still transitions.", | ||
| ) |
There was a problem hiding this comment.
Swallows fatal update errors
except Exception as update_error swallows failures from the enriched PUT path, so we fall back to the status-only update and silently drop error_info/metadata — should we narrow this to httpx.HTTPError plus explicit non-2xx handling, let unexpected exceptions propagate, and log the warning with exc_info=True?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`apps/opik-python-backend/src/opik_backend/studio/status_manager.py` around lines
149-154 inside `OptimizationStatusManager.update_status`’s enriched PUT fallback,
replace the broad `except Exception as update_error` with an explicit exception list
covering only the expected cases (at least `httpx.HTTPError` plus the `RuntimeError` you
raise when the response has no `status_code` or is non-2xx). Let unexpected exceptions
propagate instead of being converted into a terminal-state transition, and update the
`logger.warning` call to include `exc_info=True` so the stack trace is preserved. Ensure
`httpx` is imported in this module if needed for the narrowed exception type.
thiagohora
left a comment
thiagohora
left a comment
There was a problem hiding this comment.
Opik reviewer (mined from your team's review history)
11 findings — 0 high · 3 medium · 8 low. Suppressed by team conventions: see suppressed.md.
React 👍/👎 on each comment — your feedback helps tune what it flags.
| # deterministic, explained degradation, not an accidental "healthy" score. | ||
| assert result is not None, "no result returned" | ||
| assert result.get("status") != "cancelled", "optimization was cancelled" | ||
| assert result.get("initial_score") == 0.0, ( |
There was a problem hiding this comment.
[low] missing-column test under-verifies its 'explained, not silent' contract
The docstring makes the central claim that a missing mapped column degrades to a defined, EXPLAINED per-item failure (ScoreResult(0.0, reason="Error: ...")) and 'never an unexplained/silent score'. But the assertions only check status != 'cancelled', initial_score == 0.0, and score == 0.0. A genuinely silent 0.0 regression (e.g. the mapped param resolving to None/empty and score() returning 0.0 with no error, or the per-item TypeError being swallowed with no reason) would satisfy every assertion identically. The test therefore guards against crashes and non-deterministic scores but does NOT verify the 'explained' half of its own promise, which is the behavior OPIK-7172 introduced.
💡 Assert the per-item failure reason actually surfaces — e.g. fetch the run's feedback scores / spans via opik_client and assert at least one score's reason contains 'Error:' (matching the ScoreResult(0.0, reason=f"Error: {...}") emitted in studio/metrics.py::isolated_metric), so a future silent-0.0 regression is caught rather than passing green.
general finding
| # explanatory (traceback-derived, truncated) reason. | ||
| result = metric_fn({}, "anything") | ||
| assert result.value == 0.0 | ||
| assert "Error" in result.reason |
There was a problem hiding this comment.
[low] Near-tautological assertion in runtime-error test
test_code_metric_runtime_score_error_returns_zero asserts only result.value == 0.0 and "Error" in result.reason. The error branch in isolated_metric (studio/metrics.py:906) hardcodes reason=f"Error: {error_msg[:200]}", so the substring "Error" is present on every failure regardless of what actually went wrong. The test therefore would still pass if a regression dropped the real traceback/error text (e.g. the ValueError "kaboom" from BoomMetric.score) from the reason, defeating the stated intent of verifying an 'explanatory (traceback-derived, truncated) reason'.
💡 Assert on the actual propagated error content, e.g. assert "kaboom" in result.reason (the run_user_code traceback includes ValueError: kaboom), so the test genuinely verifies the real error message reaches the ScoreResult reason rather than just the hardcoded prefix.
general finding
Address thiagohora finding (OptimizationDAO:1006): the error_info (and adjacent studio_config) deserialization caught bare Exception, masking unexpected runtime failures (e.g. NPEs) alongside the expected JSON parse error. JsonUtils.readValue throws UncheckedIOException on a malformed payload, so catch that narrowly and let real bugs surface; the row-id context in the log is preserved. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address thiagohora finding (metrics.py:894): the LLM `output` was injected into score()'s kwargs unconditionally. For a strict signature that doesn't declare an `output` param (e.g. `def score(self, reference)`), that arrives as an unexpected keyword -> TypeError -> swallowed to a masked ScoreResult(0.0) — the exact OPIK-7172 failure mode the strict-signature handling was added to remove. Inject `output` only when the signature can accept it: `accepts_var_keyword or "output" in score_params`. Added a regression test for a strict metric that does not declare `output`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address thiagohora finding (metrics.py:799): the docstring said **kwargs metrics
receive the remaining columns "minus those consumed as a rename source", but the
code splats every column (`data = {**dataset_item}`) and only overlays the
renamed param — so a rename source IS still present under its original name. Also
refreshed the `output` bullet to match the now-conditional injection (a strict
signature without an `output` param does not receive it).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…_info Address thiagohora finding (status_manager:94): message/traceback were truncated with value[:MAX], keeping the HEAD. Python tracebacks put the innermost frame and the actual raise site at the END, so for long tracebacks the persisted reason showed only outer runner frames and cut off the frames closest to the failure — undercutting OPIK-7172's whole point (surfacing the failure reason). Now keep the message head (short, meaningful at its start) but the traceback TAIL, with a "...[traceback truncated]..." marker, capped at MAX_ERROR_INFO_LENGTH. The caller's dict is copied, never mutated. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address thiagohora finding (status_manager:61): the merge inserted error_info
between status and metadata, changing the 2nd positional's meaning — a future
positional caller `update_status("completed", some_metadata)` would silently
bind some_metadata to error_info. All in-repo callers already use keywords, so
make error_info/metadata keyword-only (`*`) to remove the trap.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address thiagohora finding (status_manager:89): the error_info path had no unit
coverage (only touched indirectly by e2e). Added TestErrorInfoForwarding:
- mark_error({...}) routes through the raw client with error_info in the body;
- mark_error(None)/mark_error({}) sends NO error_info (status-only, typed path)
so it can't clobber a previously persisted reason;
- an over-length message is head-truncated and an over-length traceback keeps
its tail (innermost frame) under the cap, without mutating the caller's dict.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address thiagohora findings (process_worker _find_basemetric_classdef vs get_metric_class, metrics.py:848): - get_metric_class now restricts to classes DEFINED in the module (`cls.__module__ == module.__name__`), so an IMPORTED concrete BaseMetric subclass (e.g. `from opik.evaluation.metrics import Equals`) is never instantiated instead of the user's class just because its name sorts first (#6). - _find_basemetric_classdef now resolves INDIRECT subclasses transitively over classes defined in the file, so it selects the same alphabetically-first class runtime `issubclass` picks — no more build/runtime divergence that applies the wrong signature/name (silent 0.0 / "-") (#2). - validate_user_code no longer hard-rejects when the only BaseMetric link is through an IMPORTED base (statically unresolvable but instantiable at runtime); it defers with permissive defaults when a class is defined, and still rejects a file with no class at all (#1 regression guard). Added tests: transitive indirect-subclass selection matches runtime, imported subclass is not instantiated, imported-base defers instead of rejecting, no-class still rejected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…hains Keep the FE objective_name extractor in sync with the backend selector change: findMetricClassBody now resolves INDIRECT (transitive) BaseMetric subclasses defined in the file — collecting all class declarations, seeding from direct alias subclasses, then transitively adding classes that subclass an already-known metric class — before picking the alphabetically-first. This matches process_worker._find_basemetric_classdef / get_metric_class, so an indirect subclass that runtime instantiates yields the same objective_name here (no "-"/stalled-polling mismatch). Unresolvable (imported-base) cases still return null and defer to the backend. Added a transitive-subclass test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…uild/score test Address baz follow-ups on the review batch: - process_worker.validate_user_code: when no class statically resolves to a BaseMetric subclass but classes are defined, read name + signature flags from the alphabetically-first defined class that declares score() (mirrors runtime get_metric_class), instead of returning blanket permissive defaults. This keeps a STRICT imported-base metric from being force-splatted into a masked 0.0 and preserves its objective_name. Only when no defined class declares score() (it's inherited from the imported base) do we fall back to permissive defaults. - Strengthened tests: the imported-base case now builds via MetricFactory.build and asserts the real ScoreResult value/name (Equals-based), plus a direct validate_user_code assertion that the strict signature is read (not blanket). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address baz finding (process_worker:362): _find_basemetric_classdef and the imported-base fallback collected ClassDefs via ast.walk, which includes classes NESTED in a method/class. Those never become module attributes, so runtime get_metric_class (inspect.getmembers) never sees them — but a nested helper with a score() sorting alphabetically first could be picked at build, disagreeing with runtime. Both now use _top_level_classdefs (tree.body only), matching runtime's module-level view. Added a regression test: a nested AHelper.score() must not override the top-level metric's strict signature. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… conflict) main merged 000107_apply_traces_local_v2_real_data_codec_refinements; rename the optimizations error_info analytics migration to the next free prefix (000108) and update its changeset id to clear the migration-prefix-conflict check. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This comment was marked as low quality.
This comment was marked as low quality.
Sorry, something went wrong.
3 participants
Details
Re-exposes the custom-code metric (
METRIC_TYPE.CODE) in the v2 Optimization Studio new-run flow as a first-class, validated option, with real error surfacing. The FE plumbing for CODE already existed and had only been dropped from the dropdown; the substance here is validation, error surfacing, and a build-time false-rejection fix.compile()/instantiate without runningscore()(fixes wrongly rejecting valid metrics); rename-capableargumentsmap forscore(); persisterror_infoon the Optimization record (ClickHouse migration000103+ DAO +mark_error, with a raw-REST fallback so it works against the pinned olderopikSDK).score()-signature → arg/column mapping UI with dataset-column validation; error callout + Logs panel onERROR;kwargs.get(...)helper copy + template consolidation.error_infothrough the optimizations update + optimizer finalize paths.📄 Full write-up (research → design → review → e2e verification): https://claude.ai/code/artifact/f059581f-1c86-4a73-911b-da84fdabd637
Follow-up (not blocking): the SDK
rest_api/**was hand-edited, not fern-regenerated — runscripts/generate_openapi.shonce the model lands and bump the python-backendopikpin (the raw-REST fallback covers the interim).Change checklist
Issues
AI-WATERMARK
AI-WATERMARK: yes
Testing
opik-python-backend):pytest tests/unit—TestCodeMetriccovers syntax error, missingBaseMetric, runtime error →0.0, and rename with extra unmapped columns.opik-python-backend/tests/e2e/test_studio_optimization.py): code metric, arguments-map rename, missing-column, and syntax-error →errorsurfacing. Reproduced end-to-end against a locally-built branch backend (Docker + JDK 25, migration000103,error_infocolumn): before the fix the run hung atrunningwith no error row; after, it reacheserrorwitherror_infopersisted and the test passes.OptimizationsResourceTest.updateByIdverified locally (Tests run: 3, Failures: 0, testcontainers) and green in CI (Integration Group 15).tsc --noEmit, eslint, vitest (optimizations.test.ts57/57); drove the new-run sidebar in a browser (option appears, syntax-error blocks submit,score()→arg/column mapping lists real dataset columns, helper copy).cache_write_tokensusage-schema), Opik Optimizer E2E (No module named 'fastapi'in litellm), E2E Lib Integration (Anthropic sample-agent 500), Python SDK E2E (flake — also red on unrelated PRs).Documentation
kwargs.get(...). No external documentation changes required.