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 share refactor - ping on exp:remove, do not fail on error #9248

Merged
merged 1 commit into from
Mar 27, 2023

Conversation

skshetry
Copy link
Member

@skshetry skshetry commented Mar 27, 2023

  1. Pings Studio on exp remove
  2. Does not fail on any error during request.post
  3. UI improvement on exp remove

Closes #9227.

1) Pings Studio on exp remove
2) Does not fail on any error during request.post
3) UI improvement on exp remove
@skshetry skshetry added enhancement Enhances DVC A: experiments Related to dvc exp skip-changelog Skips changelog labels Mar 27, 2023
Comment on lines +58 to +59
# TODO: Should we use git_remote to associate with Studio project
# instead of using `git ls-remote` on fallback?
Copy link
Member Author

@skshetry skshetry Mar 27, 2023

Choose a reason for hiding this comment

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

@dberenbaum, a bit of an edge case here.
What project/repository should we associate on dvc exp push origin? The origin remote url, or where the upstream remote was set to (during git push —set-upstream)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since it's an explicit remote, I think it should be the origin remote url. If the user wants to use a different remote, then they shouldn't specify origin, right?

WRT using the upstream remote, there's some very old discussion in #6332 (comment) and #6427. I agree it would be nice to have some way to use the upstream remote, but I think it's not that simple since we aren't pushing a branch and it's not a blocker.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I meant it in terms of a Studio project. We send remote url (aka repo_url) to Studio.
At the moment, we are sending —set-upstream url instead of the remote that is passed as argument on exp-push or exp-remove.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thinking about it more, it does not make sense to pass a different repo_url if you have pushed to a separate remote. I am changing it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, makes sense, thanks @skshetry!

@skshetry skshetry merged commit 1bc4c1d into iterative:main Mar 27, 2023
@skshetry skshetry deleted the studio-exp-share-refactor branch March 27, 2023 17:06
daavoo pushed a commit that referenced this pull request Mar 28, 2023
studio experiments share refactor

1) Pings Studio on exp remove
2) Does not fail on any error during request.post
3) UI improvement on exp remove
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 enhancement Enhances DVC skip-changelog Skips changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

exp remove: ping studio to remove experiment
2 participants