Skip to content
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

exp remove: ping studio to remove experiment #9227

Closed
Tracked by #9074
dberenbaum opened this issue Mar 22, 2023 · 6 comments · Fixed by #9248
Closed
Tracked by #9074

exp remove: ping studio to remove experiment #9227

dberenbaum opened this issue Mar 22, 2023 · 6 comments · Fixed by #9248
Labels
A: experiments Related to dvc exp p2-medium Medium priority, should be done, but less important product: VSCode Integration with VSCode extension

Comments

@dberenbaum
Copy link
Collaborator

dvc exp remove -g should also ping Studio to drop the experiment from the UI. Currently, there is no way to clean up experiments once they have been added in Studio.

@dberenbaum dberenbaum added p1-important Important, aka current backlog of things to do product: VSCode Integration with VSCode extension A: experiments Related to dvc exp labels Mar 22, 2023
@skshetry
Copy link
Member

I wonder how Studio removes existing commits. It’s probably tied to branch lifecycle. cc @amritghimire

Anyway, I don’t think we should eagerly remove from CLI. It’s better to remove it from the web UI or automatically on the next parse.

@amritghimire
Copy link
Contributor

It will be marked as deleted commit in next parse. We can use the same webhook trigger on delete to start the parse.

@dberenbaum
Copy link
Collaborator Author

Here's what it looks like when a branch is deleted in Studio (the scatter-jitter branch in this case):

Screen.Recording.2023-03-23.at.11.14.52.AM.mov

@skshetry I agree that removing from the web UI would be better, but we still need to address what should happen when using dvc exp remove -g. It could look similar to this branch delete workflow where Studio shows that there is an update and removes the experiment when the user clicks to update or reloads.

@skshetry
Copy link
Member

As long as it gets eventually removed, I'd prefer to focus on other parts of the integration. If Studio does not do that yet, it should do that during parse automatically.

I think for now, we should think of extending the payload to {added: list[str], removed: list[str]} to be safe. But we are still discussing changing the API that I don't want to propose this right away. It's something that we should keep in mind though.

For the release on the DVC side, I don't think this is urgent or P1. We can do it later as long as it gets removed eventually.

@dberenbaum
Copy link
Collaborator Author

Sounds good @skshetry, I agree we can make it a follow-up once we have solidified, so I can move this down to p2 for now.

@dberenbaum dberenbaum added p2-medium Medium priority, should be done, but less important and removed p1-important Important, aka current backlog of things to do labels Mar 24, 2023
@skshetry
Copy link
Member

Closed by #9248.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: experiments Related to dvc exp p2-medium Medium priority, should be done, but less important product: VSCode Integration with VSCode extension
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants