Rewrote protobuf generation scripts in Python #12527

Avasam · 2024-08-15T06:13:44Z

This is so much faster on Windows, takes a few seconds to run (including downloads and pre-commit). Compared to the over 1m it used to take me on WSL.

I have two design questions:

Which usage do we prefer:

# A)
python scripts/sync_proto/tensorflow.py

# B)
python scripts/sync_proto.py tensorflow
# Which I could make work with the following for shell autocomplete
python scripts/sync_proto.py ./stubs/tensorflow

Should I add a parametrized main function (something like sync_stubs_with_proto) that takes all of a scripts' special needs as parameters (including a "post-run" Callable). Or keep the 3 scripts separate with shared helper functions.

I'm also open to name changes suggestions.

Avasam · 2024-08-16T19:20:57Z

requirements-tests.txt

@@ -13,6 +13,8 @@ ruff==0.5.4              # must match .pre-commit-config.yaml

 # Libraries used by our various scripts.
 aiohttp==3.10.2
+grpcio-tools


I'm sure there's a minimal version of protoc that is shipped with grpcio-tools that we should be using, but I can't recall off the top of my head and would have to search through past PRs to find what it was.

Avasam · 2024-08-16T19:21:48Z

scripts/sync_protobuf/_helpers.py

+from http.client import HTTPResponse
+from pathlib import Path
+from typing import TYPE_CHECKING, Iterable
+from urllib.request import urlopen


I'm purposefully avoiding requests here, as to not add requests and types-requests in requirements-tests.txt

scripts/sync_protobuf/google_protobuf.py

…to proto-scripts

Avasam · 2024-08-16T19:36:16Z

requirements-tests.txt

+# grpc install only fails on Windows, but let's avoid building sdist on other platforms
+# https://github.com/grpc/grpc/issues/36201
+grpcio-tools; python_version < "3.13"


xref grpc/grpc#36201 & grpc/grpc#34922

…scripts

srittau

Thanks! I'm not really familiar with protobuf or protobuf stubs generation so my comments are limited to general issues.

In general, there are a few places that use / as path separator, so I'd expect problems on Windows, but I'm fine with that for now.

scripts/sync_protobuf/_utils.py

srittau · 2024-09-12T09:17:59Z

scripts/sync_protobuf/google_protobuf.py

+        data: dict[str, dict[str, dict[str, str]]] = json.load(file)
+    # The root key will be the protobuf source code version
+    return next(iter(data.values()))["languages"]["python"]


I'd like to see some validation of the version, considering its coming from an outside source. Something like:

Suggested change

data: dict[str, dict[str, dict[str, str]]] = json.load(file)

# The root key will be the protobuf source code version

return next(iter(data.values()))["languages"]["python"]

data = json.load(file)

# The root key will be the protobuf source code version

version = next(iter(data.values()))["languages"]["python"]

assert isinstance(version, str)

assert re.fullmatch(r"...", version) # proper re here

return version

This way we're also sure (at runtime) that version has the correct type and format.

I feel like validating the version string is unnecessary extra work. If they somehow write an invalid Python version, our script doesn't need to fail. We're not doing anything with it other than displaying it. Proper validation should probably use a Python packaging library (I don't remember which).
The str assertion I still find valuable in case protobuff changes the structure of that file and the value becomes an object (dict)

scripts/sync_protobuf/google_protobuf.py

srittau · 2024-09-12T09:29:06Z

scripts/sync_protobuf/google_protobuf.py

+    temp_dir = Path(tempfile.mkdtemp())
+    # Fetch s2clientprotocol (which contains all the .proto files)
+    archive_path = temp_dir / ARCHIVE_FILENAME
+    download_file(ARCHIVE_URL, archive_path)
+    extract_archive(archive_path, temp_dir)
+
+    # Remove existing pyi
+    for old_stub in STUBS_FOLDER.rglob("*_pb2.pyi"):
+        old_stub.unlink()
+
+    PROTOC_VERSION = run_protoc(
+        proto_paths=(f"{EXTRACTED_PACKAGE_DIR}/src",),
+        mypy_out=STUBS_FOLDER,
+        proto_globs=extract_proto_file_paths(temp_dir),
+        cwd=temp_dir,
+    )
+
+    PYTHON_PROTOBUF_VERSION = extract_python_version(temp_dir / EXTRACTED_PACKAGE_DIR / "version.json")
+
+    # Cleanup after ourselves, this is a temp dir, but it can still grow fast if run multiple times
+    shutil.rmtree(temp_dir)


To make sure the temp directory is always cleaned up:

Suggested change

temp_dir = Path(tempfile.mkdtemp())

# Fetch s2clientprotocol (which contains all the .proto files)

archive_path = temp_dir / ARCHIVE_FILENAME

download_file(ARCHIVE_URL, archive_path)

extract_archive(archive_path, temp_dir)

# Remove existing pyi

for old_stub in STUBS_FOLDER.rglob("*_pb2.pyi"):

old_stub.unlink()

PROTOC_VERSION = run_protoc(

proto_paths=(f"{EXTRACTED_PACKAGE_DIR}/src",),

mypy_out=STUBS_FOLDER,

proto_globs=extract_proto_file_paths(temp_dir),

cwd=temp_dir,

)

PYTHON_PROTOBUF_VERSION = extract_python_version(temp_dir / EXTRACTED_PACKAGE_DIR / "version.json")

# Cleanup after ourselves, this is a temp dir, but it can still grow fast if run multiple times

shutil.rmtree(temp_dir)

with tempfile.TemporaryDirectory() as td:

temp_dir = Path(td)

# Fetch s2clientprotocol (which contains all the .proto files)

archive_path = temp_dir / ARCHIVE_FILENAME

download_file(ARCHIVE_URL, archive_path)

extract_archive(archive_path, temp_dir)

# Remove existing pyi

for old_stub in STUBS_FOLDER.rglob("*_pb2.pyi"):

old_stub.unlink()

PROTOC_VERSION = run_protoc(

proto_paths=(f"{EXTRACTED_PACKAGE_DIR}/src",),

mypy_out=STUBS_FOLDER,

proto_globs=extract_proto_file_paths(temp_dir),

cwd=temp_dir,

)

PYTHON_PROTOBUF_VERSION = extract_python_version(temp_dir / EXTRACTED_PACKAGE_DIR / "version.json")

I did that originally, but it was more annoying to comment out for debugging purposes. Maybe I could do like #12151

srittau · 2024-09-12T09:30:03Z

scripts/sync_protobuf/s2clientprotocol.py

+
+
+def main() -> None:
+    temp_dir = Path(tempfile.mkdtemp())


srittau · 2024-09-12T09:32:16Z

scripts/sync_protobuf/tensorflow.py

+
+
+def main() -> None:
+    temp_dir = Path(tempfile.mkdtemp())


Co-authored-by: Sebastian Rittau <srittau@rittau.biz>

Avasam added 5 commits August 15, 2024 02:12

WIP (s2clientprotocol done)

db53bda

Completed tensorflow script

cdd58f3

Fix CI tests

9f33b0e

Finished google protobuf

c1ba6e5

Merge branch 'main' into proto-scripts

da40ac3

Avasam marked this pull request as ready for review August 16, 2024 19:17

Avasam commented Aug 16, 2024

View reviewed changes

scripts/sync_protobuf/google_protobuf.py Outdated Show resolved Hide resolved

Forgot to delete old script

5f2ec26

Avasam changed the title ~~[WIP] Rewrote protobuf generation scripts in Python~~ Rewrote protobuf generation scripts in Python Aug 16, 2024

Avasam added 2 commits August 16, 2024 15:35

Don't install grpcio-tools on Python 3.13

893a223

Merge branch 'proto-scripts' of https://github.com/Avasam/typeshed in…

b23bbae

…to proto-scripts

Avasam commented Aug 16, 2024

View reviewed changes

Avasam added the project: infrastructure typeshed build, test, documentation, or distribution related label Aug 18, 2024

Avasam added 2 commits August 19, 2024 11:47

Merge branch 'main' of https://github.com/python/typeshed into proto-…

b0a2250

…scripts

Rename _helpers to _utils

75a746c

Avasam mentioned this pull request Sep 7, 2024

[stubsabot] Bump protobuf to 5.28.* #12604

Closed

Merge branch 'main' into proto-scripts

d00e261

srittau requested changes Sep 12, 2024

View reviewed changes

Avasam added the status: needs decision Needs a final decision by the typeshed maintainers label Sep 12, 2024

Avasam mentioned this pull request Sep 18, 2024

Optional pytype install on Windows (no CI) #12669

Merged

Avasam and others added 3 commits September 18, 2024 19:38

Apply suggestions from code review

ae26a36

Co-authored-by: Sebastian Rittau <srittau@rittau.biz>

assert google_protobuf.extract_python_version is a str

598d0a7

Merge branch 'main' into proto-scripts

3e10b43

srittau approved these changes Sep 19, 2024

View reviewed changes

srittau merged commit c025e37 into python:main Sep 19, 2024
81 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrote protobuf generation scripts in Python #12527

Rewrote protobuf generation scripts in Python #12527

Avasam commented Aug 15, 2024 •

edited

Loading

Avasam Aug 16, 2024

Avasam Aug 16, 2024 •

edited

Loading

Avasam Aug 16, 2024

srittau left a comment

srittau Sep 12, 2024

Avasam Sep 18, 2024 •

edited

Loading

srittau Sep 12, 2024

Avasam Sep 18, 2024

srittau Sep 12, 2024

srittau Sep 12, 2024

-        data: dict[str, dict[str, dict[str, str]]] = json.load(file)
-    # The root key will be the protobuf source code version
-    return next(iter(data.values()))["languages"]["python"]
+        data = json.load(file)
+        # The root key will be the protobuf source code version
+        version = next(iter(data.values()))["languages"]["python"]
+        assert isinstance(version, str)
+        assert re.fullmatch(r"...", version)  # proper re here
+        return version

Rewrote protobuf generation scripts in Python #12527

Rewrote protobuf generation scripts in Python #12527

Conversation

Avasam commented Aug 15, 2024 • edited Loading

Avasam Aug 16, 2024

Choose a reason for hiding this comment

Avasam Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

Avasam Aug 16, 2024

Choose a reason for hiding this comment

srittau left a comment

Choose a reason for hiding this comment

srittau Sep 12, 2024

Choose a reason for hiding this comment

Avasam Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

srittau Sep 12, 2024

Choose a reason for hiding this comment

Avasam Sep 18, 2024

Choose a reason for hiding this comment

srittau Sep 12, 2024

Choose a reason for hiding this comment

srittau Sep 12, 2024

Choose a reason for hiding this comment

Avasam commented Aug 15, 2024 •

edited

Loading

Avasam Aug 16, 2024 •

edited

Loading

Avasam Sep 18, 2024 •

edited

Loading