Yicong-Huang commented on code in PR #5077:
URL: https://github.com/apache/texera/pull/5077#discussion_r3293527387
##########
.github/workflows/build.yml:
##########
@@ -315,11 +315,20 @@ jobs:
# mirrors a subset of common deps (e.g. pillow); without this
# flag a dependabot bump to a version not yet mirrored there
# fails to resolve even though PyPI has it.
+ # dev-requirements.txt provides the betterproto plugin used by
+ # genPythonProto (wired into amber/Compile in amber/build.sbt).
run: |
python -m pip install uv
if [ -f amber/requirements.txt ]; then uv pip install --system
--index-strategy unsafe-best-match -r amber/requirements.txt; fi
if [ -f amber/operator-requirements.txt ]; then uv pip install
--system --index-strategy unsafe-best-match -r amber/operator-requirements.txt;
fi
if [ -f amber/dev-requirements.txt ]; then uv pip install --system
--index-strategy unsafe-best-match -r amber/dev-requirements.txt; fi
+ - name: Install protoc 3.19.4
Review Comment:
nit: is it possible to refer to some dependency file to declare version? so
that we don't need to update CI file when need to bump protoc version.
##########
.github/workflows/build.yml:
##########
@@ -621,6 +630,22 @@ jobs:
run: |
python -m pip install uv
if [ -f amber/dev-requirements.txt ]; then uv pip install --system
-r amber/dev-requirements.txt; fi
+ - name: Install protoc 3.19.4
+ # Matches PB.protocVersion in amber/build.sbt.
+ run: |
+ curl -fsSL -o /tmp/protoc.zip
https://github.com/protocolbuffers/protobuf/releases/download/v3.19.4/protoc-3.19.4-linux-x86_64.zip
+ sudo unzip -o /tmp/protoc.zip -d /usr/local
+ sudo chmod +x /usr/local/bin/protoc
+ sudo chmod -R a+rX /usr/local/include/google
+ - name: Set up JDK 17
+ uses: actions/setup-java@v5
+ with:
+ distribution: temurin
+ java-version: 17
+ - name: Setup sbt launcher
+ uses: sbt/setup-sbt@508b753e53cb6095967669e0911487d2b9bc9f41 # v1.1.22
+ - name: Generate Python proto bindings
+ run: sbt "WorkflowExecutionService/genPythonProto"
Review Comment:
hmm, can we not rely on sbt to generate python protobuf? it is unnecessary
to install JDK just for this purpose.
##########
bin/computing-unit-worker.dockerfile:
##########
@@ -37,14 +37,27 @@ COPY build.sbt build.sbt
COPY .jvmopts .jvmopts
# Update system and install dependencies. python3-minimal is needed by
-# bin/licensing/concat_license_binary.py below.
+# bin/licensing/concat_license_binary.py below; python3-pip + curl are
+# for the protoc + betterproto[compiler] install below.
RUN apt-get update && apt-get install -y \
netcat \
unzip \
+ curl \
libpq-dev \
python3-minimal \
+ python3-pip \
&& apt-get clean
+# protoc 3.19.4 (matches PB.protocVersion in amber/build.sbt) and the
+# betterproto plugin are required by the genPythonProto sbt task so the
+# generated amber/src/main/python/proto/ tree is populated before the
+# dist is packaged.
+RUN curl -fsSL -o /tmp/protoc.zip
https://github.com/protocolbuffers/protobuf/releases/download/v3.19.4/protoc-3.19.4-linux-x86_64.zip
\
+ && unzip -o /tmp/protoc.zip -d /usr/local \
+ && chmod +x /usr/local/bin/protoc \
+ && rm /tmp/protoc.zip \
+ && pip3 install --no-cache-dir 'betterproto[compiler]==2.0.0b7'
Review Comment:
betterproto[compiler]==2.0.0b7 this is also hard coded. can we refer to
requirements.txt instead?
##########
AGENTS.md:
##########
@@ -83,8 +83,16 @@ One Python venv shared across worktrees, sibling of the
texera checkout:
```bash
python3.12 -m venv ../venv312 && source ../venv312/bin/activate
pip install -r amber/requirements.txt -r amber/operator-requirements.txt
+# For pytest or sbt-driven Python codegen, also install dev deps:
+pip install -r amber/dev-requirements.txt
```
+`amber/src/main/python/proto/` is gitignored and regenerated by the
+`genPythonProto` sbt task in [`amber/build.sbt`](amber/build.sbt), which
+runs as part of `sbt amber/compile` (or `sbt amber/genPythonProto`
+directly). Requires `protoc` on PATH (pin to `3.19.4` to match
+`PB.protocVersion`); skipped with a warning when `protoc` is missing.
Review Comment:
let's remove this from Agent.md. too many details.
##########
bin/computing-unit-master.dockerfile:
##########
@@ -36,15 +36,28 @@ COPY project/ project/
COPY build.sbt build.sbt
COPY .jvmopts .jvmopts
-# Update system and install dependencies. python3-minimal is needed by
-# bin/licensing/concat_license_binary.py below.
+# python3-minimal is needed by bin/licensing/concat_license_binary.py
+# below; python3-pip + curl are for the protoc + betterproto[compiler]
+# install below.
RUN apt-get update && apt-get install -y \
netcat \
unzip \
+ curl \
libpq-dev \
python3-minimal \
+ python3-pip \
&& apt-get clean
+# protoc 3.19.4 (matches PB.protocVersion in amber/build.sbt) and the
+# betterproto plugin are required by the genPythonProto sbt task so the
+# generated amber/src/main/python/proto/ tree is populated before the
+# dist is packaged.
Review Comment:
we are hard coding this 3.19.4 version in too many places. let's define it
at one place and all refer to it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
