Push running checkpoint to remote #6332

karajan1001 · 2021-07-19T12:06:02Z

Fix #6182

❗ I have followed the Contributing to DVC checklist.
📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.
Add checkpoint backup dvc.org#2672

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

pmrowla

looks good, just a couple small comments

dvc/repo/experiments/executor/base.py

efiop · 2021-07-21T09:33:22Z

dvc/repo/experiments/executor/base.py

@@ -464,6 +466,20 @@ def checkpoint_callback(
            exp_rev = cls.commit(
                scm, exp_hash, exp_name=name, force=force, checkpoint=True
            )
+
+            git_remote = os.environ.get("DVC_EXP_AUTO_PUSH", None)


Please put this env var into dvc/env.py and use it as a var.

also what about a different meaning, e.g. DVC_EXP_AUTO_PUSH=10 meaning "push every 10 checkpoints?"

@casperdcl
DVC_EXP_AUTO_PUSH is the remote git repo for auto push. Maybe this name needs to be modified. But it is better to discuss this problem in the PR for dvc.org

PR for dvc.org

I don't follow. I understand what you're using DVC_EXP_AUTO_PUSH for. I'm asking for:

DVC_EXP_AUTO_PUSH to be changed to mean something else (push every N epochs)

or a new variable DVC_EXP_AUTO_PUSH_EVERY added (push every N epochs)

or 2 different variables with less ambiguous names DVC_EXP_AUTOPUSH_CHECKPOINT=0, DVC_EXP_AUTOPUSH_REMOTE=""

Basically I think a "push every N epochs" config should be supported in this PR rather than in a follow-up so that we can keep API consistent. WDYT @efiop @pmrowla?

@casperdcl
Is "push every N commits" actually a requirement for CML? This seems like an additional feature request that's unrelated to being able to recover checkpoints from a dead CI runner.

And it seems like there's still some behavior discussion that would need to be worked out there, i.e. what happens if I have it set to 10 and my experiment generates less than 10 commits, should DVC not push anything?

It also seems like something that should maybe be tied to dvclive and not DVC itself, since DVC doesn't even have a real concept of epoch count/iteration awareness (related dvclive ticket: iterative/dvclive#113). Since dvclive is aware of actual epoch counts, it can just call experiments.push at the appropriate time without the need for adding more env vars

DVC_EXP_GIT_REMOTE is used for the default exp push git remote. Both dvc exp push and auto push can use it (But the auto push cann't work without it, while dvc exp push can run git remote arguments).

But it is a totally new feature, involving changes on dvc exp push should not be mixed in this PR.

Configuring multiple git remotes is a pretty typical use case

ok in that case I'd go with

Split into two env vars

i.e. DVC_EXP_CHECKPOINT_PUSH:bool and DVC_EXP_GIT_REMOTE:str?

Regarding:

I think that the behavior of auto push should go with dvc exp push

This also makes sense to me

This is also not specific to checkpoints, regular experiments will end be pushed the same way if the env var is set.

I think we need to keep CHECKPOINT out of the names of these env vars for this reason.

Splitting into two env vars makes sense, but it does make the UX more complex. I think it's worth making sure this is really necessary before creating extra complexity for hypothetical future needs, so I have a couple follow-ups:

Why not rename the env var to DVC_EXP_GIT_REMOTE and make no other changes? That provides future flexibility but also keeps the UX concise.

Alternatively, I think we could reconsider having a default remote.

Configuring multiple git remotes is a pretty typical use case (original/upstream repo vs my fork/origin). If I'm working on a fork and want my run experiments pushed to the original repo but not my fork, I need to be able to configure the remote.

Is this likely in CI?

Even locally, are users likely to do git push -u upstream mybranch? My typical workflow would be to pull from upstream and push to origin by default.

If we aren't following Git default remote configuration, why not document that origin is the default instead of requiring it to be explicit?

Is this likely in CI?

in CI probably not

Even locally, are users likely to do git push -u upstream mybranch? My typical workflow would be to pull from upstream and push to origin by default.

I do this regularly with DVC - we have CI configured to run a different set of actions on pull requests vs branches pushed directly to iterative/dvc, so there are times when I want to push a certain branch for specific features (that will require the stricter branch tests) to upstream rather than my personal fork

If we aren't following Git default remote configuration, why not document that origin is the default instead of requiring it to be explicit?

This is an option, although as someone who is going to have to answer support questions regarding "why is the default for this experiment origin instead of the default I set on my git branch", my preference would be to continue requiring the explicit argument

@dberenbaum

Alternatively, I think we could reconsider having a default remote.

I think having a default remote for the dvc exp push is reasonable, both dvc push and git push have default remotes.
How about adding it to the dvc.config, it is more manageable.

And if a default remote for dvc exp push is setted, the only env arguments here to control the auto push process is DVC_EXP_CHECKPOINT_PUSH:bool.

I don't think users will directly use auto push without using dvc exp push previously. If they are familiar with dvc exp push, and knows how to set up a default remote. The auto push configuration is quite a simple one.

Fix iterative#6182

Co-authored-by: Peter Rowlands (변기호) <peter@pmrowla.com>

1. move env name to dvc/env.py. 2. add some tests for it.

tests/func/experiments/test_checkpoints.py

dvc/repo/experiments/executor/base.py

tests/func/experiments/test_checkpoints.py

1. change the behaviour of self remote 2. do not use string DVC_EXP_AUTO_PUSH 3. downgrade the logger level in auto push 4. use full branch ref

dvc/repo/experiments/executor/base.py

Co-authored-by: Peter Rowlands (변기호) <peter@pmrowla.com>

jorgeorpinel · 2021-07-28T08:07:40Z

dvc/repo/experiments/executor/base.py

+                f"try to auto checkpoints to {git_remote} which is the "
+                "running repository dvc cache will be pushed to the "
+                "default remote while git references will not be pushed"


I'm not sure I understand this message. Maybe something like

Try to save the experiment status to {git_remote} after every checkpoint.
It's data will be uploaded to the default remote (if any).

Not sure what Git refecences we're talking about. Meaning the experiments themselves? If so what gets saved to {git_remote} ?

We are git pushing the currently running experiment ref. In this condition, git_remote points to the current git repo itself, so nothing actually happens on the git side. However, when this env var is set, we also automate dvc pushing used cache objects and run cache for the currently running experiment as well.

So the side effect here is that nothing will be "pushed" anywhere on the git side of things, but DVC outputs and run cache will still be pushed to the default configured DVC remote.

The warning message should probably just be something like

'{git_remote}' points to the current Git repo, experiment Git refs will not be pushed. DVC cache and run cache will automatically be pushed to the default DVC remote (if any) on each experiment commit.

karajan1001 · 2021-08-03T13:53:23Z

In summary,

split the current env into two (confirm the usage with CML team).
create a new ticket for the default dvc exp push git remote.
auto push on every several epochs -- a new feature not to discuss here (also a new ticket ) .

had I missed something?

casperdcl · 2021-08-03T18:17:36Z

@iterative/cml

dvc/repo/experiments/executor/base.py

dvc/scm/git/backend/dulwich.py

dvc/repo/experiments/executor/base.py

tests/func/experiments/test_checkpoints.py

Co-authored-by: Peter Rowlands (변기호) <peter@pmrowla.com>

pmrowla

Looks good for the most part, I think this should be the last set of changes to do

tests/func/experiments/test_checkpoints.py

dvc/scm/git/backend/base.py

dvc/scm/git/backend/dulwich.py

Co-authored-by: Peter Rowlands (변기호) <peter@pmrowla.com>

for more information, see https://pre-commit.ci

tests/func/experiments/test_checkpoints.py

dberenbaum · 2021-08-11T20:23:29Z

dvc/repo/experiments/executor/base.py

+    @classmethod
+    def _validate_remotes(cls, dvc: "Repo", git_remote: Optional[str]):
+
+        if git_remote == dvc.root_dir:


Is there a reason to have this explicit check? Would this qualify as a valid git remote? Is there some reason to think a user might set DVC_EXP_GIT_REMOTE to their local project's root dir?

Also, why fail for an invalid Git remote but only warn for this scenario?

git_remote is just a variable that is either a path/URL to a git repo (including local ones) or a remote name. If this check is hit it means it's a local path that points directly to the current DVC root directory, which in most cases is a valid git repo path.

This behavior can be useful - if you do DVC_EXP_GIT_REMOTE=. the end result would be that we auto-push DVC cache for your local experiment runs. (The git push step becomes a no-op, the same thing that happens if you do a CLI git push to your own current repo directory)

dberenbaum · 2021-08-11T20:26:18Z

dvc/repo/experiments/executor/base.py

+
+        if git_remote == dvc.root_dir:
+            logger.warning(
+                f"'{git_remote}' points to the current Git repo, experiment "


f"'{git_remote}' points to the current Git repo, experiment "

I'm not sure this is helpful since a valid remote would also point to the current Git repo, right? Maybe something like "local workspace" makes more sense than "current Git repo"?

The local workspace is your current Git repo. https://github.com/my/repo.git is not your current Git repo - it's a remote Git repo, and potentially where your current Git repo is cloned from, but "current repo" would always be your current local workspace repo

casperdcl · 2021-08-12T15:32:29Z

thanks @karajan1001 - is there a follow-up PR in https://github.com/iterative/dvc.org/pulls?

karajan1001 · 2021-08-13T07:29:40Z

@casperdcl related iterative/dvc.org#2672

* 'python310' of github.com:skshetry/dvc: Unpin networkx Remove a special queued experiments (iterative#6393) build(deps): bump google-cloud-storage from 1.41.1 to 1.42.0 (iterative#6415) Push running checkpoint to remote (iterative#6332)

karajan1001 requested a review from a team as a code owner July 19, 2021 12:06

karajan1001 requested a review from efiop July 19, 2021 12:06

karajan1001 marked this pull request as draft July 19, 2021 12:06

karajan1001 requested a review from pmrowla July 19, 2021 12:07

pmrowla suggested changes Jul 19, 2021

View reviewed changes

dvc/repo/experiments/executor/base.py Outdated Show resolved Hide resolved

dvc/repo/experiments/executor/base.py Outdated Show resolved Hide resolved

efiop reviewed Jul 21, 2021

View reviewed changes

karajan1001 and others added 4 commits July 27, 2021 09:58

Push running checkpoint to remote

14175c8

Fix iterative#6182

Update dvc/repo/experiments/executor/base.py

770e5bc

Co-authored-by: Peter Rowlands (변기호) <peter@pmrowla.com>

Get the branch name

15036f3

Finish this PR

205ce57

1. move env name to dvc/env.py. 2. add some tests for it.

karajan1001 force-pushed the fix6182 branch from 54a58e6 to 205ce57 Compare July 27, 2021 02:03

Update after finally commit

b6f147a

karajan1001 commented Jul 27, 2021

View reviewed changes

tests/func/experiments/test_checkpoints.py Outdated Show resolved Hide resolved

karajan1001 commented Jul 27, 2021

View reviewed changes

tests/func/experiments/test_checkpoints.py Outdated Show resolved Hide resolved

pmrowla reviewed Jul 27, 2021

View reviewed changes

dvc/repo/experiments/executor/base.py Outdated Show resolved Hide resolved

pmrowla suggested changes Jul 27, 2021

View reviewed changes

Some problems found in review

65eb8d6

1. change the behaviour of self remote 2. do not use string DVC_EXP_AUTO_PUSH 3. downgrade the logger level in auto push 4. use full branch ref

karajan1001 marked this pull request as ready for review July 27, 2021 10:10

karajan1001 requested a review from pmrowla July 27, 2021 10:28

pmrowla reviewed Jul 27, 2021

View reviewed changes

dvc/repo/experiments/executor/base.py Outdated Show resolved Hide resolved

pmrowla reviewed Jul 27, 2021

View reviewed changes

dvc/repo/experiments/executor/base.py Show resolved Hide resolved

karajan1001 mentioned this pull request Jul 27, 2021

Add checkpoint backup iterative/dvc.org#2672

Merged

Update dvc/repo/experiments/executor/base.py

5077862

Co-authored-by: Peter Rowlands (변기호) <peter@pmrowla.com>

jorgeorpinel reviewed Jul 28, 2021

View reviewed changes

add remote check before auto push

dd7b380

dberenbaum mentioned this pull request Aug 2, 2021

dvclive.init: Future of arguments iterative/dvclive#128

Closed

Split current one env into two

92d9b07

karajan1001 changed the title ~~[WIP] Push running checkpoint to remote~~ Push running checkpoint to remote Aug 4, 2021

Some changes in review

50d419f

karajan1001 commented Aug 9, 2021

View reviewed changes

dvc/repo/experiments/executor/base.py Outdated Show resolved Hide resolved

Add validation to repo url

5cf541c

pmrowla reviewed Aug 10, 2021

View reviewed changes

dvc/scm/git/backend/dulwich.py Outdated Show resolved Hide resolved

Add new type of SCMError

dfcc946

karajan1001 requested a review from pmrowla August 10, 2021 07:01

pmrowla suggested changes Aug 10, 2021

View reviewed changes

dvc/repo/experiments/executor/base.py Outdated Show resolved Hide resolved

tests/func/experiments/test_checkpoints.py Show resolved Hide resolved

karajan1001 and others added 2 commits August 10, 2021 16:45

Update dvc/repo/experiments/executor/base.py

060ec01

Co-authored-by: Peter Rowlands (변기호) <peter@pmrowla.com>

Use spy tests call counts and args

ffb657a

karajan1001 requested a review from pmrowla August 10, 2021 14:36

pmrowla suggested changes Aug 11, 2021

View reviewed changes

tests/func/experiments/test_checkpoints.py Outdated Show resolved Hide resolved

dvc/scm/git/backend/base.py Outdated Show resolved Hide resolved

dvc/scm/git/backend/dulwich.py Outdated Show resolved Hide resolved

karajan1001 and others added 4 commits August 11, 2021 14:02

Update dvc/scm/git/backend/base.py

41aeaaf

Co-authored-by: Peter Rowlands (변기호) <peter@pmrowla.com>

Update dvc/scm/git/backend/dulwich.py

488c7f3

Co-authored-by: Peter Rowlands (변기호) <peter@pmrowla.com>

[pre-commit.ci] auto fixes from pre-commit.com hooks

4a16226

for more information, see https://pre-commit.ci

Remove validate_git_repo to validate_git_remote

32677a2

casperdcl reviewed Aug 11, 2021

View reviewed changes

tests/func/experiments/test_checkpoints.py Show resolved Hide resolved

casperdcl assigned karajan1001 Aug 11, 2021

casperdcl added A: experiments Related to dvc exp p1-important Important, aka current backlog of things to do product: VSCode Integration with VSCode extension labels Aug 11, 2021

dberenbaum reviewed Aug 11, 2021

View reviewed changes

pmrowla approved these changes Aug 12, 2021

View reviewed changes

karajan1001 merged commit 6dd435e into iterative:master Aug 12, 2021

karajan1001 deleted the fix6182 branch August 12, 2021 07:17

efiop added the feature is a feature label Aug 15, 2021

dberenbaum mentioned this pull request Mar 27, 2023

exp share refactor - ping on exp:remove, do not fail on error #9248

Merged

dberenbaum mentioned this pull request Jun 21, 2023

exp push: default to --rev HEAD #9595

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Push running checkpoint to remote #6332

Push running checkpoint to remote #6332

karajan1001 commented Jul 19, 2021 •

edited by jorgeorpinel

pmrowla left a comment

efiop Jul 21, 2021

casperdcl Jul 26, 2021 •

edited

karajan1001 Jul 27, 2021

casperdcl Jul 27, 2021 •

edited

pmrowla Jul 28, 2021 •

edited

karajan1001 Aug 2, 2021

casperdcl Aug 2, 2021

dberenbaum Aug 2, 2021

pmrowla Aug 3, 2021

karajan1001 Aug 3, 2021

jorgeorpinel Jul 28, 2021 •

edited

pmrowla Jul 28, 2021

pmrowla Jul 28, 2021

karajan1001 commented Aug 3, 2021 •

edited

casperdcl commented Aug 3, 2021

pmrowla left a comment

dberenbaum Aug 11, 2021

dberenbaum Aug 11, 2021

pmrowla Aug 12, 2021 •

edited

dberenbaum Aug 11, 2021

pmrowla Aug 12, 2021

casperdcl commented Aug 12, 2021

karajan1001 commented Aug 13, 2021 •

edited

Push running checkpoint to remote #6332

Push running checkpoint to remote #6332

Conversation

karajan1001 commented Jul 19, 2021 • edited by jorgeorpinel

pmrowla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

casperdcl Jul 26, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

casperdcl Jul 27, 2021 • edited

Choose a reason for hiding this comment

pmrowla Jul 28, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgeorpinel Jul 28, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karajan1001 commented Aug 3, 2021 • edited

casperdcl commented Aug 3, 2021

pmrowla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmrowla Aug 12, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

casperdcl commented Aug 12, 2021

karajan1001 commented Aug 13, 2021 • edited

karajan1001 commented Jul 19, 2021 •

edited by jorgeorpinel

casperdcl Jul 26, 2021 •

edited

casperdcl Jul 27, 2021 •

edited

pmrowla Jul 28, 2021 •

edited

jorgeorpinel Jul 28, 2021 •

edited

karajan1001 commented Aug 3, 2021 •

edited

pmrowla Aug 12, 2021 •

edited

karajan1001 commented Aug 13, 2021 •

edited