Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

[issue-2572] [FE] Update experiment name metadata in UI #3136

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
riturajFi wants to merge 34 commits into comet-ml:main
base: main
Choose a base branch
Loading
from riturajFi:feature/update-experiment-name-metadata

Conversation

Copy link

@riturajFi riturajFi commented Aug 29, 2025 •
edited
Loading

Details

Summary

Introduced a new API for updating an experiment’s name and metadata.

Changes

  • Added API support for updating experiment details (name & metadata).
  • Integrated the feature into the front-end.
  • Extended the Python SDK to support updates.
  • Extended the TypeScript SDK to support updates.
Screencast.from.2025年09月01日.20-23-22.webm

Change checklist

  • User facing
  • Documentation update

Issues

Testing

Documentation

riturajFi added 22 commits August 21, 2025 09:29
@riturajFi riturajFi requested a review from a team as a code owner August 29, 2025 06:24
@riturajFi riturajFi marked this pull request as draft August 29, 2025 06:29
Copy link
Author

Hi @yaricom @Lothiraldan @dsblank @vincentkoc , would like this PR to be reviewd

Copy link
Member

@riturajFi can you please address the linter and test failures please before team can review. Thanks for your contribution.

@vincentkoc vincentkoc changed the title (削除) Feature/update experiment name metadata (削除ここまで) (追記) [issue-2572] [FE] Update experiment name metadata in UI (追記ここまで) Sep 2, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces functionality to update experiment names and metadata across all Opik applications. The feature enables users to modify existing experiments through API endpoints, frontend UI, and SDK methods.

Key changes:

  • Added API endpoint for updating experiment name and metadata with validation and change detection
  • Created interactive frontend dialog with Monaco editor for JSON metadata editing
  • Extended both Python and TypeScript SDKs with update methods

Reviewed Changes

Copilot reviewed 20 out of 21 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/priv/ExperimentsResource.java REST endpoint for experiment updates with change detection
apps/opik-backend/src/main/java/com/comet/opik/domain/ExperimentService.java Business logic for planning and applying experiment updates
apps/opik-backend/src/main/java/com/comet/opik/domain/ExperimentDAO.java Database operations for updating experiment name and metadata
apps/opik-frontend/src/components/shared/UpdateExperimentDialog/UpdateExperimentDialog.tsx React dialog component with Monaco editor for experiment editing
apps/opik-frontend/src/api/datasets/useExperimentUpdate.ts Frontend mutation hook for experiment updates
sdks/python/src/opik/api_objects/opik_client.py Python SDK method for updating experiments
sdks/typescript/src/opik/client/Client.ts TypeScript SDK method for updating experiments
sdks/typescript/tests/experiment/client-experiment.test.ts Unit and integration tests for TypeScript SDK
Files not reviewed (1)
  • apps/opik-frontend/package-lock.json: Language not supported

datasetName: "test-dataset",
});

console.log(JSON.stringify(created))
Copy link

Copilot AI Sep 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug console.log statement should be removed from test code. Use proper test logging or assertions instead.

Suggested change
console.log(JSON.stringify(created))

Copilot uses AI. Check for mistakes.

try {
parsedMetadata = metadata ? JSON.parse(metadata) : {};
} catch (e) {
alert("Invalid JSON in metadata");
Copy link

Copilot AI Sep 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using browser alert() for error messages is not consistent with the application's UI patterns. Use the toast notification system instead, similar to other components in the codebase.

Copilot uses AI. Check for mistakes.

Comment on lines +15 to +21
export function UpdateExperimentDialog({
open,
setOpen,
onConfirm,
latestName,
latestMetadata
}) {
Copy link

Copilot AI Sep 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function parameters lack TypeScript type definitions. Define an interface for the props to ensure type safety and better developer experience.

Suggested change
export function UpdateExperimentDialog({
open,
setOpen,
onConfirm,
latestName,
latestMetadata
}) {
interface UpdateExperimentDialogProps {
open: boolean;
setOpen: (open: boolean) => void;
onConfirm: () => void;
latestName: string;
latestMetadata: any;
}
export function UpdateExperimentDialog({
open,
setOpen,
onConfirm,
latestName,
latestMetadata
}: UpdateExperimentDialogProps) {

Copilot uses AI. Check for mistakes.

-------------------
id: str
newName: Optional[str]
newMetadata: Optional[str]
Copy link

Copilot AI Sep 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docstring parameter type annotation is incorrect. newMetadata should be Optional[Dict[str, Any]] not Optional[str] to match the actual parameter type.

Suggested change
newMetadata: Optional[str]
newMetadata: Optional[Dict[str, Any]]

Copilot uses AI. Check for mistakes.

title="Delete experiment"
description="Deleting an experiment will remove all samples in the experiment. Related traces won’t be affected. This action can’t be undone. Are you sure you want to continue?"
confirmText="Delete experiment"
confirmText="Delete experiments"
Copy link

Copilot AI Sep 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The confirm text should be "Delete experiment" (singular) since the action deletes only one experiment, not multiple experiments.

Suggested change
confirmText="Delete experiments"
confirmText="Delete experiment"

Copilot uses AI. Check for mistakes.

Comment on lines 529 to 537
ObjectMapper mapper = new ObjectMapper();
JsonNode meta;

try {
meta = mapper.readTree(plan.targetMetadata());
} catch (JsonProcessingException e) {
log.error("Invalid metadata JSON stored for experiment {}: {}", id, plan.targetMetadata(), e);
meta = mapper.createObjectNode().put("raw", plan.targetMetadata());
}
Copy link

Copilot AI Sep 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent indentation after the blocking call. The ObjectMapper instantiation should be aligned with the surrounding code block.

Suggested change
ObjectMapper mapper = new ObjectMapper();
JsonNode meta;
try {
meta = mapper.readTree(plan.targetMetadata());
} catch (JsonProcessingException e) {
log.error("Invalid metadata JSON stored for experiment {}: {}", id, plan.targetMetadata(), e);
meta = mapper.createObjectNode().put("raw", plan.targetMetadata());
}
ObjectMapper mapper = new ObjectMapper();
JsonNode meta;
try {
meta = mapper.readTree(plan.targetMetadata());
} catch (JsonProcessingException e) {
log.error("Invalid metadata JSON stored for experiment {}: {}", id, plan.targetMetadata(), e);
meta = mapper.createObjectNode().put("raw", plan.targetMetadata());
}

Copilot uses AI. Check for mistakes.

Copy link
Member

@andrescrz andrescrz left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @riturajFi

Thanks a lot for your contribution.

I left a review on the backend part, with the general guidelines to follow in this service.

The main question here is how to handle the ClickHouse updates, which isn't straight forward and probably one of the reasons why we haven't implemented this ourselves yet.

I'll discuss and double check the approach with the team for that, but for now, the best option is to handle the update with a INSERT + SELECT query (see examples in this DAO already) and document that the endpoint is only meant for the UI and doesn't handle concurrent scenarios for the SDK (already the case).

Other members of the team will review the FE and SDK parts. In the meantime, please remove changes on the auto-generated code.

Thank you very much for all this work!

Comment on lines +611 to +620
private static final String UPDATE_BY_ID = """
ALTER TABLE experiments
UPDATE
name = :name,
metadata = :metadata,
last_updated_by = :last_updated_by
WHERE id = :id
AND workspace_id = :workspace_id
;
""";
Copy link
Member

@andrescrz andrescrz Sep 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per architectural reasons, we don't do mutations in ClickHouse, as they're not performant:

Regarding entities stored in CH, only spans and traces are PATCHED. The approach is to query and re-insert only the data that changes, in this case it'd be name and metadata. In this approach is tricky to keep data consistency for concurrent SDK flows. But maybe is reasonable to just document that this endpoint is for UI purposes only.

Another alternative is lightweight updates:

But considering that is a Beta feature, has some performance implications, and it'd have the same concurrency issues, I'd go with the previous approach.

return Response.ok(feedbackScoreNames).build();
}

@POST
Copy link
Member

@andrescrz andrescrz Sep 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a PATCH operation.

Copy link
Member

@andrescrz andrescrz Sep 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need tests for this new endpoint. Please follow the conventions in the code base.

}

@POST
@Path("/update/{id}")
Copy link
Member

@andrescrz andrescrz Sep 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After updating to PATCH, the verb should be removed from the path. It isn't a good REST practice.


@POST
@Path("/update/{id}")
public Response updateExperimentById(
Copy link
Member

@andrescrz andrescrz Sep 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: let's rename the method to update.

@@ -0,0 +1,13 @@
package com.comet.opik.domain;
Copy link
Member

@andrescrz andrescrz Sep 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move to package com.comet.opik.api;

import com.fasterxml.jackson.databind.annotation.JsonNaming;

@JsonIgnoreProperties(ignoreUnknown = true)
@JsonNaming(PropertyNamingStrategies.SnakeCaseStrategy.class)
Copy link
Member

@andrescrz andrescrz Sep 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add @Builder(toBuilder = true).


import io.reactivex.rxjava3.annotations.Nullable;

public record ExperimentUpdatePlan(
Copy link
Member

@andrescrz andrescrz Sep 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't be needed.

Copy link
Member

@andrescrz andrescrz Sep 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have an automation for everything under sdks/python/src/opik/rest_api/ folder, please remove these changes. They'll be auto-generated.

Copy link
Member

@andrescrz andrescrz Sep 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for the typescript SDK, we have an automation for everything under rest_api folder, please remove these changes. They'll be auto-generated.

Copy link
Author

Great insights @andrescrz ! Will do the changes right away!
andrescrz reacted with heart emoji

Copy link
Contributor

@andriidudar andriidudar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for your contribution. I have provided a few comments regarding the frontend implementation for your consideration.

"@dnd-kit/sortable": "^8.0.0",
"@hookform/resolvers": "^3.10.0",
"@lezer/highlight": "^1.2.1",
"@monaco-editor/react": "^4.7.0",
Copy link
Contributor

@andriidudar andriidudar Sep 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not introduce this very heavy dependency, we already have the editor we use in other places, Codemirror.

import { useToast } from "@/components/ui/use-toast";
import api, { EXPERIMENTS_REST_ENDPOINT } from "@/api/api";

type UseExperimentUpdate = {
Copy link
Contributor

@andriidudar andriidudar Sep 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great if you could align this with the existing naming conventions and parameter structures we use for other update endpoints. For reference, you can check [apps/opik-frontend/src/api/projects/useProjectUpdateMutation.ts]. This will help keep things consistent, especially once the backend is migrated to use a PATCH request.

const experiment = context.row.original;
const [open, setOpen] = useState<boolean>(false);

const[openEditPanel,setOpenEditPanel]=useState<boolean>(false);
Copy link
Contributor

@andriidudar andriidudar Sep 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’d suggest not introducing new state here. It would be better to follow the existing pattern we use elsewhere—for example, in [apps/opik-frontend/src/components/pages/ProjectsPage/ProjectRowActionsCell.tsx]. This will help keep the implementation consistent across the codebase.

}}
>
<Pen className="mr-2 size-4" />
Update
Copy link
Contributor

@andriidudar andriidudar Sep 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please update the wording here to use "Edit", so it stays consistent with the rest of the codebase?

@@ -0,0 +1,121 @@
import {
Copy link
Contributor

@andriidudar andriidudar Sep 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned above regarding the code editor, I’d recommend using this file as a reference for integrating the CodeMirror editor:
[apps/opik-frontend/src/components/pages/DatasetItemsPage/AddEditDatasetItemDialog.tsx].

</DialogHeader>
<DialogAutoScrollBody className="space-y-4">
{/* Name Input */}
<div className="flex flex-col gap-2 pb-4">
Copy link
Contributor

@andriidudar andriidudar Sep 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let’s reuse the existing input component from [apps/opik-frontend/src/components/ui/input.tsx] to keep things consistent across the codebase.

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is being reviewed by Cursor Bugbot

Details

Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.

To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.

environments.OpikApiEnvironment.Default,
`v1/private/experiments/update/${id}`,
),
method: "POST",
Copy link

@cursor cursor bot Sep 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: API Endpoint Mismatch Causes Client Errors

The frontend, Python, and TypeScript clients for updating experiments have an API endpoint mismatch with the backend. All clients incorrectly include an /update segment in the URL path. Additionally, the Python and TypeScript SDKs use a POST method instead of the expected PATCH. This results in 404 errors.

Additional Locations (2)

Fix in Cursor Fix in Web

expect(updated.name).toBe("updated-exp");
//expect(updated.metadata).toEqual({ k: "v", num: 1 });
});
});
Copy link

