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

Review the new DVC sharing experiments workflow and unify it with VS Code #3574

Closed
8 tasks done
shcheklein opened this issue Mar 28, 2023 · 65 comments · Fixed by #4600
Closed
8 tasks done

Review the new DVC sharing experiments workflow and unify it with VS Code #3574

shcheklein opened this issue Mar 28, 2023 · 65 comments · Fixed by #4600
Assignees
Labels
A: experiments Area: experiments table webview and everything related A: integration Area: DVC integration layer priority-p1 Regular product backlog

Comments

@shcheklein
Copy link
Member

shcheklein commented Mar 28, 2023

With iterative/dvc#9074 and https://github.com/iterative/studio/issues/5050 DVC introduced a way to connect to Studio and a workflow that sends experiments to Studio.

We need to review and unify/make it compatible:

@shcheklein shcheklein added priority-p1 Regular product backlog A: experiments Area: experiments table webview and everything related A: integration Area: DVC integration layer 📦 product Needs product input or is being actively worked on labels Mar 28, 2023
@dberenbaum
Copy link
Contributor

Can we detect that it's already being shared? Show a link?

We could fetch from the git remote or use dvc exp list to see what's been pushed. Do we need anything specific to Studio?

I'm a bit behind on the Studio efforts to have more shareable links, but I think we can follow it in https://github.com/iterative/studio/issues/5419 and https://github.com/iterative/studio/issues/5150.

  • Auto-sharing mode - does it contradict or not with existing DVC workflow?

I guess it means this:

Screenshot 2023-03-29 at 3 42 58 PM

I don't think it contradicts. Does it do anything besides add the STUDIO_TOKEN to experiment actions @mattseddon? I think it's needed since there's no other obvious way to enable this behavior when working in the extension.

  • Do we still need to use our own client implementation or not?

I hope not.

@dberenbaum
Copy link
Contributor

  • Sharing experiments. Can we detect that it's already being shared? Show a link?

I don't know if we have a good way to detect what experiments have been live shared. For exp refs, we could use dvc exp list to check the git remote. If they exist there, there should be a record in Studio, although it's possible it hasn't been retrieved/parsed yet by Studio.

I would add an item to consolidate and unify the sharing options. Here's what I see today:

Screenshot 2023-04-12 at 2 16 34 PM

Overall, I think we can have a single "Share" action that relies on dvc exp push now and drop the existing different ways to share. This will hopefully make sharing more straightforward.

Now Studio should show any experiments that are pushed to the Git remote via dvc exp push, so we shouldn't need to rely on the live metrics API anymore. dvc exp push will notify Studio if STUDIO_TOKEN is set or if the token is available in studio.token in the DVC config (depends on iterative/dvc#9257; we will likely recommend using dvc config --global studio.token xxx for CLI users).

dvc exp push will push the cache by default. Not sure if we need an option to disable that, but we should at least be aware that it could make pushing slow.

@shcheklein
Copy link
Member Author

Sharing experiments. Can we detect that it's already being shared? Show a link?

I think DVCLive creates some special signal file with a process pid? Can we utilize it cc @mattseddon @daavoo .

On the "share" actions. I agree, @dberenbaum we can add item to the list to consolidate this and simplify. We still keep an apply action though, right? So that I can make locally a commit out of an experiment at least. We just don't send these commits, neither we try to do dvc push, etc.

@dberenbaum
Copy link
Contributor

We still keep an apply action though, right? So that I can make locally a commit out of an experiment at least. We just don't send these commits, neither we try to do dvc push, etc.

Agreed, we can keep local actions like "Apply to workspace" and "Create new branch," I just don't think we need to combine them with the sharing action.

@mattseddon
Copy link
Member

I would add an item to consolidate and unify the sharing options. Here's what I see today:

Screenshot 2023-04-12 at 2 16 34 PM

Overall, I think we can have a single "Share" action that relies on dvc exp push now and drop the existing different ways to share. This will hopefully make sharing more straightforward.

