From 604b4a4e13c390b92c2913023de0c41f2a60f15c Mon Sep 17 00:00:00 2001 From: gtarpenning Date: Mon, 25 Jul 2022 17:08:52 -0700 Subject: [PATCH 1/7] improved error handling for git-version param in launch --- wandb/sdk/launch/utils.py | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/wandb/sdk/launch/utils.py b/wandb/sdk/launch/utils.py index 27a93378ba7..f50ef58be3b 100644 --- a/wandb/sdk/launch/utils.py +++ b/wandb/sdk/launch/utils.py @@ -430,12 +430,26 @@ def _fetch_git_repo(dst_dir: str, uri: str, version: Optional[str]) -> None: except git.exc.GitCommandError as e: raise ExecutionError( "Unable to checkout version '%s' of git repo %s" - "- please ensure that the version exists in the repo. " + "- please ensure that the version exists in the repo. \n" "Error: %s" % (version, uri, e) ) else: - repo.create_head("master", origin.refs.master) - repo.heads.master.checkout() + version = "master" # change to "main" for current github practice? + try: + repo.create_head(version, origin.refs[version]) + repo.heads[version].checkout() + except git.exc.GitCommandError as e: + raise ExecutionError( + "Unable to checkout version '%s' of git repo %s" + "- please ensure that the version exists in the repo. \n" + "Error: %s" % (version, uri, e) + ) + except (AttributeError, IndexError) as e: + raise ExecutionError( + "Unable to checkout default version '%s' of git repo %s " + "- to specify a git version use: --git-version \n" + "Error: %s" % (version, uri, e) + ) repo.submodule_update(init=True, recursive=True) From 350851ebafa8d756a878516b306741f8f7bfdf04 Mon Sep 17 00:00:00 2001 From: gtarpenning Date: Fri, 5 Aug 2022 10:34:56 -0700 Subject: [PATCH 2/7] added fallback from main -> master when no git version found --- wandb/sdk/launch/utils.py | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/wandb/sdk/launch/utils.py b/wandb/sdk/launch/utils.py index f50ef58be3b..fea2cb10d2c 100644 --- a/wandb/sdk/launch/utils.py +++ b/wandb/sdk/launch/utils.py @@ -424,20 +424,28 @@ def _fetch_git_repo(dst_dir: str, uri: str, version: Optional[str]) -> None: repo = git.Repo.init(dst_dir) origin = repo.create_remote("origin", uri) origin.fetch() - if version is not None: + + if version: try: repo.git.checkout(version) except git.exc.GitCommandError as e: raise ExecutionError( "Unable to checkout version '%s' of git repo %s" - "- please ensure that the version exists in the repo. \n" + "- please ensure that the version exists in the repo. " "Error: %s" % (version, uri, e) ) else: - version = "master" # change to "main" for current github practice? + # Check if main is in origin, else set branch to master + branches = [ref.name for ref in repo.references] + if "main" in branches or "origin/main" in branches: + version = "main" + else: + version = "master" + try: repo.create_head(version, origin.refs[version]) repo.heads[version].checkout() + wandb.termlog(f"No git branch passed. Defaulted to branch: {version}") except git.exc.GitCommandError as e: raise ExecutionError( "Unable to checkout version '%s' of git repo %s" @@ -450,6 +458,7 @@ def _fetch_git_repo(dst_dir: str, uri: str, version: Optional[str]) -> None: "- to specify a git version use: --git-version \n" "Error: %s" % (version, uri, e) ) + repo.submodule_update(init=True, recursive=True) From 484365cdff7f9529aa8d87ada8218a7423ab3044 Mon Sep 17 00:00:00 2001 From: gtarpenning Date: Thu, 11 Aug 2022 14:14:37 -0700 Subject: [PATCH 3/7] temp working on tests --- tests/unit_tests/tests_launch/test_launch.py | 50 +++++++++++++++----- wandb/sdk/launch/utils.py | 13 +++-- 2 files changed, 48 insertions(+), 15 deletions(-) diff --git a/tests/unit_tests/tests_launch/test_launch.py b/tests/unit_tests/tests_launch/test_launch.py index d8f590e4264..8d9b406b8c1 100644 --- a/tests/unit_tests/tests_launch/test_launch.py +++ b/tests/unit_tests/tests_launch/test_launch.py @@ -37,18 +37,26 @@ def mocked_fetchable_git_repo(): m = mock.Mock() - def populate_dst_dir(dst_dir): - with open(os.path.join(dst_dir, "train.py"), "w") as f: - f.write(fixture_open("train.py").read()) - with open(os.path.join(dst_dir, "requirements.txt"), "w") as f: - f.write(fixture_open("requirements.txt").read()) - with open(os.path.join(dst_dir, "patch.txt"), "w") as f: - f.write("test") - return mock.Mock() + def mock_specific_branch(branch_name): + def populate_dst_dir(dst_dir): + repo = mock.Mock() + reference = mock.Mock() + reference.name = branch_name + repo.references = [reference] + with open(os.path.join(dst_dir, "train.py"), "w") as f: + f.write(fixture_open("train.py").read()) + with open(os.path.join(dst_dir, "requirements.txt"), "w") as f: + f.write(fixture_open("requirements.txt").read()) + with open(os.path.join(dst_dir, "patch.txt"), "w") as f: + f.write("test") + return repo - m.Repo.init = mock.Mock(side_effect=populate_dst_dir) - with mock.patch.dict("sys.modules", git=m): - yield m + m.Repo.init = mock.Mock(side_effect=populate_dst_dir) + + with mock.patch.dict("sys.modules", git=m): + yield m + + return mock_specific_branch @pytest.fixture @@ -1459,3 +1467,23 @@ def test_launch_no_url_job_or_docker_image( ) except wandb.errors.LaunchError as e: assert "Must specify a uri, job or docker image" in str(e) + + +def test_launch_git_version_master( + live_mock_server, + test_settings, + mocked_fetchable_git_repo, +): + api = wandb.sdk.internal.internal_api.Api( + default_settings=test_settings, load_settings=False + ) + with mocked_fetchable_git_repo("master"): + try: + launch.run( + api=api, + uri="https://foo:bar@github.com/FooTest/Foo.git", + job=None, + project="new-test", + ) + except wandb.errors.LaunchError as e: + assert "No git branch passed. Defaulted to branch: master" in str(e) diff --git a/wandb/sdk/launch/utils.py b/wandb/sdk/launch/utils.py index 6ed601f84ab..68e03eda62f 100644 --- a/wandb/sdk/launch/utils.py +++ b/wandb/sdk/launch/utils.py @@ -427,7 +427,7 @@ def _fetch_git_repo(dst_dir: str, uri: str, version: Optional[str]) -> None: origin = repo.create_remote("origin", uri) origin.fetch() - if version: + if version is not None: try: repo.git.checkout(version) except git.exc.GitCommandError as e: @@ -437,8 +437,13 @@ def _fetch_git_repo(dst_dir: str, uri: str, version: Optional[str]) -> None: "Error: %s" % (version, uri, e) ) else: + if repo.getattr("references", None) is not None: + print(repo) + print(repo.references) + branches = [ref.name for ref in repo.references] + else: + branches = [] # Check if main is in origin, else set branch to master - branches = [ref.name for ref in repo.references] if "main" in branches or "origin/main" in branches: version = "main" else: @@ -449,13 +454,13 @@ def _fetch_git_repo(dst_dir: str, uri: str, version: Optional[str]) -> None: repo.heads[version].checkout() wandb.termlog(f"No git branch passed. Defaulted to branch: {version}") except git.exc.GitCommandError as e: - raise ExecutionError( + raise LaunchError( "Unable to checkout version '%s' of git repo %s" "- please ensure that the version exists in the repo. \n" "Error: %s" % (version, uri, e) ) except (AttributeError, IndexError) as e: - raise ExecutionError( + raise LaunchError( "Unable to checkout default version '%s' of git repo %s " "- to specify a git version use: --git-version \n" "Error: %s" % (version, uri, e) From e23a7bfd3d30673d6a240ba1ceaf8fcc09267572 Mon Sep 17 00:00:00 2001 From: gtarpenning Date: Thu, 11 Aug 2022 16:29:21 -0700 Subject: [PATCH 4/7] added tests and slighly changed return-path of git fetch function, possibly breaking (!) --- tests/unit_tests/tests_launch/test_launch.py | 142 +++++++++++++++---- wandb/sdk/launch/_project_spec.py | 10 +- wandb/sdk/launch/utils.py | 11 +- 3 files changed, 121 insertions(+), 42 deletions(-) diff --git a/tests/unit_tests/tests_launch/test_launch.py b/tests/unit_tests/tests_launch/test_launch.py index 8d9b406b8c1..4e3f5cfad63 100644 --- a/tests/unit_tests/tests_launch/test_launch.py +++ b/tests/unit_tests/tests_launch/test_launch.py @@ -37,26 +37,18 @@ def mocked_fetchable_git_repo(): m = mock.Mock() - def mock_specific_branch(branch_name): - def populate_dst_dir(dst_dir): - repo = mock.Mock() - reference = mock.Mock() - reference.name = branch_name - repo.references = [reference] - with open(os.path.join(dst_dir, "train.py"), "w") as f: - f.write(fixture_open("train.py").read()) - with open(os.path.join(dst_dir, "requirements.txt"), "w") as f: - f.write(fixture_open("requirements.txt").read()) - with open(os.path.join(dst_dir, "patch.txt"), "w") as f: - f.write("test") - return repo - - m.Repo.init = mock.Mock(side_effect=populate_dst_dir) - - with mock.patch.dict("sys.modules", git=m): - yield m + def populate_dst_dir(dst_dir): + with open(os.path.join(dst_dir, "train.py"), "w") as f: + f.write(fixture_open("train.py").read()) + with open(os.path.join(dst_dir, "requirements.txt"), "w") as f: + f.write(fixture_open("requirements.txt").read()) + with open(os.path.join(dst_dir, "patch.txt"), "w") as f: + f.write("test") + return mock.Mock() - return mock_specific_branch + m.Repo.init = mock.Mock(side_effect=populate_dst_dir) + with mock.patch.dict("sys.modules", git=m): + yield m @pytest.fixture @@ -1469,21 +1461,109 @@ def test_launch_no_url_job_or_docker_image( assert "Must specify a uri, job or docker image" in str(e) -def test_launch_git_version_master( +""" Gross fixture for explicit branch name -- TODO: fix using parameterization (?)""" + + +@pytest.fixture +def mocked_fetchable_git_repo_master(): + m = mock.Mock() + + def populate_dst_dir(dst_dir): + repo = mock.Mock() + reference = mock.Mock() + reference.name = "master" + repo.references = [reference] + + def create_remote(o, r): + origin = mock.Mock() + origin.refs = {"master": mock.Mock()} + return origin + + repo.create_remote = create_remote + repo.heads = {"master": mock.Mock()} + with open(os.path.join(dst_dir, "train.py"), "w") as f: + f.write(fixture_open("train.py").read()) + with open(os.path.join(dst_dir, "requirements.txt"), "w") as f: + f.write(fixture_open("requirements.txt").read()) + with open(os.path.join(dst_dir, "patch.txt"), "w") as f: + f.write("test") + return repo + + m.Repo.init = mock.Mock(side_effect=populate_dst_dir) + with mock.patch.dict("sys.modules", git=m): + yield m + + +@pytest.fixture +def mocked_fetchable_git_repo_main(): + m = mock.Mock() + + def populate_dst_dir(dst_dir): + repo = mock.Mock() + reference = mock.Mock() + reference.name = "main" + repo.references = [reference] + + def create_remote(o, r): + origin = mock.Mock() + origin.refs = {"main": mock.Mock()} + return origin + + repo.create_remote = create_remote + repo.heads = {"main": mock.Mock()} + + with open(os.path.join(dst_dir, "train.py"), "w") as f: + f.write(fixture_open("train.py").read()) + with open(os.path.join(dst_dir, "requirements.txt"), "w") as f: + f.write(fixture_open("requirements.txt").read()) + with open(os.path.join(dst_dir, "patch.txt"), "w") as f: + f.write("test") + return repo + + m.Repo.init = mock.Mock(side_effect=populate_dst_dir) + with mock.patch.dict("sys.modules", git=m): + yield m + + +def test_launch_git_version_branch_set( + live_mock_server, test_settings, mocked_fetchable_git_repo_master, mock_load_backend +): + api = wandb.sdk.internal.internal_api.Api( + default_settings=test_settings, load_settings=False + ) + mock_with_run_info = launch.run( + api=api, uri="https://foo:bar@github.com/FooTest/Foo.git", version="foobar" + ) + + assert "foobar" in str(mock_with_run_info.args[0].git_version) + + +def test_launch_git_version_default_master( + live_mock_server, test_settings, mocked_fetchable_git_repo_master, mock_load_backend +): + api = wandb.sdk.internal.internal_api.Api( + default_settings=test_settings, load_settings=False + ) + mock_with_run_info = launch.run( + api=api, + uri="https://foo:bar@github.com/FooTest/Foo.git", + ) + + assert "master" in str(mock_with_run_info.args[0].git_version) + + +def test_launch_git_version_default_main( live_mock_server, test_settings, - mocked_fetchable_git_repo, + mocked_fetchable_git_repo_main, + mock_load_backend, ): api = wandb.sdk.internal.internal_api.Api( default_settings=test_settings, load_settings=False ) - with mocked_fetchable_git_repo("master"): - try: - launch.run( - api=api, - uri="https://foo:bar@github.com/FooTest/Foo.git", - job=None, - project="new-test", - ) - except wandb.errors.LaunchError as e: - assert "No git branch passed. Defaulted to branch: master" in str(e) + mock_with_run_info = launch.run( + api=api, + uri="https://foo:bar@github.com/FooTest/Foo.git", + ) + + assert "main" in str(mock_with_run_info.args[0].git_version) diff --git a/wandb/sdk/launch/_project_spec.py b/wandb/sdk/launch/_project_spec.py index 117cdeb4a6e..7fd562d45a1 100644 --- a/wandb/sdk/launch/_project_spec.py +++ b/wandb/sdk/launch/_project_spec.py @@ -287,11 +287,13 @@ def _fetch_project_local(self, internal_api: Api) -> None: raise LaunchError( "Reproducing a run requires either an associated git repo or a code artifact logged with `run.log_code()`" ) - utils._fetch_git_repo( + branch_name = utils._fetch_git_repo( self.project_dir, run_info["git"]["remote"], run_info["git"]["commit"], ) + if self.git_version is None: + self.git_version = branch_name patch = utils.fetch_project_diff( source_entity, source_project, source_run_name, internal_api ) @@ -357,7 +359,11 @@ def _fetch_project_local(self, internal_api: Api) -> None: "Entry point for repo not specified, defaulting to python main.py" ) self.add_entry_point(["python", "main.py"]) - utils._fetch_git_repo(self.project_dir, self.uri, self.git_version) + branch_name = utils._fetch_git_repo( + self.project_dir, self.uri, self.git_version + ) + if self.git_version is None: + self.git_version = branch_name class EntryPoint: diff --git a/wandb/sdk/launch/utils.py b/wandb/sdk/launch/utils.py index 68e03eda62f..f4d26d08503 100644 --- a/wandb/sdk/launch/utils.py +++ b/wandb/sdk/launch/utils.py @@ -411,7 +411,7 @@ def apply_patch(patch_string: str, dst_dir: str) -> None: raise wandb.Error("Failed to apply diff.patch associated with run.") -def _fetch_git_repo(dst_dir: str, uri: str, version: Optional[str]) -> None: +def _fetch_git_repo(dst_dir: str, uri: str, version: Optional[str]) -> str: """Clones the git repo at ``uri`` into ``dst_dir``. checks out commit ``version`` (or defaults to the head commit of the repository's @@ -438,8 +438,6 @@ def _fetch_git_repo(dst_dir: str, uri: str, version: Optional[str]) -> None: ) else: if repo.getattr("references", None) is not None: - print(repo) - print(repo.references) branches = [ref.name for ref in repo.references] else: branches = [] @@ -453,12 +451,6 @@ def _fetch_git_repo(dst_dir: str, uri: str, version: Optional[str]) -> None: repo.create_head(version, origin.refs[version]) repo.heads[version].checkout() wandb.termlog(f"No git branch passed. Defaulted to branch: {version}") - except git.exc.GitCommandError as e: - raise LaunchError( - "Unable to checkout version '%s' of git repo %s" - "- please ensure that the version exists in the repo. \n" - "Error: %s" % (version, uri, e) - ) except (AttributeError, IndexError) as e: raise LaunchError( "Unable to checkout default version '%s' of git repo %s " @@ -467,6 +459,7 @@ def _fetch_git_repo(dst_dir: str, uri: str, version: Optional[str]) -> None: ) repo.submodule_update(init=True, recursive=True) + return version def merge_parameters( From 43388cc168d68b3b73c3158634be6e6af7e65f46 Mon Sep 17 00:00:00 2001 From: gtarpenning Date: Thu, 11 Aug 2022 16:52:19 -0700 Subject: [PATCH 5/7] fixed broken tests LOL --- .../tests_launch/test_launch.py | 52 ++++++------------- 1 file changed, 16 insertions(+), 36 deletions(-) diff --git a/tests/unit_tests_old/tests_launch/test_launch.py b/tests/unit_tests_old/tests_launch/test_launch.py index a8c143fcea1..12af3753808 100644 --- a/tests/unit_tests_old/tests_launch/test_launch.py +++ b/tests/unit_tests_old/tests_launch/test_launch.py @@ -38,13 +38,25 @@ def mocked_fetchable_git_repo(): m = mock.Mock() def populate_dst_dir(dst_dir): + repo = mock.Mock() + reference = mock.Mock() + reference.name = "master" + repo.references = [reference] + + def create_remote(o, r): + origin = mock.Mock() + origin.refs = {"master": mock.Mock()} + return origin + + repo.create_remote = create_remote + repo.heads = {"master": mock.Mock()} with open(os.path.join(dst_dir, "train.py"), "w") as f: f.write(fixture_open("train.py").read()) with open(os.path.join(dst_dir, "requirements.txt"), "w") as f: f.write(fixture_open("requirements.txt").read()) with open(os.path.join(dst_dir, "patch.txt"), "w") as f: f.write("test") - return mock.Mock() + return repo m.Repo.init = mock.Mock(side_effect=populate_dst_dir) with mock.patch.dict("sys.modules", git=m): @@ -1461,41 +1473,9 @@ def test_launch_no_url_job_or_docker_image( assert "Must specify a uri, job or docker image" in str(e) -""" Gross fixture for explicit branch name -- TODO: fix using parameterization (?)""" - - -@pytest.fixture -def mocked_fetchable_git_repo_master(): - m = mock.Mock() - - def populate_dst_dir(dst_dir): - repo = mock.Mock() - reference = mock.Mock() - reference.name = "master" - repo.references = [reference] - - def create_remote(o, r): - origin = mock.Mock() - origin.refs = {"master": mock.Mock()} - return origin - - repo.create_remote = create_remote - repo.heads = {"master": mock.Mock()} - with open(os.path.join(dst_dir, "train.py"), "w") as f: - f.write(fixture_open("train.py").read()) - with open(os.path.join(dst_dir, "requirements.txt"), "w") as f: - f.write(fixture_open("requirements.txt").read()) - with open(os.path.join(dst_dir, "patch.txt"), "w") as f: - f.write("test") - return repo - - m.Repo.init = mock.Mock(side_effect=populate_dst_dir) - with mock.patch.dict("sys.modules", git=m): - yield m - - @pytest.fixture def mocked_fetchable_git_repo_main(): + """Gross fixture for explicit branch name -- TODO: fix using parameterization (?)""" m = mock.Mock() def populate_dst_dir(dst_dir): @@ -1526,7 +1506,7 @@ def create_remote(o, r): def test_launch_git_version_branch_set( - live_mock_server, test_settings, mocked_fetchable_git_repo_master, mock_load_backend + live_mock_server, test_settings, mocked_fetchable_git_repo, mock_load_backend ): api = wandb.sdk.internal.internal_api.Api( default_settings=test_settings, load_settings=False @@ -1539,7 +1519,7 @@ def test_launch_git_version_branch_set( def test_launch_git_version_default_master( - live_mock_server, test_settings, mocked_fetchable_git_repo_master, mock_load_backend + live_mock_server, test_settings, mocked_fetchable_git_repo, mock_load_backend ): api = wandb.sdk.internal.internal_api.Api( default_settings=test_settings, load_settings=False From 16bfe492d8160b295b5ab29a0809013cf3326641 Mon Sep 17 00:00:00 2001 From: gtarpenning Date: Thu, 11 Aug 2022 17:02:04 -0700 Subject: [PATCH 6/7] added one little launchError check --- wandb/sdk/launch/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/wandb/sdk/launch/utils.py b/wandb/sdk/launch/utils.py index f4d26d08503..fad33f400b6 100644 --- a/wandb/sdk/launch/utils.py +++ b/wandb/sdk/launch/utils.py @@ -431,7 +431,7 @@ def _fetch_git_repo(dst_dir: str, uri: str, version: Optional[str]) -> str: try: repo.git.checkout(version) except git.exc.GitCommandError as e: - raise ExecutionError( + raise LaunchError( "Unable to checkout version '%s' of git repo %s" "- please ensure that the version exists in the repo. " "Error: %s" % (version, uri, e) From bb60d6a661c60b3857661bdfe115123092ce4e6b Mon Sep 17 00:00:00 2001 From: gtarpenning Date: Fri, 12 Aug 2022 09:28:09 -0700 Subject: [PATCH 7/7] sneaky flake8 error --- wandb/sdk/launch/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/wandb/sdk/launch/utils.py b/wandb/sdk/launch/utils.py index fad33f400b6..17861645bd4 100644 --- a/wandb/sdk/launch/utils.py +++ b/wandb/sdk/launch/utils.py @@ -10,7 +10,7 @@ import wandb from wandb import util from wandb.apis.internal import Api -from wandb.errors import CommError, ExecutionError, LaunchError +from wandb.errors import CommError, LaunchError if TYPE_CHECKING: # pragma: no cover from wandb.apis.public import Artifact as PublicArtifact