@cursor cursor bot Sep 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Test Code Issues: Debugging, Hardcoded Values, Localhost URL

The updateExperiment integration test appears to be debugging code. It includes a console.log, hardcoded experiment IDs, a localhost API URL, and commented-out assertions. This setup will likely fail in CI/CD or other test environments.

Fix in Cursor Fix in Web


return useMutation({
mutationFn: async ({ id, name, metadata }: UseExperimentUpdate) => {
const { data } = await api.patch(`${EXPERIMENTS_REST_ENDPOINT}update/${id}`, {
Copy link

@cursor cursor bot Sep 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Experiment Update Endpoint URL Mismatch

The useExperimentUpdate mutation sends PATCH requests to /v1/private/experiments/update/${id}. The backend's corresponding update endpoint expects /v1/private/experiments/{id}, without the /update/ segment. This URL mismatch results in 404 errors when updating experiments.

Fix in Cursor Fix in Web

Copy link
Member

Hi @riturajFi

We've just added a new experiment update API endpoint which supports your use cases, see: #3263

You don't need to implement that part anymore. You can't simplify this PR and simply use it from the FE and SDK changes.

Feel free to rebase the latest changes from main branch, resolve the conflicts and resume this implementation.

Let me know if any question.
Regards,
Andrés

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

Reviewers

@andrescrz andrescrz andrescrz left review comments

Copilot code review Copilot Copilot left review comments

@cursor cursor[bot] cursor[bot] left review comments

@andriidudar andriidudar andriidudar requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[FR]: Add support for updating experiments

AltStyle によって変換されたページ (->オリジナル) /