Now Studio should show any experiments that are pushed to the Git remote via dvc exp push, so we shouldn't need to rely on the live metrics API anymore. dvc exp push will notify Studio if STUDIO_TOKEN is set or if the token is available in studio.token in the DVC config (depends on iterative/dvc#9257; we will likely recommend using dvc config --global studio.token xxx for CLI users).

I'm happy to drop Commit and Share + Branch and Share and update Share to Studio to use exp push and label it with simply Share. My one question would be "how do we promote Studio as a sharing option"? Currently, with the custom implementation of the Studio client and the command's label, we can do a few things:

  1. Promote Studio as a sharing option.
  2. We can check to see if there is a Studio token set for the extension and redirect to the setup page if there isn't one present.
  3. If we receive a 401 we can notify the user that they need to update their token.

Should we drop this logic altogether and not worry about the edge cases?
How do we show users that sharing to Studio is an option?

It feels like we need an API that we can use at startup to check whether or not the user has a valid token and if not prompt them to add one/update. Otherwise, we can't provide much help to users once they leave the happy path.

dvc exp push will push the cache by default. Not sure if we need an option to disable that, but we should at least be aware that it could make pushing slow.

From testing out with the demo project moving from the old API to using exp push blew the time out to share an experiment from 1s to ~90s, after the initial push this dropped to 14s. It would be good to be able to reduce that time in any way possible.

@mattseddon
Copy link
Member

FWIW I also ran into the need to authenticate the process running DVC with Github today. I need to work out how to get access to the credentials using the VS Code environment.

@dberenbaum
Copy link
Contributor

I'm happy to drop Commit and Share + Branch and Share and update Share to Studio to use exp push and label it with simply Share. My one question would be "how do we promote Studio as a sharing option"? Currently, with the custom implementation of the Studio client and the command's label, we can do a few things:

Sorry, I'm getting confused by all the different ways to share. Do you mean how do we promote live sharing to Studio, or any kind of sharing to Studio?

It feels like we need an API that we can use at startup to check whether or not the user has a valid token and if not prompt them to add one/update.

Not sure I follow all the suggestions, but can we start simpler by checking whether the user has a token at all and assume it's valid if it exists? I think checking the token validity is less of a priority for now.

From testing out with the demo project moving from the old API to using exp push blew the time out to share an experiment from 1s to ~90s, after the initial push this dropped to 14s.

Are you testing with checkpoints?

@mattseddon
Copy link
Member

mattseddon commented Apr 18, 2023

Sorry, I'm getting confused by all the different ways to share. Do you mean how do we promote live sharing to Studio, or any kind of sharing to Studio?

Any kind of sharing to Studio. How do we highlight this as a collaboration option?

Note: I can see from iterative/dvc.org#4470 that if a user has a token present in their config then they can live share experiments but not sure what the default behaviour is.

Not sure I follow all the suggestions, but can we start simpler by checking whether the user has a token at all and assume it's valid if it exists? I think checking the token validity is less of a priority for now.

I think we need a Studio API that we can check auth details against but this should be fixed/made obsolete by https://github.com/iterative/studio/issues/5158.

A couple of follow-up questions:

  • Can I use dvc config studio.token to check if a user has a Studio token in their config? (if yes I can use it as an environment variable if "live sharing" is enabled)
  • When setting the token into a user's config should I set it as --local, --global or --system? (my guess is --global)

If users have already set this up I think we should still prompt them to add again as this makes it explicit that we are saving it in their DVC config (rather than expending effort migrating the secret).

Are you testing with checkpoints?

In this instance no, checkpoints were dropped on the branch that I was testing with.

@dberenbaum
Copy link
Contributor

Thanks @mattseddon! A couple of not directly related thoughts before responding to your questions/comments.

Looking through the Studio onboarding from VS Code, do we need the "sign in" button? The "get token" button will take them to the same place if not signed in.

To make sharing more useful, can we also include an action to share after selecting multiple experiments (it looks like you can only share one at a time now)?

Any kind of sharing to Studio. How do we highlight this as a collaboration option?

When choosing to share, we could prompt them with something like Connect to Studio to see and manage shared experiments if they aren't connected already (with an option to stop showing the prompt). Open to other ideas here.

  • Can I use dvc config studio.token to check if a user has a Studio token in their config? (if yes I can use it as an environment variable if "live sharing" is enabled)

Yes and yes.

  • When setting the token into a user's config should I set it as --local, --global or --system? (my guess is --global)

Yes --global sounds good to me.

If users have already set this up I think we should still prompt them to add again as this makes it explicit that we are saving it in their DVC config (rather than expending effort migrating the secret).

Sounds good.

From testing out with the demo project moving from the old API to using exp push blew the time out to share an experiment from 1s to ~90s, after the initial push this dropped to 14s. It would be good to be able to reduce that time in any way possible.

We probably need an option to share with/without the cache, whether that is two separate sharing actions or whether there's some other way (prompt, settings option, etc.).

@mattseddon
Copy link
Member

Looking through the Studio onboarding from VS Code, do we need the "sign in" button? The "get token" button will take them to the same place if not signed in.

We can do this. I was just trying to give a linear/defined process for the user to follow. We can drop the sign-in button. The user may just have to use some brain power.

To make sharing more useful, can we also include an action to share after selecting multiple experiments (it looks like you can only share one at a time now)?

This is correct. I will enable it through the multi-select context menu. When using exp push with multiple experiments is push parallelised? If not I will push individually and update the progress notification each time an experiment is pushed.

When choosing to share, we could prompt them with something like Connect to Studio to see and manage shared experiments if they aren't connected already (with an option to stop showing the prompt). Open to other ideas here.

Sounds reasonable. I'll add buttons to a toast that let the user:

  1. open the setup page with the studio section focused
  2. sets a config option that means they won't be asked again

We probably need an option to share with/without the cache, whether that is two separate sharing actions or whether there's some other way (prompt, settings option, etc.).

After considering the implications a bit closer I think we should stick with the default, to begin with.

I'll update the description. Please LMK if I have missed anything 🙏🏻.

@dberenbaum
Copy link
Contributor

Drop the sign-in button from Studio setup page

IMO it's simpler with fewer buttons, but Studio won't redirect users to the token if they aren't already logged in, so it could also cause confusion. Up to you what to do.

When using exp push with multiple experiments is push parallelised? If not I will push individually and update the progress notification each time an experiment is pushed.

Yes, at least I think there's some optimization when pushing the cache for multiple experiments, so I would suggest doing it that way. The final output should have the info you want (what was pushed to both git and the cache), but it won't be streamed with each individual experiment, and there's currently no machine-readable output format.

We probably need an option to share with/without the cache, whether that is two separate sharing actions or whether there's some other way (prompt, settings option, etc.).

After considering the implications a bit closer I think we should stick with the default, to begin with.

Okay. Would like to hear more of your thoughts on this since I agree it will be slow and maybe not what people want when they are only interested in things like metrics and plots.

@mattseddon
Copy link
Member

Drop the sign-in button from Studio setup page

IMO it's simpler with fewer buttons, but Studio won't redirect users to the token if they aren't already logged in, so it could also cause confusion. Up to you what to do.

Should be fixed by https://github.com/iterative/studio/issues/5715. Maybe we can wait for that.

After considering the implications a bit closer I think we should stick with the default, to begin with.

Okay. Would like to hear more of your thoughts on this since I agree it will be slow and maybe not what people want when they are only interested in things like metrics and plots.

I was thinking about this from a pessimistic/"what could go wrong" with someone's workflow point of view. If they push the cache by default then at least everything will be re-creatable/shared and there shouldn't be any surprises later on. IIUC this helps to solve the "I forgot the push" issue.

@dberenbaum
Copy link
Contributor

If they push the cache by default then at least everything will be re-creatable/shared and there shouldn't be any surprises later on. IIUC this helps to solve the "I forgot the push" issue.

When choosing to share, we could prompt them with something like Connect to Studio to see and manage shared experiments if they aren't connected already (with an option to stop showing the prompt). Open to other ideas here.

If we are going to have a prompt when sharing, we could include a checkbox for whether to share the cache, which could be checked by default.

@mattseddon
Copy link
Member

If we are going to have a prompt when sharing, we could include a checkbox for whether to share the cache, which could be checked by default.

This would be on the settings page, right? We could start with it in the "Studio" section but I think we would want to move it to the new "Remote" section once we get to it. Maybe we can delay until that is added? Up to you.

@dberenbaum
Copy link
Contributor

This would be on the settings page, right?

I was thinking it could be part of this:

Sounds reasonable. I'll add buttons to a toast that let the user:

1. open the setup page with the studio section focused

2. sets a config option that means they won't be asked again

Maybe we can delay until that is added?

Yup, we can start without it.

@dberenbaum
Copy link
Contributor

Another small item to add: When running a notebook or other no-pipeline workflow, the checbox for sharing new experiments live won't do anything. We could show instructions like "For experiments run outside of this extension, set the DVC_STUDIO_TOKEN environment variable to share experiments live."

@mattseddon mattseddon assigned mattseddon and unassigned dberenbaum Apr 26, 2023
@mattseddon
Copy link
Member

Can we go back to a spinner or something similar while the push is happening?

Yes, we can do that.

Can we coordinate on the iconography with https://github.com/iterative/studio/issues/6870? I think the cloud is a good idea since dvc exp push by default pushes to both the git and dvc remotes, but would be good to make sure we have some consistency.

Yes, I will keep an eye on that. For native icons choices are limited to these ones. Maybe in Studio, we could show a cloud with a slash through it to signify that it is not uploaded (pushed) yet.

Will it sync eventually, like when someone pushes another experiment using the extension?

Currently, it will sync as soon as there is a change in the workspace. This falls down when dvc exp remove -g origin <exp-name> is run from the terminal (no update is shown in the table).

Thanks @mattseddon for the quick iteration! Should we already introduce a separate column for these states? Where will we put a link to Studio for example?

I considered this and decided that because we are always fighting for space this would be the best option. As there is nothing to show for commits we would be wasting space there as well.
The Studio link can replace the cloud icon for completed experiments (under the correct circumstances). For running experiments get Studio link can appear in the context menu. WDYT?

@shcheklein
Copy link
Member Author

The Studio link can replace the cloud icon for completed experiments (under the correct circumstances). For running experiments get Studio link can appear in the context menu. WDYT?

Hmm, I don't see how that could happen to be honest.

I would as usual err on the side of being super obvious and look decent vs optimizing things to look good. E.g. if it means three columns with distinct obvious names - I would even go for that. At least initially.

For running experiments get Studio link can appear in the context menu. WDYT?

My take - that would be confusing - when fields change they semantics.

As there is nothing to show for commits we would be wasting space there as well.

They also could show Studio link, GH link, their status (we could check in background data eventually).

@mattseddon
Copy link
Member

Hmm, I don't see how that could happen to be honest.

Link icon:

image

@shcheklein
Copy link
Member Author

My take - that's too crowded, also we miss making it explicit (e.g. Studio column name would make the purpose clear).

@mattseddon
Copy link
Member

@dberenbaum @shcheklein is this the correct view/state that we want a shareable link to point to:

image

@shcheklein
Copy link
Member Author

