-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 push: studio integration #9074
Comments
Github CLI has |
Edited to add:
|
What would this link to? Is it the default project view or something else? Afaik there is no real concept of a view for a single experiment in Studio |
I think it's discussed in https://github.com/iterative/studio/issues/5150.
See https://github.com/iterative/studio/issues/5273#issuecomment-1462105892. @skshetry Looks like we already have the API needed for this. |
I'll leave that upto the Studio team to decide. :) Also there are open questions how Studio should pull new refs. Webhooks don't work for refs. |
This may be a bit tricky with just |
Some updates here: I am waiting for Studio to parse and show the experiment refs. We already have the functionality to notify Studio of the refs. The API endpoint does refresh and tries to parse new commits.
I am thinking of implementing support in dvc config for token and calling this item done here for now. Secure storage implementation can be done in the future.
Looks like this was deprioritized. So I don't see it happening any time soon. I have 2 questions regarding the workflow:
|
Sounds good @skshetry, thanks!
I think it should push by default. It's a lightweight ping, and if it fails we can make it not too alarming. I don't see much reason not to push in most cases. We already will need to figure out how to differentiate this from the live sharing to studio that's enabled currently by the env var, so let's not add more options than necessary yet.
Is it needed? Similar to above, is there a situation where it's harmful to send a ping to Studio if the token exists? Should we return an error code on failures to ping Studio? My only worry would be not breaking CI jobs if the ping to Studio fails. |
I see we might end up trying to avoid having a token according to https://github.com/iterative/studio/issues/5050#issuecomment-1479601463:
In that case, I can see more why we may need some way to enable/disable sending to Studio. It might be fine to ping by default and quietly fail since it's a pretty lightweight request, but I'd agree we probably need some way to opt out of this behavior. |
@skshetry We also may need to show some warning if the ping to Studio is successful but Studio cannot show it because the parent commit has not been pushed. See https://github.com/iterative/studio/issues/4940#issuecomment-1481247133. Should we add it as a task? |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
FYI you may have to sync with main for this to work. We changed the endpoint. |
The only thing that's required for the release is #9295, but it's not a blocker on dvc side. |
For dvc/dvc/repo/experiments/executor/base.py Lines 641 to 649 in efc2d7a
DVCLive should do the same order of calls when |
We could also remove the |
@skshetry Could you open a docs issue/PR for this work 🙏 ? |
@skshetry I added this item:
Also, are you still planning on this item?
Even if it's a project-level URL, I think it would be helpful to both indicate the ping was successful and make it easy to navigate to Studio to see the experiments. |
It can be implemented later on when Studio starts returning URL. |
This comment was marked as outdated.
This comment was marked as outdated.
I need your opinion here, @dberenbaum. Note that we have I'd prefer being explicit here with |
Maybe the problem is that we are using It could work like:
|
Are there any other ways to control besides using env vars (personal bias - I never liked things like |
If we use |
Can it be / should it be controlled on the code level as well? E.g. enabled by default is token is set + an option to opt out.? |
@shcheklein Can we agree on the schema and follow up on the exact workflow? Any thoughts on using both Using both of those options as described above unifies the live sharing and exp push workflow, and it gives flexibility to tweak the workflows. We can save the token and either share or not, and can change the default behavior (we could enable |
@dberenbaum could you please clarify the details a bit? I think it's hard to understand how all the things fit together. So, we have a token in the DVC config now (who is reading it and how do we pass it downstream in DVC). Af far as I understand it's also possible to set We don't use Anyways, I think we need to clarify the whole picture tbh here. It's hard to agree w/o knowing the details (or may be I don't know them enough. |
Here's a proposal:
|
okay, I see. So it's just names for configs, not env vars. So, the question here is to decide on names and see if those two are more or less enough and seem reasonable from the user perspective (assuming we can change and fine tune their exact behavior, etc), right?
It's a bit of a contradiction? A few things to think, from the top of my head:
|
Maybe you took me a bit too literally 😄. Do you have confusion over how it will work in some scenario?
See https://github.com/iterative/dvc/blob/main/dvc/env.py. Agree that it should probably be There is also https://dvc.org/doc/dvclive/env. @skshetry Maybe you have thoughts on sections/naming for both options in the config?
Not sure I get what each option there would mean. I think we could adjust the "default" behavior via the UX in #9265. For example:
In other words, rather than have an option like |
@skshetry Let's wrap this up by:
Edit: updated the post at the top. |
@dberenbaum can you please clarify where did we land on with the experience? I'm a bit worried we're adding friction to the user for the happy flow |
@omesser There's a lot of related discussion in #9295. For now, here's how it will work:
Nothing else is planned for now. We discussed auto-pushing exp refs and the corresponding dvc cached outputs, but after the discussion in #9295, I think we should wait. We have taken some steps to make live updates feel connected to the rest of the experiments workflow (like replacing the live row with the exp ref row in Studio). Live sharing is a different scenario than |
Whoops, thanks @skshetry! |
To make
exp push/pull
more useful, users need to be able to see and manage the exp refs from within Studio (since those exp refs are ignored by GitHub and other Git providers). One of the challenges is that there is not even any trigger by which the Git providers can alert Studio that new exp refs have been pushed.To enable Studio support for pushed experiments, DVC needs to:
Tasks
STUDIO_REPO_URL
in run_env. #9161Blocked/out of scope issues:
Tasks
Related:
The text was updated successfully, but these errors were encountered: