feat: Optimize Dockerfile and update setup documentation - #173
feat: Optimize Dockerfile and update setup documentation#173ajkhabbazi wants to merge 4 commits into
Conversation
- Replace Ubuntu 20.04 + deadsnakes with python:3.11-slim - Update protobuf to ^4.25.0 (TF 2.15.1 compatible) - Update grpcio-tools to ^1.60.0 - Regenerate protobuf files for protobuf 4.x compatibility - Add Apple Silicon notes to Docker docs - Regenerate poetry.lock
Brings the branch up to date with upstream and resolves the poetry.lock conflict by regenerating the lock from the merged pyproject.toml. Reverts the auto-generated *_pb2.py files to upstream, as requested in review - they are regenerated at build time by the Dockerfile.
|
Hi @ajkhabbazi thanks for this update. We are seeing some failing workflow checks related to the pb2 files - if we are updating the version of the Protos, we should regenerate the pb2 files. I see you excluded them, but it seems we may need to include them. If we include the new proto versions and all tests are passing, we can be confident in the newly generated files. FYI - I had undergone a similar effort in the past to update the pb2 files and the Linux dependency (not the Python dependency), but never checked in the updated Protos. We probably want to also update the version of the underlying See #92 Also, if we are able to upgrade to Python version 3.14, that would match Google internal Python version, which just got bumped. Please let me know if you are able to take on the Python version update as well. Either as part of this PR or separate follow-on. |
The protobuf runtime bump to ^4.25.0 is incompatible with the legacy gencode checked into the repo, which fails at import with "Descriptors cannot be created directly". Regenerate the three pb2.py files with protoc 3.21.12, matching the version installed by CI on Linux and Mac and documented in docs/setup/mac.md. The output is identical to that of google#92. Also switch the Dockerfile to the apt-installed protoc rather than grpc_tools.protoc, which bundled 4.25.x and so generated different output inside the container than every other environment. This requires libprotobuf-dev for the well-known types, which --no-install-recommends had excluded. grpcio-tools is no longer needed and is dropped.
|
@s2t2 Thanks -- regenerated and pushed. All three *_pb2.py files are back in, generated with libprotoc 3.21.12. They come out byte-identical to the ones in #92, so that confirms your earlier regeneration was right. Worth closing #92 as superseded once this lands -- it does have the protos committed, it just never merged. On pinning protoc, I think we're already there. CI installs 3.21.12 on both platforms today and docs/setup/mac.md documents it from your #93, so the checked-in files now just match what everyone has. One thing I hit along the way. The Dockerfile was regenerating with grpc_tools.protoc, which bundles libprotoc 25.1, so the container produced different gencode than everywhere else. Switched it to the apt protoc already in the image, which needed libprotobuf-dev for the well-known types. grpcio-tools is dropped. Container output now matches the committed files exactly. The workflow is awaiting approval whenever you get a chance. On Python 3.14, that one isn't possible in this PR. tensorflow is pinned at 2.15.1, tf-agents to that, and dm-reverb has no wheels past 3.11 (#102), so it needs a TensorFlow upgrade first. Happy to look into it separately once this merges. |
3 participants
Fixes #80
Supersedes #111 (carries @igopalakrishna's original work forward).
Two changes on top:
copybara_pushto bring the branch up to date (it was 10 commits behind). Onlypoetry.lockconflicted; regenerated it from the mergedpyproject.tomlrather than resolving by hand.*_pb2.pyfiles to upstream, per @s2t2's review. They are regenerated at build time by the Dockerfile, so nothing is lost.Verified: image builds clean,
pytestpasses in the container (830 passed, 37 skipped), and all three*_pb2.pyfiles are generated inside the image and import correctly.