I don't think we need it to be selected (checkbox). I think it's better to open plots in this case tbh (since we don't have any other way to highlight the row when it open on the FE, or do we @iterative/studio-frontend ?)

Checkbox enables these actions at the top? (and hides the Plots button, etc). May be it's even fine to keep checkbox selected, but can we then show more actions?

some ideas, Matt, I think it's totally fine as a first iteration! thanks 🙏

@mvshmakov
Copy link

I think it's better to open plots in this case tbh

👍🏻

We've recently split the selected elements from active ones. Selected elements correspond to the checkboxes, and activated ones open the plots. Both can be persisted in the URL.

Checkbox enables these actions at the top?

Yes, they do.

May be it's even fine to keep checkbox selected, but can we then show more actions?

Not really, there is not enough real estate in the row. We'll have to squash them aggressively.

If that is acceptable, I'd prefer sticking to the activated rows.

@mattseddon
Copy link
Member

mattseddon commented Aug 17, 2023

A couple of questions from me:

  • Should live experiments have plots that are visible in the Studio UI?
  • Do all views/projects have plots? If not do we know the percentage that don't have plots?

@shcheklein
Copy link
Member Author

Should live experiments have plots that are visible in the Studio UI?

Yes, in most cases.

Do all views/projects have plots? If not do we know the percentage that don't have plots?

No, some projects don't and it's a good point. Means that it's better to show the table. Also show the Plots button (so, no checkbox?).

If that is acceptable, I'd prefer sticking to the activated rows.

what are the activated rows?

@mvshmakov
Copy link

what are the activated rows?

Activated row (plot button)

Screenshot 2023-08-18 at 13 58 15

Selected row (checkbox)

Screenshot 2023-08-18 at 13 58 37

Activated AND selected row (plot button AND checkbox)

Screenshot 2023-08-18 at 13 58 49

@shcheklein
Copy link
Member Author

Okay, I guess let's do checkbox and if there are plots we can do a leap of faith and show them I guess. It's a bit suboptimal tbh. Checkbox are not meant for this use case (they are about making actions on an experiment). We have a way to highlight an experiment (for a second at the moment). I wonder if can / should utilize that instead?

@mattseddon
Copy link
Member

Okay, I guess let's do checkbox and if there are plots we can do a leap of faith and show them I guess. It's a bit suboptimal tbh. Checkbox are not meant for this use case (they are about making actions on an experiment). We have a way to highlight an experiment (for a second at the moment). I wonder if can / should utilize that instead?

Coming back to the original suggestion question quickly:

image

The reason for using the checkbox here was to apply the show only selected commits filter:

image

This means we can share a URL of the form:

https://studio.iterative.ai/user/{user_id}/projects/{view_id}?showOnlySelected=1&experimentReferences={exp_ref}&activeExperimentReferences={exp_ref}%3Aprimary

Whoever receives the link will be able to:

  1. Compare the experiment with the baseline.
  2. Easily view plots if they exist.

I suggested this view because even though it is not optimal I felt that it provides value whilst also eliminating edge cases (e.g. no plots).

I should have explained this, to begin with.

@mattseddon
Copy link
Member

Demo of shareable links for completed/pushed experiments (#4557)

Screen.Recording.2023-08-23.at.11.11.13.am.mov

(this has been a long time coming)

@mattseddon
Copy link
Member

Getting closer to finishing this now with https://github.com/iterative/studio/pull/7332 ready for review and a rough prototype done on the extension side.

I have encountered an issue with the proposed links getting bounced if no plots are available for a record. For this reason, I'm going to resort to the following links:

${this.baseUrl}?showOnlySelected=1&experimentReferences=${sha} for completed & pushed experiments
${this.baseUrl}?showOnlySelected=1&liveExperiments=${baselineSha}%3A${name} for live experiments

Copy Studio Link will be available for as a context menu option for all experiments that exist on the user's instance of Studio.

LMK if you have questions regarding any of the above.

🙏🏻

@jellebouwman
Copy link

LMK if you have questions regarding any of the above.

I have encountered an issue with the proposed links getting bounced if no plots are available for a record. For this reason, I'm going to resort to the following links:

Thanks for the writeup @mattseddon! Does this mean a link like https://studio.iterative.ai/user/jellebouwman/projects/demo-bank-customer-churn-esgpd8e876?showOnlySelected=1&panels=plots%2C&commits=67521d6d2acc16218e4cf6378b73732b14754140&activeCommits=67521d6d2acc16218e4cf6378b73732b14754140%3Aprimary (note activeCommits) breaks the URL for you?

IMO - we should try to let you trigger the plots for a rev and should fail gracefully (keep the rest of the ProjectURL working) if the rev doesn't have plots. Let me know if that's the case right now, and I can try to work on a fix for you.

@mattseddon
Copy link
Member

Final demo for this one.

Shareable links are provided for all experiments found on Studio (live or pushed) via the experiments table context menu:

Screen.Recording.2023-09-05.at.8.14.11.am.mov

@shcheklein
Copy link
Member Author

Thanks, Matt. Looks great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: experiments Area: experiments table webview and everything related A: integration Area: DVC integration layer priority-p1 Regular product backlog
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants