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

studio: push exp refs #9295

Closed
Tracked by #9074
dberenbaum opened this issue Mar 6, 2023 · 33 comments
Closed
Tracked by #9074

studio: push exp refs #9295

dberenbaum opened this issue Mar 6, 2023 · 33 comments
Labels
A: experiments Related to dvc exp p3-nice-to-have It should be done this or next sprint product: Studio Integration with Studio

Comments

@dberenbaum
Copy link
Collaborator

dberenbaum commented Mar 6, 2023

DVCLive should dvc exp push experiment refs when:

  1. STUDIO_TOKEN is set (or live sharing to Studio is otherwise enabled)
  2. save_dvc_exp=True (even if using dvc exp run; not sure what's best if using dvc exp run but not save_dvc_exp=True)

On the Studio side, this dvc exp push should replace the live metrics with the exp ref. This would help unify the live metrics and exp refs. For typical workflows, it means that live metrics would show live updates only temporarily and eventually show a "completed" exp ref.

Edit: update from related discussion. DVCLive should not have to push experiment refs. DVC should push them during either dvc exp save or dvc exp run if the following are both true:

  1. The Studio token is available. This can be found either in the studio.token config option or the DVC_STUDIO_TOKEN env var (let's rename STUDIO_TOKEN to DVC_STUDIO_TOKEN for consistency with other env vars). In the future, we could also have a way to pass the token via CLI/Python API. We can follow the same conventions for the Studio URL.
  2. Automatic sharing/pushing is enabled. This can be enabled via the exp.auto_push config option or the DVC_EXP_AUTO_PUSH env var. Like the Studio token, we could also have a way to enable/disable through CLI/Python API. We can follow the same conventions for the Git remote URL, which should be optional.

This means including STUDIO_TOKEN/DVC_STUDIO_TOKEN would not be enough on its own to enable live sharing of experiments. I think it's okay because I would push for the onboarding to be made easier via #9265 to not rely on this env var and instead have the env var mostly used for CI.

@dberenbaum dberenbaum added the p2-medium Medium priority, should be done, but less important label Mar 6, 2023
@dberenbaum dberenbaum added p1-important Important, aka current backlog of things to do and removed p2-medium Medium priority, should be done, but less important labels Mar 21, 2023
@skshetry skshetry self-assigned this Mar 28, 2023
@dberenbaum
Copy link
Collaborator Author

I think this could actually be moved to a DVC issue. It should be that every DVC exp commit generated by dvc exp run/save should get pushed when live sharing is enabled. Related to #9074 (comment).

This also probably requires inferring a default Git branch to push to (there is a related discussion about dvc exp push that I can't seem to find).

@daavoo
Copy link
Contributor

daavoo commented Mar 31, 2023

I think this could actually be moved to a DVC issue. It should be that every DVC exp commit generated by dvc exp run/save should get pushed when live sharing is enabled. Related to #9074 (comment).

It makes sense to me.

This also probably requires inferring a default Git branch to push to

Not sure I understand what is that requirement about

@dberenbaum dberenbaum transferred this issue from iterative/dvclive Mar 31, 2023
@dberenbaum
Copy link
Collaborator Author

Sorry, I meant inferring a default Git remote.

@daavoo
Copy link
Contributor

daavoo commented Mar 31, 2023

Sorry, I meant inferring a default Git remote.

It appears that it is already handled in the current exp push:

_, repo_url = get_remote_repo(repo.scm.dulwich.repo, git_remote)

@dberenbaum
Copy link
Collaborator Author

🤨 Then why does dvc exp push require a git remote argument and auto-pushing requires DVC_EXP_GIT_REMOTE?

@daavoo
Copy link
Contributor

daavoo commented Mar 31, 2023

🤨 Then why does dvc exp push require a git remote argument and auto-pushing requires DVC_EXP_GIT_REMOTE?

Because those 2 things (remote arg, env var) are from before the code I linked, which was introduced for Studio integration.
More discussion here https://iterativeai.slack.com/archives/CNQ95CG1K/p1679983222971979

@dberenbaum
Copy link
Collaborator Author

I'm in favor of making the git remote optional in all these places. Even in a forking workflow, I don't really see the problem since there's an option to configure it, and the typical workflow would still be to push to origin.

I know @pmrowla suggested he typically pushes to upstream, but I can't recall the exact workflow there. It seems there is otherwise consensus that this is more burdensome than helpful.

@skshetry
Copy link
Member

skshetry commented Apr 1, 2023

It appears that it is already handled in the current exp push:

_, repo_url = get_remote_repo(repo.scm.dulwich.repo, git_remote)

It's not inferring default remote, it's using the same remote where dvc exp push pushed, unlike what we do in dvc-studio-client.

@pmrowla
Copy link
Contributor

pmrowla commented Apr 1, 2023

IIRC DVC_EXP_GIT_REMOTE was something that had been requested on the CML side, so that exp's run inside CI workflows would be preserved, but I'm not sure if anyone is actually using it

@dberenbaum
Copy link
Collaborator Author

It's not inferring default remote, it's using the same remote where dvc exp push pushed, unlike what we do in dvc-studio-client.

What does dvc-studio-client do?

IIRC DVC_EXP_GIT_REMOTE was something that had been requested on the CML side, so that exp's run inside CI workflows would be preserved, but I'm not sure if anyone is actually using it

I suggest we keep it but make it optional.

@skshetry
Copy link
Member

skshetry commented Apr 3, 2023

dvc-studio-client uses branch’s upstream remote that is set (usually when doing git push —set-upstream <remote>.

whereas dvc exp push requires an explicit remote which may or may not be same as the default remote for the branch.

Note: there is no default remote in Git, only default remote for each branch.

@dberenbaum
Copy link
Collaborator Author

Is dvc-studio-client using that solely to decide which studio project url to use? Can we pass a remote to dvc-studio-client? Even if we decide to try to infer the remote, I think we will always want to have an option to set it explicitly, and we should be respecting that in dvc-studio-client.

@dberenbaum
Copy link
Collaborator Author

@skshetry @daavoo @shcheklein (and anyone else interested): I updated my thoughts on this ticket at the top. PTAL and give your feedback so we can finalize this. I'd like to make a decision before we put out any content about this so we don't have to change it too many times.

@daavoo
Copy link
Contributor

daavoo commented Apr 4, 2023

Not sure I understand the following:

This means including STUDIO_TOKEN/DVC_STUDIO_TOKEN would not be enough on its own to enable live sharing of experiments.

Do you mean that the "live sharing" (sending updates to studio/api/live while the experiment is running) should be coupled (enabled/disabled) with the "auto push" (pushing exp refs on completion)?

@dberenbaum
Copy link
Collaborator Author

Yes, that's what I meant @daavoo.

@daavoo
Copy link
Contributor

daavoo commented Apr 4, 2023

Yes, that's what I meant @daavoo.

Ok. I slightly disagree then 😄

I reason that exp push might not be a lightweight operation (especially defaulting to push cache), and I can imagine scenarios where I would want to send the lightweight, live-sharing updates from all my experiments but only exp push a few or even one.
At the very least, I would like that behavior to be possible

@dberenbaum
Copy link
Collaborator Author

Good point. Currently we can push/share 1) live metrics, 2) exp refs, and 3) cache updates. I think of live metrics as a temporary placeholder for exp refs (are there times when it would be important to share live metrics but not the exp ref?). For cache updates, if we go in the direction of using Studio for recovering interrupted training jobs, we may have live pushes to the cache in some scenarios.

I would prefer to split behavior by cache/no-cache instead of live/non-live. We could make the auto push option a string (like suggested here) that specifies what gets auto-pushed (basic vs cache?), or we could have another option for whether to include the cache.

@daavoo
Copy link
Contributor

daavoo commented Apr 4, 2023

I would prefer to split behavior by cache/no-cache instead of live/non-live.

👍

@skshetry
Copy link
Member

skshetry commented Apr 5, 2023

How would dvclive work? It still has to use STUDIO_TOKEN for that, no?
Regarding exp push on exp run, I don't think we should support dvc exp push --no-cache.

In any case, it seems we need both environment variables.

@skshetry
Copy link
Member

skshetry commented Apr 5, 2023

Or, we could encourage use of cli flag for auto-push, and use STUDIO_TOKEN for live updates.

STUDIO_TOKEN=token # only for live updates
dvc exp run --push

It will be more explicit this way. :)

@skshetry
Copy link
Member

skshetry commented Apr 5, 2023

Discussed with @dberenbaum. It looks like we will use the existing DVC_EXP_AUTO_PUSH with multiple options which by default will push experiments and cache, and live metrics if STUDIO_TOKEN is set (minor nitpick: AUTO may be unnecessary here).

We also discussed that we'll need some CLI flags for exp run/save, and some config options.

On DVCLive side, it will have to read from dvc's config, and dvc will have to also set DVC_EXP_AUTO_PUSH envvar when running scripts to inform dvclive.

@skshetry
Copy link
Member

skshetry commented Apr 5, 2023

For CLI, we could name it dvc exp run --push/push-no-cache or --push={all,no-cache} or something similar.

@dberenbaum
Copy link
Collaborator Author

dberenbaum commented Apr 5, 2023

This quickly got overly complicated 😅 . I can lower the priority of automatically pushing exp refs, and instead let's focus on what's implemented already.

The behavior of the env var STUDIO_TOKEN and the config option for studio.token seem disconnected. I have to manage my token in two different places to use both live sharing and updating exp refs in studio. What is the simplest way we can reconcile this?

The minimum I can think of to make them more compatible:

  • In dvc exp push (and anywhere else in DVC), setting STUDIO_TOKEN should be the same as having studio.config (the env var should override the config option). Minor: I'd suggest making DVC_STUDIO_TOKEN for clarity and consistency.
  • In DVCLive, if STUDIO_TOKEN=true, look for the config studio.token value and use that. We can always add other ways to enable this in the future (Live(studio=True)).

@skshetry
Copy link
Member

skshetry commented Apr 6, 2023

DVCLive should auto-discover studio.token from dvc's config, no need to set STUDIO_TOKEN=true.

@dberenbaum
Copy link
Collaborator Author

DVCLive should auto-discover studio.token from dvc's config, no need to set STUDIO_TOKEN=true.

We could do it this way, but is this a reasonable default behavior whenever the token is saved to the config? I would like to save my Studio token once to my global config file, but I'd be less likely to do that if it meant all my experiments from all my projects get automatically live shared to Studio.

@skshetry
Copy link
Member

skshetry commented Apr 7, 2023

I'll suggest keeping this issue open and releasing what we have for now. The only thing that's not possible at this time is not pushing the cache and that can be done by following ways:

dvc exp run
dvc exp push <last-exp-run> --no-cache

If we want to show the URL in exp run with auto-push, I can take a look. Otherwise, let's release this and come back to it later.

@dberenbaum
Copy link
Collaborator Author

  • In dvc exp push (and anywhere else in DVC), setting STUDIO_TOKEN should be the same as having studio.config (the env var should override the config option). Minor: I'd suggest making DVC_STUDIO_TOKEN for clarity and consistency.

@skshetry Agreed we shouldn't let this block anything, but what do you think about this part at least? It seems pretty straightforward, and at least it means someone who sets the env var gets all the benefits you added to dvc exp push.

@skshetry
Copy link
Member

skshetry commented Apr 7, 2023

STUDIO_TOKEN envvar already overrides the config value (at the moment, it requires additional feature flag but that will go away).

token = os.environ.get("STUDIO_TOKEN") or config.get("studio_token")

Minor: I'd suggest making DVC_STUDIO_TOKEN for clarity and consistency.

I don't have issue with this, although I don't think consistency matters here as the token is applicable for the whole ecosystem, not just dvc. We could support both, prioritizing DVC_* envvar, although no strong opinion. Also, how should dvclive work? It already uses STUDIO_TOKEN, and may be a breaking change.

@dberenbaum
Copy link
Collaborator Author

STUDIO_TOKEN envvar already overrides the config value (at the moment, it requires additional feature flag but that will go away).

🤦 Sorry, then I guess we are in good shape here.

I don't have issue with this, although I don't think consistency matters here as the token is applicable for the whole ecosystem, not just dvc. We could support both, prioritizing DVC_* envvar, although no strong opinion. Also, how should dvclive work? It already uses STUDIO_TOKEN, and may be a breaking change.

Yup, I agree with this approach of giving DVC_* priority, although I don't think it's a huge deal this early if we break it. Either way, we could update all the Studio and DVCLive docs to use DVC_STUDIO_TOKEN instead of STUDIO_TOKEN going forward. I wouldn't worry about DVCLive or Studio using DVC_* since we are working to unify all of these as part of the DVC ecosystem.

@dberenbaum dberenbaum added p3-nice-to-have It should be done this or next sprint and removed p1-important Important, aka current backlog of things to do labels Apr 10, 2023
@dberenbaum
Copy link
Collaborator Author

Lowering the priority.

Either way, we could update all the Studio and DVCLive docs to use DVC_STUDIO_TOKEN instead of STUDIO_TOKEN going forward.

@daavoo Thoughts on this?

@daavoo
Copy link
Contributor

daavoo commented Apr 11, 2023

@daavoo Thoughts on this?

No big deal. Can even support both in the same line of code

@skshetry skshetry removed their assignment Apr 18, 2023
@dberenbaum dberenbaum added product: VSCode Integration with VSCode extension A: experiments Related to dvc exp labels Apr 27, 2023
@dberenbaum
Copy link
Collaborator Author

Coming back to this one. Rather than automatically pushing to the git and/or dvc remote, WDYT about hinting to do dvc exp push as part of the output of the "done" event? We could also show hints in Studio for completed live experiments to be pushed.

@dberenbaum
Copy link
Collaborator Author

Closing this as not planned since I don't think there's any reason to auto-push at the moment.

@dberenbaum dberenbaum closed this as not planned Won't fix, can't repro, duplicate, stale May 15, 2023
@daavoo daavoo added product: Studio Integration with Studio and removed product: VSCode Integration with VSCode extension labels May 17, 2023
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 p3-nice-to-have It should be done this or next sprint product: Studio Integration with Studio
Projects
None yet
Development

No branches or pull requests

4 participants