-
Notifications
You must be signed in to change notification settings - Fork 29
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
Comments
We could fetch from the git remote or use 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.
I guess it means this: I don't think it contradicts. Does it do anything besides add the
I hope not. |
I don't know if we have a good way to detect what experiments have been live shared. For exp refs, we could use I would add an item to consolidate and unify the sharing options. Here's what I see today: Overall, I think we can have a single "Share" action that relies on Now Studio should show any experiments that are pushed to the Git remote via
|
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 |
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. |
I'm happy to drop
Should we drop this logic altogether and not worry about the edge cases? 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.
From testing out with the demo project moving from the old API to using |
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. |
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?
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.
Are you testing with checkpoints? |
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.
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:
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).
In this instance no, checkpoints were dropped on the branch that I was testing with. |
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)?
When choosing to share, we could prompt them with something like
Yes and yes.
Yes
Sounds good.
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.). |
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.
This is correct. I will enable it through the multi-select context menu. When using
Sounds reasonable. I'll add buttons to a toast that let the user:
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 🙏🏻. |
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.
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.
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. |
Should be fixed by https://github.com/iterative/studio/issues/5715. Maybe we can wait for that.
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. |
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. |
I was thinking it could be part of this:
Yup, we can start without it. |
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." |
Yes, we can do that.
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.
Currently, it will sync as soon as there is a change in the workspace. This falls down when
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. |
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.
My take - that would be confusing - when fields change they semantics.
They also could show Studio link, GH link, their status (we could check in background data eventually). |
My take - that's too crowded, also we miss making it explicit (e.g. Studio column name would make the purpose clear). |
@dberenbaum @shcheklein is this the correct view/state that we want a shareable link to point to: |
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 🙏 |
👍🏻 We've recently split the
Yes, they do.
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 |
A couple of questions from me:
|
Yes, in most cases.
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?).
what are the |
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: The reason for using the checkbox here was to apply the show only selected commits filter: This means we can share a URL of the form:
Whoever receives the link will be able to:
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. |
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) |
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:
LMK if you have questions regarding any of the above. 🙏🏻 |
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 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. |
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 |
Thanks, Matt. Looks great. |
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:
The text was updated successfully, but these errors were encountered: