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

Push running checkpoint to remote #6332

Merged
merged 24 commits into from
Aug 12, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
14175c8
Push running checkpoint to remote
karajan1001 Jul 19, 2021
770e5bc
Update dvc/repo/experiments/executor/base.py
karajan1001 Jul 20, 2021
15036f3
Get the branch name
karajan1001 Jul 21, 2021
205ce57
Finish this PR
karajan1001 Jul 26, 2021
b6f147a
Update after finally commit
karajan1001 Jul 27, 2021
65eb8d6
Some problems found in review
karajan1001 Jul 27, 2021
5077862
Update dvc/repo/experiments/executor/base.py
karajan1001 Jul 27, 2021
dd7b380
add remote check before auto push
karajan1001 Aug 2, 2021
92d9b07
Split current one env into two
karajan1001 Aug 4, 2021
7c271b3
Rename DVC_EXP_CHECKPOINT_PUSH to DVC_EXP_AUTO_PUSH
karajan1001 Aug 5, 2021
34f4da3
value error
karajan1001 Aug 5, 2021
37d2150
Remove getenv_bool switch to env2bool
karajan1001 Aug 6, 2021
0cfd8d1
Better on handling dulwich exceptions
karajan1001 Aug 6, 2021
ce7cd7d
Name changing removing prints
karajan1001 Aug 6, 2021
e666d2e
Update dvc/env.py
karajan1001 Aug 9, 2021
50d419f
Some changes in review
karajan1001 Aug 9, 2021
5cf541c
Add validation to repo url
karajan1001 Aug 10, 2021
dfcc946
Add new type of SCMError
karajan1001 Aug 10, 2021
060ec01
Update dvc/repo/experiments/executor/base.py
karajan1001 Aug 10, 2021
ffb657a
Use spy tests call counts and args
karajan1001 Aug 10, 2021
41aeaaf
Update dvc/scm/git/backend/base.py
karajan1001 Aug 11, 2021
488c7f3
Update dvc/scm/git/backend/dulwich.py
karajan1001 Aug 11, 2021
4a16226
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Aug 11, 2021
32677a2
Remove validate_git_repo to validate_git_remote
karajan1001 Aug 11, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 1 addition & 12 deletions dvc/repo/experiments/executor/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -252,10 +252,6 @@ def on_diverged_ref(orig_ref: str, new_rev: str):

@classmethod
def _validate_remotes(cls, dvc: "Repo", git_remote: Optional[str]):
from dulwich.client import get_transport_and_path
from dulwich.porcelain import get_remote_repo

from dvc.scm.base import SCMError

if git_remote == dvc.root_dir:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@pmrowla pmrowla Aug 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

logger.warning(
Expand All @@ -264,14 +260,7 @@ def _validate_remotes(cls, dvc: "Repo", git_remote: Optional[str]):
"will automatically be pushed to the default DVC remote "
"(if any) on each experiment commit."
)
try:
_, location = get_remote_repo(dvc.scm.dulwich.repo, git_remote)
get_transport_and_path(location)
except Exception as exc:
raise SCMError(
f"'{git_remote}' is not a valid Git remote or URL"
) from exc

dvc.scm.validate_git_repo(git_remote)
dvc.cloud.get_remote_odb()

@classmethod
Expand Down
1 change: 1 addition & 0 deletions dvc/scm/git/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,7 @@ def get_fs(self, rev: str):
checkout_index = partialmethod(_backend_func, "checkout_index")
status = partialmethod(_backend_func, "status")
merge = partialmethod(_backend_func, "merge")
validate_git_repo = partialmethod(_backend_func, "validate_git_repo")

def resolve_rev(self, rev: str) -> str:
from dvc.repo.experiments.utils import exp_refs_by_name
Expand Down
4 changes: 4 additions & 0 deletions dvc/scm/git/backend/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -347,3 +347,7 @@ def merge(

Returns revision of the merge commit or None if no commit was made.
"""

@abstractmethod
def validate_git_repo(self, url: str):
"""validate remote git repo"""
karajan1001 marked this conversation as resolved.
Show resolved Hide resolved
14 changes: 14 additions & 0 deletions dvc/scm/git/backend/dulwich.py
Original file line number Diff line number Diff line change
Expand Up @@ -646,3 +646,17 @@ def merge(
squash: bool = False,
) -> Optional[str]:
raise NotImplementedError

def validate_git_repo(self, url: str):
from dulwich.client import LocalGitClient, get_transport_and_path
from dulwich.porcelain import get_remote_repo

try:
_, location = get_remote_repo(self.repo, url)
client, path = get_transport_and_path(location)
except Exception as exc:
raise SCMError(
f"'{url}' is not a valid Git remote or URL"
) from exc
if isinstance(client, LocalGitClient) and not os.path.exists(path):
karajan1001 marked this conversation as resolved.
Show resolved Hide resolved
raise SCMError(f"'{url}' is not a valid Git remote or URL")
karajan1001 marked this conversation as resolved.
Show resolved Hide resolved
3 changes: 3 additions & 0 deletions dvc/scm/git/backend/gitpython.py
Original file line number Diff line number Diff line change
Expand Up @@ -618,3 +618,6 @@ def merge(
raise MergeConflictError("Merge contained conflicts") from exc
raise SCMError("Merge failed") from exc
return None

def validate_git_repo(self, url: str):
raise NotImplementedError
3 changes: 3 additions & 0 deletions dvc/scm/git/backend/pygit2.py
Original file line number Diff line number Diff line change
Expand Up @@ -561,3 +561,6 @@ def merge(
self.repo.state_cleanup()
self.repo.index.write()
return None

def validate_git_repo(self, url: str):
raise NotImplementedError