diff --git a/.github/workflows/end2endtest.yml b/.github/workflows/end2endtest.yml index 2576bbf..f0fdc56 100644 --- a/.github/workflows/end2endtest.yml +++ b/.github/workflows/end2endtest.yml @@ -24,7 +24,7 @@ jobs: steps: - uses: actions/checkout@v2 - name: Set Up Python ${{ matrix.python-version }} - uses: actions/setup-python@v2 + uses: actions/setup-python@v4 with: python-version: ${{ matrix.python-version }} cache: "pip" diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 2e73999..c2ed3ad 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -21,7 +21,7 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v2 - - uses: actions/setup-python@v2 + - uses: actions/setup-python@v4 with: python-version: 3.8 # Install package and deps so third-party packages are sorted diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index c422394..4453cc9 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -11,7 +11,7 @@ jobs: steps: - uses: actions/checkout@v2 - name: Setup Python - uses: actions/setup-python@v2 + uses: actions/setup-python@v4 with: python-version: "3.8" - name: Install Build Package diff --git a/.github/workflows/unittest.yml b/.github/workflows/unittest.yml index dbc1a42..5cedf6e 100644 --- a/.github/workflows/unittest.yml +++ b/.github/workflows/unittest.yml @@ -24,7 +24,7 @@ jobs: steps: - uses: actions/checkout@v2 - name: Set Up Python ${{ matrix.python-version }} - uses: actions/setup-python@v2 + uses: actions/setup-python@v4 with: python-version: ${{ matrix.python-version }} cache: "pip" diff --git a/git_theta/git_utils.py b/git_theta/git_utils.py index af27fba..51f0640 100644 --- a/git_theta/git_utils.py +++ b/git_theta/git_utils.py @@ -1,5 +1,7 @@ """Utilities for manipulating git.""" +import copy +import dataclasses import filecmp import fnmatch import io @@ -10,7 +12,7 @@ import shutil import subprocess import sys -from typing import List, Sequence, Union +from typing import Dict, List, Optional, Sequence, Union import git @@ -26,6 +28,11 @@ from git_theta import async_utils +# These are the git attributes that git-theta currently uses to manage checked-in +# files. Defined as a variable in case extra functionality ever requires more +# attributes. +THETA_ATTRIBUTES = ("filter", "merge", "diff") + def get_git_repo(): """ @@ -107,7 +114,26 @@ def get_gitattributes_file(repo): return os.path.join(repo.working_dir, ".gitattributes") -def read_gitattributes(gitattributes_file): +@dataclasses.dataclass +class GitAttributes: + """Git attributes for a file that matches pattern.""" + + pattern: str + attributes: Dict[str, str] + raw: Optional[str] = None + + def __str__(self): + if self.raw: + return self.raw + attrs = " ".join(f"{k}={v}" if v else k for k, v in self.attributes.items()) + return f"{self.pattern} {attrs}" + + def __eq__(self, o): + raw_eq = self.raw == o.raw if self.raw and o.raw else True + return self.pattern == o.pattern and self.attributes == o.attributes and raw_eq + + +def read_gitattributes(gitattributes_file) -> List[GitAttributes]: """ Read contents of this repo's .gitattributes file @@ -123,14 +149,33 @@ def read_gitattributes(gitattributes_file): """ if os.path.exists(gitattributes_file): with open(gitattributes_file, "r") as f: - return [line.rstrip("\n") for line in f] + return [parse_gitattributes(line.rstrip("\n")) for line in f] else: return [] +def parse_gitattributes(gitattributes: str) -> GitAttributes: + # TODO: Fix for escaped patterns + pattern, *attributes = gitattributes.split(" ") + attrs = {} + # Overwrite as we go to get the LAST attribute behavior + for attribute in attributes: + if "=" in attribute: + key, value = attribute.split("=") + # TODO: Update to handle unsetting attributes like "-diff". Currently we + # just copy then as keys for printing but don't check their semantics, + # for example a file with an unset diff does currently throw an error + # when adding git-theta tracking. + else: + key = attribute + value = None + attrs[key] = value + return GitAttributes(pattern, attrs, gitattributes) + + @file_or_name(gitattributes_file="w") def write_gitattributes( - gitattributes_file: Union[str, io.FileIO], attributes: List[str] + gitattributes_file: Union[str, io.FileIO], attributes: List[GitAttributes] ): """ Write list of attributes to this repo's .gitattributes file @@ -143,60 +188,112 @@ def write_gitattributes( attributes: Attributes to write to .gitattributes """ - gitattributes_file.write("\n".join(attributes)) + gitattributes_file.write("\n".join(map(str, attributes))) # End file with newline. gitattributes_file.write("\n") -def add_theta_to_gitattributes(gitattributes: List[str], path: str) -> str: - """Add a filter=theta that covers file_name. +def add_theta_to_gitattributes( + gitattributes: List[GitAttributes], + path: str, + theta_attributes: Sequence[str] = THETA_ATTRIBUTES, +) -> List[GitAttributes]: + """Add git attributes required by git-theta for path. + + If there is a pattern that covers the current file that applies the git-theta + attributes, no new pattern is added. If there is a pattern that covers the + current file and sets attributes used by git-theta an error is raised. If + there is a pattern that sets non-overlapping attributes they are copied into + a new path-specific pattern. If there is no match, a new path-specific + pattern is always created. Parameters ---------- - gitattributes: A list of the lines from the gitattribute files. + gitattributes: A list of parsed git attribute entries. path: The path to the model we are adding a filter to. + Raises + ------ + ValueError + `path` is covered by an active git attributes entry that sets merge, + filter, or diff to a value other than "theta". + Returns ------- - List[str] - The lines to write to the new gitattribute file with a (possibly) new - filter=theta added that covers the given file. + List[GitAttributes] + The git attributes write to the new gitattribute file with a (possibly) + new (filter|merge|diff)=theta added that covers `path`. """ - pattern_found = False - new_gitattributes = [] - for line in gitattributes: - # TODO(bdlester): Revisit this regex to see if it when the pattern - # is escaped due to having spaces in it. - match = re.match(r"^\s*(?P[^\s]+)\s+(?P.*)$", line) - if match: - # If there is already a pattern that covers the file, add the filter - # to that. - if fnmatch.fnmatchcase(path, match.group("pattern")): - pattern_found = True - if not "filter=theta" in match.group("attributes"): - line = f"{line.rstrip()} filter=theta" - if not "merge=theta" in match.group("attributes"): - line = f"{line.rstrip()} merge=theta" - if not "diff=theta" in match.group("attributes"): - line = f"{line.rstrip()} diff=theta" - new_gitattributes.append(line) - # If we don't find a matching pattern, add a new line that covers just this - # specific file. - if not pattern_found: - new_gitattributes.append(f"{path} filter=theta merge=theta diff=theta") - return new_gitattributes - - -def get_gitattributes_tracked_patterns(gitattributes_file): + previous_attribute = None + # Find if an active gitattribute entry applies to path + for gitattribute in gitattributes[::-1]: + if fnmatch.fnmatchcase(path, gitattribute.pattern): + previous_attribute = gitattribute + break + # If path is already managed by a git attributes entry. + if previous_attribute: + # If all of the theta attributes are set, we don't do anything. + if all( + previous_attribute.attributes.get(attr) == "theta" + for attr in theta_attributes + ): + return gitattributes + # If any of the attributes theta uses is set to something else, error out. + if any( + attr in previous_attribute.attributes + and previous_attribute.attributes[attr] != "theta" + for attr in theta_attributes + ): + raise ValueError( + f"Git Attributes used by git-theta are already set for {path}. " + f"Found filter={previous_attribute.attributes.get('filter')}, " + f"diff={previous_attribute.attributes.get('diff')}, " + f"merge={previous_attribute.attributes.get('merge')}." + ) + # If the old entry set other attributes, make sure they are preserved. + attributes = ( + copy.deepcopy(previous_attribute.attributes) if previous_attribute else {} + ) + for attr in theta_attributes: + attributes[attr] = "theta" + new_attribute = GitAttributes(path, attributes) + gitattributes.append(new_attribute) + return gitattributes + + +def get_gitattributes_tracked_patterns( + gitattributes_file, theta_attributes: Sequence[str] = THETA_ATTRIBUTES +): gitattributes = read_gitattributes(gitattributes_file) theta_attributes = [ - attribute for attribute in gitattributes if "filter=theta" in attribute + attr + for attr in gitattributes + if attr.attributes.get(a) == "theta" + for a in theta_attributes ] + return [attr.pattern for attr in theta_attributes] # TODO: Correctly handle patterns with escaped spaces in them patterns = [attribute.split(" ")[0] for attribute in theta_attributes] return patterns +def is_theta_tracked( + path: str, + gitattributes: List[GitAttributes], + theta_attributes: Sequence[str] = THETA_ATTRIBUTES, +) -> bool: + """Check if `path` is tracked by git-theta based on `.gitattributes`. + + Note: The last line that matches in .gitattributes is the active one so + start from the end. If the first match (really last) does not have the + theta filter active then the file is not tracked by Git-Theta. + """ + for attr in gitattributes[::-1]: + if fnmatch.fnmatchcase(path, attr.pattern): + return all(attr.attributes.get(a) == "theta" for a in theta_attributes) + return False + + def add_file(f, repo): """ Add file to git staging area diff --git a/git_theta/scripts/git_theta.py b/git_theta/scripts/git_theta.py index fb379aa..86ecba8 100755 --- a/git_theta/scripts/git_theta.py +++ b/git_theta/scripts/git_theta.py @@ -80,12 +80,12 @@ def post_commit(args): theta_commits = theta.ThetaCommits(repo) gitattributes_file = git_utils.get_gitattributes_file(repo) - patterns = git_utils.get_gitattributes_tracked_patterns(gitattributes_file) + gitattributes = git_utils.read_gitattributes(gitattributes_file) oids = set() commit = repo.commit("HEAD") for path in commit.stats.files.keys(): - if any([fnmatch.fnmatchcase(path, pattern) for pattern in patterns]): + if git_utils.is_theta_tracked(path, gitattributes): curr_metadata = metadata.Metadata.from_file(commit.tree[path].data_stream) prev_metadata = metadata.Metadata.from_commit(repo, path, "HEAD~1") diff --git a/tests/git_utils_test.py b/tests/git_utils_test.py index ad8e704..37d7aaf 100644 --- a/tests/git_utils_test.py +++ b/tests/git_utils_test.py @@ -8,62 +8,182 @@ def test_add_theta_gitattributes_empty_file(): - assert git_utils.add_theta_to_gitattributes([], "example") == [ + assert list(map(str, git_utils.add_theta_to_gitattributes([], "example"))) == [ "example filter=theta merge=theta diff=theta" ] +def test_is_theta_tracked_with_override(): + attrs = [ + git_utils.parse_gitattributes(a) + for a in ( + "mymodel.pt filter=theta merge=theta diff=theta", + "*.pt filter=theta merge=theta diff=theta", + ) + ] + print(attrs) + assert git_utils.is_theta_tracked("mymodel.pt", attrs) + + +def test_is_theta_tracked_with_override_false(): + attrs = [ + git_utils.parse_gitattributes(a) + for a in ( + "mymodel.pt filter=theta merge=theta diff=theta", + "*.pt filter=lfs merge=theta diff=lfs", + ) + ] + assert git_utils.is_theta_tracked("mymodel.pt", attrs) == False + + +def test_is_theta_tracked_no_lines(): + assert git_utils.is_theta_tracked("mymodel.pt", []) == False + + +def test_is_theta_tracked_no_attrs(): + assert ( + git_utils.is_theta_tracked( + "mymodel.pt", [git_utils.parse_gitattributes("mymodel.pt")] + ) + == False + ) + + +def test_is_theta_tracked_with_following_filter(): + attrs = [ + git_utils.parse_gitattributes(a) + for a in ( + "mymodel.pt filter=theta merge=theta diff=theta", + "*.pt filter=theta filter=lfs merge=lfs diff=lfs", + ) + ] + assert git_utils.is_theta_tracked("mymodel.pt", attrs) == False + + +def test_parse_gitattributes_uses_last(): + attr = git_utils.parse_gitattributes("example.txt merge=theta merge=wrong") + assert attr.attributes["merge"] == "wrong" + + +def test_parse_gitattributes_no_equal(): + s = "example.txt thing" + attr = git_utils.parse_gitattributes(s) + assert attr.attributes == {"thing": None} + assert str(attr) == s + + +def test_parse_gitattributes_raw_string(): + og_string = "example.txt merge=theta merge=wrong" + attr = git_utils.parse_gitattributes(og_string) + assert str(attr) == og_string + attr.raw = None + assert str(attr) == "example.txt merge=wrong" + + def test_add_theta_gitattributes_no_match(): + # Should add a new path atts = [ - "Some-other-path filter=lfs", - "*-cool-models.pt filter=theta merge=theta diff=theta", + git_utils.parse_gitattributes(a) + for a in ( + "Some-other-path filter=lfs", + "*-cool-models.pt filter=theta merge=theta diff=theta", + ) ] model_path = "path/to/my/model.pt" assert ( - git_utils.add_theta_to_gitattributes(atts, model_path)[-1] + str(git_utils.add_theta_to_gitattributes(atts, model_path)[-1]) == f"{model_path} filter=theta merge=theta diff=theta" ) -def test_add_theta_gitattributes_exact_match(): +def test_add_theta_gitattributes_exact_match_with_conflicting_attributes(): model_path = "really/cool/model/yall.ckpt" - atts = [f"{model_path} filter=lfs"] - assert ( - git_utils.add_theta_to_gitattributes(atts, model_path)[-1] - == f"{model_path} filter=lfs filter=theta merge=theta diff=theta" - ) + atts = [git_utils.parse_gitattributes(f"{model_path} filter=lfs")] + with pytest.raises(ValueError): + new_attributes = git_utils.add_theta_to_gitattributes(atts, model_path) -def test_add_theta_gitattributes_pattern_match(): +def test_add_theta_gitattributes_pattern_match_with_conflicting_attributes(): model_path = "literal-the-best-checkpoint.pt" - atts = ["*.pt thing"] - assert ( - git_utils.add_theta_to_gitattributes(atts, model_path)[-1] - == f"*.pt thing filter=theta merge=theta diff=theta" - ) + atts = [git_utils.parse_gitattributes("*.pt thing merge=lfs")] + with pytest.raises(ValueError): + new_attributes = git_utils.add_theta_to_gitattributes(atts, model_path) + +def test_add_theta_gitattributes_exact_match_disjoint_attributes(): + # Should create a new attribute with values copied over + model_path = "my-test_model" + atts = [ + git_utils.parse_gitattributes(a) + for a in ("my-test_model merge=theta diff=theta banana=fruit",) + ] + new_att = git_utils.add_theta_to_gitattributes(atts, model_path)[-1] + assert new_att.attributes["banana"] == "fruit" + assert new_att.attributes["filter"] == "theta" + + +def test_add_theta_gitattributes_pattern_disjoint_attributes(): + # Should create a new attribute with values copied over + model_path = "my-test_model" + atts = [ + git_utils.parse_gitattributes(a) + for a in ("my-test* merge=theta diff=theta banana=fruit",) + ] + new_att = git_utils.add_theta_to_gitattributes(atts, model_path)[-1] + assert new_att.pattern == model_path + assert new_att.attributes["banana"] == "fruit" + assert new_att.attributes["filter"] == "theta" -def test_add_theta_gitattributes_multiple_matches(): + +def test_add_theta_gitattributes_disjoint_attributes_multiple_matches(): + # Should create a new attribute with values copied over model_path = "100-on-mnist.npy" - atts = ["*.npy other-filter", f"{model_path} other-filter"] - assert git_utils.add_theta_to_gitattributes(atts, model_path) == [ - f"{attr} filter=theta merge=theta diff=theta" for attr in atts + atts = [ + git_utils.parse_gitattributes(a) + for a in ("*.npy other-filter", f"{model_path} target-filter") ] + new_attributes = git_utils.add_theta_to_gitattributes(atts, model_path) + # Note: target-filter is expected rather than other-filter because the *last* + # filter in the file is the active one. + assert ( + str(new_attributes[-1]) + == f"{model_path} target-filter filter=theta merge=theta diff=theta" + ) def test_add_theta_gitattributes_match_with_theta_already(): + # Should be a no-op model_path = "my-bad-model.chkp" - atts = ["my-*-model.chkp filter=theta merge=theta diff=theta"] - assert git_utils.add_theta_to_gitattributes(atts, model_path) == atts + atts = [ + git_utils.parse_gitattributes(a) + for a in ( + "my-*-model.chkp filter=theta merge=theta diff=theta", + "example.txt thing", + ) + ] + new_attributes = git_utils.add_theta_to_gitattributes(atts, model_path) + assert new_attributes == atts + + +# This should fail until unsetting attributes are handled. +@pytest.mark.xfail +def test_add_theta_gitattributes_unset_diff(): + # The attribute represention (the dict) my change when unsetting is implemented. + attr = git_utils.GitAttributes("example.pt", {"-diff": None}) + with pytest.raises(ValueError): + new_attributes = git_utils.add_theta_to_gitattributes([attr], "example.pt") def test_add_theta_gitattributes_rest_unchanged(): model_path = "model-v3.pt" atts = [ - "some-other-path filter=theta merge=theta diff=theta", - "really-reaaaally-big-files filter=lfs", - r"model-v\d.pt filter", - "another filter=theta merge=theta diff=theta", + git_utils.parse_gitattributes(a) + for a in ( + "some-other-path filter=theta merge=theta diff=theta", + "really-reaaaally-big-files filter=lfs", + r"model-v\d.pt filter", + "another filter=theta merge=theta diff=theta", + ) ] results = git_utils.add_theta_to_gitattributes(atts, model_path) for i, (a, r) in enumerate(zip(atts, results)): @@ -78,18 +198,25 @@ def gitattributes(): "*.pt filter=theta merge=theta diff=theta", "*.png filter=lfs", "really-big-file filter=lfs", - "something else, who knows how cool it could be", + "something else", + ], [ + git_utils.GitAttributes( + "*.pt", {"filter": "theta", "merge": "theta", "diff": "theta"} + ), + git_utils.GitAttributes("*.png", {"filter": "lfs"}), + git_utils.GitAttributes("really-big-file", {"filter": "lfs"}), + git_utils.GitAttributes("something", {"else": None}), ] def test_read_gitattributes(gitattributes, tmp_path): """Test that reading gitattributes removes newlines.""" + attributes_text, gitattributes = gitattributes gitattributes_file = tmp_path / ".gitattributes" with open(gitattributes_file, "w") as wf: - wf.write("\n".join(gitattributes)) + wf.write("\n".join(attributes_text)) read_attributes = git_utils.read_gitattributes(gitattributes_file) - for attr in read_attributes: - assert not attr.endswith("\n") + assert read_attributes == gitattributes def test_read_gitattributes_missing_file(tmp_path): @@ -111,6 +238,7 @@ def test_read_gitattributes_empty_file(tmp_path): def test_write_gitattributes(gitattributes, tmp_path): """Test that attributes are written to file unchanged and include newlines.""" + gitattributes = gitattributes[0] attr_file = tmp_path / ".gitattributes" for attr in gitattributes: assert not attr.endswith("\n") @@ -141,8 +269,9 @@ def test_write_gitattributes_creates_file(gitattributes, tmp_path): def test_read_write_gitattributes_write_read_round_trip(gitattributes, tmp_path): """Test that we can write attributes, then read them back and they will match.""" + attributes_text, gitattributes = gitattributes attr_file = tmp_path / ".gitattributes" - git_utils.write_gitattributes(attr_file, gitattributes) + git_utils.write_gitattributes(attr_file, attributes_text) read_attrs = git_utils.read_gitattributes(attr_file) assert read_attrs == gitattributes