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

Integrate remaining share experiment command with the extension (exp push) #3781

Merged
merged 6 commits into from
May 1, 2023

Conversation

mattseddon
Copy link
Member

@mattseddon mattseddon commented Apr 28, 2023

1/3 main <- this <- #3792 <- #3793

Relates to #3574.

This PR integrates exp push with the extension. As per this I've renamed the action from "Share" to "Push".

Demos

Share with clickable link

Screen.Recording.2023-05-01.at.10.31.31.am.mov

Error

Screen.Recording.2023-05-01.at.10.30.31.am.mov

@mattseddon mattseddon added the product PR that affects product label Apr 28, 2023
@mattseddon mattseddon self-assigned this Apr 28, 2023
@mattseddon mattseddon changed the base branch from main to fail-on-git-prompt April 28, 2023 04:53
@mattseddon mattseddon force-pushed the rename-share-command branch 2 times, most recently from b756673 to d7c343a Compare April 28, 2023 21:55
Base automatically changed from fail-on-git-prompt to main April 29, 2023 00:32
@mattseddon mattseddon force-pushed the rename-share-command branch from d7c343a to 7fa5da1 Compare April 29, 2023 00:38
@mattseddon mattseddon force-pushed the rename-share-command branch from 8a445fd to 104e653 Compare April 30, 2023 22:00
@@ -100,7 +100,6 @@ export enum RegisteredCommands {
ADD_STUDIO_ACCESS_TOKEN = 'dvc.addStudioAccessToken',
UPDATE_STUDIO_ACCESS_TOKEN = 'dvc.updateStudioAccessToken',
REMOVE_STUDIO_ACCESS_TOKEN = 'dvc.removeStudioAccessToken',
EXPERIMENT_VIEW_SHARE_TO_STUDIO = 'dvc.views.experiments.shareExperimentToStudio',
Copy link
Member Author

Choose a reason for hiding this comment

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

EXPERIMENT_VIEW_PUSH was already set above 😕

studioAccessToken
)
) {
void promptToAddStudioToken()
Copy link
Member Author

Choose a reason for hiding this comment

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

[F] This is non-blocking. User can push as many experiments as they like without setting the token.

@mattseddon mattseddon marked this pull request as ready for review May 1, 2023 01:19
@mattseddon mattseddon requested review from sroy3 and julieg18 as code owners May 1, 2023 01:19
@mattseddon mattseddon requested a review from shcheklein May 1, 2023 01:21
@skshetry
Copy link
Member

skshetry commented May 1, 2023

@mattseddon, what do you think about making "View in Studio" to be a button like in the case of error?

@mattseddon
Copy link
Member Author

@mattseddon, what do you think about making "View in Studio" to be a button like in the case of error?

In order to use a button for that link I'd have to switch APIs and send another toast message after the progress notification completes/closes. In the case of an error, the progress notification closes unexpectedly and we fallback to some existing behaviour.

Copy link
Contributor

@julieg18 julieg18 left a comment

Choose a reason for hiding this comment

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

Great work!

@dberenbaum
Copy link
Contributor

@mattseddon, what do you think about making "View in Studio" to be a button like in the case of error?

In order to use a button for that link I'd have to switch APIs and send another toast message after the progress notification completes/closes. In the case of an error, the progress notification closes unexpectedly and we fallback to some existing behaviour.

Looks great @mattseddon! Agree with @skshetry that error handling is my one question here. When the cache fails, not showing what has been successful (pushing to GitHub, pinging Studio) feels confusing. Is there anything we can do to at least show the info about what parts of the push were successful like we do in CLI?

@shcheklein
Copy link
Member

LGTM!

  • One question - do we plan to have a separate "Share" action or do we expect users to understand that Push does this - essentially solves their high level use case - to see an experiment from Studio? cc @dberenbaum (I'm bit worried that it's now too low level)

  • An experiment can be auto-pushed, right? And live-shared - does it affect this action? Probably all of this comes down to us being able to show a link / status of an experiment.

Not blockers, some followup questions to discuss.

@mattseddon
Copy link
Member Author

Looks great @mattseddon! Agree with @skshetry that error handling is my one question here. When the cache fails, not showing what has been successful (pushing to GitHub, pinging Studio) feels confusing. Is there anything we can do to at least show the info about what parts of the push were successful like we do in CLI?

Is this important? In order to rectify the error the user is still going to have to fix the problem and push again regardless of which parts are successful.

One question - do we plan to have a separate "Share" action or do we expect users to understand that Push does this - essentially solves their high level use case - to see an experiment from Studio? cc @dberenbaum (I'm bit worried that it's now too low level)

No plan right now, change is related to this comment:

@dmpetrov Made a point about distinguishing between all the different options we have in the workflow today. For the dvc exp push action, what do you think about explicitly using "push" instead of "share" for the action name? I think it is more explicit and specific, giving more clarity that it's related to git push/dvc push and separate from "live" sharing.

@codeclimate
Copy link

codeclimate bot commented May 1, 2023

Code Climate has analyzed commit 82ea04a and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 86.6% (85% is the threshold).

This pull request will bring the total coverage in the repository to 94.6% (-0.1% change).

View more on Code Climate.

@mattseddon mattseddon merged commit cf1249e into main May 1, 2023
@mattseddon mattseddon deleted the rename-share-command branch May 1, 2023 22:25
@skshetry
Copy link
Member

skshetry commented May 2, 2023

@dberenbaum, I was asking for an actionable button.

Is this important? In order to rectify the error the user is still going to have to fix the problem and push again regardless of which parts are successful.

@mattseddon, see iterative/dvc#9342. The reasoning was that:

  1. most of the metrics/plots/params are going to be git-tracked
  2. and, even if we did not notify Studio, Studio could still parse them anyway if it got updates through other means.

This will be in the next release though, I just merged the PR.

@dberenbaum
Copy link
Contributor

  • One question - do we plan to have a separate "Share" action or do we expect users to understand that Push does this - essentially solves their high level use case - to see an experiment from Studio? cc @dberenbaum (I'm bit worried that it's now too low level)

What about "Push to share"?

I thought "push" was more explicit and "share" alone may be an abstraction that doesn't tell users what's happening (and could be confused with "live sharing"), but not a blocker if you feel strongly about calling the action "share."

  • An experiment can be auto-pushed, right? And live-shared - does it affect this action? Probably all of this comes down to us being able to show a link / status of an experiment.

Auto pushing is only currently possible in very narrow scenarios. Neither auto pushing nor live sharing should affect the action. Is your question about seeing what has been shared?

@shcheklein
Copy link
Member

Is your question about seeing what has been shared?

Yes. Connection between Studio and VS Code is still not clear to my mind.

that doesn't tell users what's happening

why do you feel they need to know what is happening? Tbh I thought we should focus more on making it clear what is that they are getting as a result (vs an implementation details).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product PR that affects product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants