Add pytype as type checking linter (#221)

* add pytype as dev requirement pytype doesn't support Windows currently, so create a new dev group for things like linting tools which will run in the CI. Signed-off-by: Spencer Schrock <sschrock@google.com> * add dev requirements to dep pinning workflow Signed-off-by: Spencer Schrock <sschrock@google.com> * add pytype job Signed-off-by: Spencer Schrock <sschrock@google.com> * fix path type error We previously passed in a string, but the hashing functions expect a pathlib.Path Signed-off-by: Spencer Schrock <sschrock@google.com> * specify DFS serializers produce a digest manifest The tests expected a digest to exist, so the type should probably reflect that. Signed-off-by: Spencer Schrock <sschrock@google.com> * add linting steps Signed-off-by: Spencer Schrock <sschrock@google.com> --------- Signed-off-by: Spencer Schrock <sschrock@google.com>
sigstore · Jun 24, 2024 · cad0a5c · cad0a5c
1 parent 3e010b5
commit cad0a5c
Show file tree

Hide file tree

Showing 9 changed files with 627 additions and 34 deletions.
diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml
@@ -55,3 +55,20 @@ jobs:
             echo "::error Found $failed whitespace errors, failing"
             exit 1
           fi
+  pytype-lint:
+    runs-on: ubuntu-latest
+    name: Type Check
+    steps:
+      - name: Check out source repository
+        uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7
+      - name: Set up Python environment
+        uses: actions/setup-python@82c7e631bb3cdc910f68e0081d67478d79c6982d # v5.1.0
+        with:
+          python-version: "3.11"
+      - name: run pytype
+        run: |
+          python -m venv venv
+          .github/workflows/scripts/venv_activate.sh
+          pip install -r model_signing/install/requirements_test_Linux.txt
+          pip install -r model_signing/install/requirements_dev_Linux.txt
+          pytype --keep-going model_signing/{hashing,manifest,serialization}
diff --git a/.github/workflows/pin_deps.yml b/.github/workflows/pin_deps.yml
@@ -34,6 +34,7 @@ jobs:
         cache-dependency-path: |
           model_signing/install/requirements_${{ matrix.os_family }}.txt
           model_signing/install/requirements_test_${{ matrix.os_family }}.txt
+          model_signing/install/requirements_dev_${{ matrix.os_family }}.txt
           slsa_for_models/install/requirements_${{ matrix.os_family }}.txt
     - name: Create an empty virtualenv and install `pip-tools`
       run: |
@@ -49,6 +50,10 @@ jobs:
         pip-compile --upgrade --generate-hashes --strip-extras --output-file=model_signing/install/requirements_${{ matrix.os_family }}.txt model_signing/install/requirements.in
         pip-compile --upgrade --generate-hashes --strip-extras --output-file=model_signing/install/requirements_test_${{ matrix.os_family }}.txt model_signing/install/requirements_test.in
         pip-compile --upgrade --generate-hashes --strip-extras --output-file=slsa_for_models/install/requirements_${{ matrix.os_family }}.txt slsa_for_models/install/requirements.in
+        # pytype doesn't support Windows
+        if [[ "${{ matrix.os_family }}" != "Windows" ]]; then
+          pip-compile --upgrade --generate-hashes --strip-extras --output-file=model_signing/install/requirements_dev_${{ matrix.os_family }}.txt model_signing/install/requirements_dev.in
+        fi
     - name: Test freeze file (for model signing)
       run: |
         set -exuo pipefail
@@ -65,6 +70,15 @@ jobs:
         .github/workflows/scripts/venv_activate.sh
         pip install -r model_signing/install/requirements_test_${{ matrix.os_family }}.txt
         pip list  # For debugging
+    - name: Test freeze file (for dev tools model signing)
+      if: ${{ matrix.os_family != 'Windows' }} # pytype doesn't support Windows
+      run: |
+        set -exuo pipefail
+        rm -rf venv  # Need clean sandbox
+        python -m venv venv
+        .github/workflows/scripts/venv_activate.sh
+        pip install -r model_signing/install/requirements_dev_${{ matrix.os_family }}.txt
+        pip list  # For debugging
     - name: Test freeze file (for SLSA for models)
       run: |
         set -exuo pipefail

diff --git a/model_signing/README.md b/model_signing/README.md
@@ -222,3 +222,25 @@ Machine M2: Debian 5.10.1 x86_64 GNU/Linux, 4GB RAM, 2 vCPUs, 56320 KB, Intel(R)
 | H2 | bertseq2seq          | 2.8G  | 13.8s | 25.9s |
 | H2 | bert-base-uncased    | 3.3G  | 22.7s | 23.3s |
 | H2 | tiiuae/falcon-7b     | 14GB  | 2m.1s | 2m3s  |
+
+## Development steps
+
+### Linting
+
+`model_signing` is automatically linted and formatted with a collection of tools:
+
+* [flake8](https://github.com/PyCQA/flake8)
+* [pytype](https://github.com/google/pytype)
+
+You can run the type checker locally by installing the `dev` dependencies:
+```shell
+python3 -m venv dev_env
+source dev_env/bin/activate
+os=Linux # Supported: Linux, Darwin.
+python3 -m pip install --require-hashes -r "install/requirements_dev_${os}".txt
+```
+
+Then point pytype at the desired module or package:
+```shell
+pytype --keep-going model_signing/hashing
+```
diff --git a/model_signing/hashing/file_test.py b/model_signing/hashing/file_test.py
@@ -14,6 +14,8 @@
 
 import pytest
 
+from pathlib import Path
+
 from model_signing.hashing import file
 from model_signing.hashing import memory
 
@@ -23,6 +25,7 @@
 _CONTENT: str = "text."  # note that these have the same length
 _FULL_CONTENT = _HEADER + _CONTENT
 _SHARD_SIZE = len(_HEADER)
+_UNUSED_PATH = Path("unused")
 
 
 @pytest.fixture(scope="session")
@@ -66,7 +69,7 @@ class TestFileHasher:
 
     def test_fails_with_negative_chunk_size(self):
         with pytest.raises(ValueError, match="Chunk size must be non-negative"):
-            file.FileHasher("unused", memory.SHA256(), chunk_size=-2)
+            file.FileHasher(_UNUSED_PATH, memory.SHA256(), chunk_size=-2)
 
     def test_hash_of_known_file(self, sample_file, expected_digest):
         hasher = file.FileHasher(sample_file, memory.SHA256())
@@ -119,12 +122,12 @@ def test_set_file(self, sample_file, sample_file_content_only):
         assert digest1.digest_value == digest2.digest_value
 
     def test_default_digest_name(self):
-        hasher = file.FileHasher("unused", memory.SHA256())
+        hasher = file.FileHasher(_UNUSED_PATH, memory.SHA256())
         assert hasher.digest_name == "file-sha256"
 
     def test_override_digest_name(self):
         hasher = file.FileHasher(
-            "unused",
+            _UNUSED_PATH,
             memory.SHA256(),
             chunk_size=10,
             digest_name_override="test-hash",
@@ -149,18 +152,20 @@ def test_fails_with_negative_shard_size(self):
             ValueError, match="Shard size must be strictly positive"
         ):
             file.ShardedFileHasher(
-                "unused", memory.SHA256(), shard_size=-2, start=0, end=42
+                _UNUSED_PATH, memory.SHA256(), shard_size=-2, start=0, end=42
             )
 
     def test_fails_with_negative_start(self):
         with pytest.raises(
             ValueError, match="File start offset must be non-negative"
         ):
-            file.ShardedFileHasher("unused", memory.SHA256(), start=-2, end=42)
+            file.ShardedFileHasher(
+                _UNUSED_PATH, memory.SHA256(), start=-2, end=42
+            )
 
     def test_set_fails_with_negative_start(self):
         hasher = file.ShardedFileHasher(
-            "unused", memory.SHA256(), start=0, end=42
+            _UNUSED_PATH, memory.SHA256(), start=0, end=42
         )
         with pytest.raises(
             ValueError, match="File start offset must be non-negative"
@@ -174,11 +179,13 @@ def test_fails_with_end_lower_than_start(self):
                 "File end offset must be stricly higher that file start offset"
             ),
         ):
-            file.ShardedFileHasher("unused", memory.SHA256(), start=42, end=2)
+            file.ShardedFileHasher(
+                _UNUSED_PATH, memory.SHA256(), start=42, end=2
+            )
 
     def test_set_fails_with_end_lower_than_start(self):
         hasher = file.ShardedFileHasher(
-            "unused", memory.SHA256(), start=0, end=42
+            _UNUSED_PATH, memory.SHA256(), start=0, end=42
         )
         with pytest.raises(
             ValueError,
@@ -195,11 +202,13 @@ def test_fails_with_zero_read_span(self):
                 "File end offset must be stricly higher that file start offset"
             ),
         ):
-            file.ShardedFileHasher("unused", memory.SHA256(), start=42, end=42)
+            file.ShardedFileHasher(
+                _UNUSED_PATH, memory.SHA256(), start=42, end=42
+            )
 
     def test_set_fails_with_zero_read_span(self):
         hasher = file.ShardedFileHasher(
-            "unused", memory.SHA256(), start=0, end=42
+            _UNUSED_PATH, memory.SHA256(), start=0, end=42
         )
         with pytest.raises(
             ValueError,
@@ -214,12 +223,12 @@ def test_fails_with_read_span_too_large(self):
             ValueError, match="Must not read more than shard_size=2"
         ):
             file.ShardedFileHasher(
-                "unused", memory.SHA256(), start=0, end=42, shard_size=2
+                _UNUSED_PATH, memory.SHA256(), start=0, end=42, shard_size=2
             )
 
     def test_set_fails_with_read_span_too_large(self):
         hasher = file.ShardedFileHasher(
-            "unused", memory.SHA256(), start=0, end=2, shard_size=2
+            _UNUSED_PATH, memory.SHA256(), start=0, end=2, shard_size=2
         )
         with pytest.raises(
             ValueError, match="Must not read more than shard_size=2"
@@ -365,13 +374,13 @@ def test_hash_of_known_file_large_shard(
 
     def test_default_digest_name(self):
         hasher = file.ShardedFileHasher(
-            "unused", memory.SHA256(), start=0, end=2, shard_size=10
+            _UNUSED_PATH, memory.SHA256(), start=0, end=2, shard_size=10
         )
         assert hasher.digest_name == "file-sha256-10"
 
     def test_override_digest_name(self):
         hasher = file.ShardedFileHasher(
-            "unused",
+            _UNUSED_PATH,
             memory.SHA256(),
             start=0,
             end=2,

diff --git a/model_signing/install/requirements_dev.in b/model_signing/install/requirements_dev.in
@@ -0,0 +1 @@
+pytype