gemini-code-assist[bot] commented on code in PR #38302:
URL: https://github.com/apache/beam/pull/38302#discussion_r3155438805


##########
sdks/python/container/Dockerfile:
##########
@@ -75,21 +75,21 @@ RUN  \
 
     # Install Apache Beam SDK. Use --no-deps and pip check to verify that all
     # necessary dependencies are specified in base_image_requirements.txt.
-    pip install --no-deps -v /opt/apache/beam/tars/apache-beam.tar.gz[gcp] && \
-    pip check || (echo "Container does not include required Beam dependencies 
or has conflicting dependencies. If Beam dependencies have changed, you need to 
regenerate base_image_requirements.txt files. See: 
https://s.apache.org/beam-python-requirements-generate"; && exit 1) && \
+    uv pip install --no-deps -v /opt/apache/beam/tars/apache-beam.tar.gz[gcp] 
--system && \
+    uv pip check || (echo "Container does not include required Beam 
dependencies or has conflicting dependencies. If Beam dependencies have 
changed, you need to regenerate base_image_requirements.txt files. See: 
https://s.apache.org/beam-python-requirements-generate"; && exit 1) && \

Review Comment:
   ![high](https://www.gstatic.com/codereviewagent/high-priority.svg)
   
   `uv pip check` requires the `--system` flag to operate on the system Python 
environment when no virtual environment is present. Without this flag, `uv` 
will search for a virtual environment, fail to find one, and exit with an 
error, which will break the Docker build.
   
   ```
       uv pip check --system || (echo "Container does not include required Beam 
dependencies or has conflicting dependencies. If Beam dependencies have 
changed, you need to regenerate base_image_requirements.txt files. See: 
https://s.apache.org/beam-python-requirements-generate"; && exit 1) && \
   ```



##########
sdks/python/container/Dockerfile:
##########
@@ -75,21 +75,21 @@ RUN  \
 
     # Install Apache Beam SDK. Use --no-deps and pip check to verify that all
     # necessary dependencies are specified in base_image_requirements.txt.
-    pip install --no-deps -v /opt/apache/beam/tars/apache-beam.tar.gz[gcp] && \
-    pip check || (echo "Container does not include required Beam dependencies 
or has conflicting dependencies. If Beam dependencies have changed, you need to 
regenerate base_image_requirements.txt files. See: 
https://s.apache.org/beam-python-requirements-generate"; && exit 1) && \
+    uv pip install --no-deps -v /opt/apache/beam/tars/apache-beam.tar.gz[gcp] 
--system && \
+    uv pip check || (echo "Container does not include required Beam 
dependencies or has conflicting dependencies. If Beam dependencies have 
changed, you need to regenerate base_image_requirements.txt files. See: 
https://s.apache.org/beam-python-requirements-generate"; && exit 1) && \
 
     # Log complete list of what exact packages and versions are installed.
-    pip freeze --all && \
+    uv pip freeze && \

Review Comment:
   ![high](https://www.gstatic.com/codereviewagent/high-priority.svg)
   
   `uv pip freeze` requires the `--system` flag to correctly list packages from 
the system Python environment. Without it, the command will likely fail in this 
environment, breaking the build chain.
   
   ```
       uv pip freeze --system && \
   ```



##########
sdks/python/container/Dockerfile:
##########
@@ -75,21 +75,21 @@ RUN  \
 
     # Install Apache Beam SDK. Use --no-deps and pip check to verify that all
     # necessary dependencies are specified in base_image_requirements.txt.
-    pip install --no-deps -v /opt/apache/beam/tars/apache-beam.tar.gz[gcp] && \
-    pip check || (echo "Container does not include required Beam dependencies 
or has conflicting dependencies. If Beam dependencies have changed, you need to 
regenerate base_image_requirements.txt files. See: 
https://s.apache.org/beam-python-requirements-generate"; && exit 1) && \
+    uv pip install --no-deps -v /opt/apache/beam/tars/apache-beam.tar.gz[gcp] 
--system && \
+    uv pip check || (echo "Container does not include required Beam 
dependencies or has conflicting dependencies. If Beam dependencies have 
changed, you need to regenerate base_image_requirements.txt files. See: 
https://s.apache.org/beam-python-requirements-generate"; && exit 1) && \
 
     # Log complete list of what exact packages and versions are installed.
-    pip freeze --all && \
+    uv pip freeze && \
 
     # Remove pip cache.
     rm -rf /root/.cache/pip && \
 
     # Update ensurepip to use most recent versions of setuptools and pip. This 
avoids some vulnerabilities which won't be fixed on older versions of python.
-    pip install upgrade_ensurepip; \
+    uv pip install upgrade_ensurepip --system; \
     python3 -m upgrade_ensurepip; \
     find /usr/local/lib/python${py_version}/ensurepip/_bundled/setuptools-* 
-type f ! -name $(basename $(ls -v 
/usr/local/lib/python${py_version}/ensurepip/_bundled/setuptools-*-py3-none-any.whl
 | tail -n 1)) -delete; \
     find /usr/local/lib/python${py_version}/ensurepip/_bundled/pip-* -type f ! 
-name $(basename $(ls -v 
/usr/local/lib/python${py_version}/ensurepip/_bundled/pip-*-py3-none-any.whl | 
tail -n 1)) -delete; \
-    pip uninstall upgrade_ensurepip -y; \
+    uv pip uninstall upgrade_ensurepip -y --system; \

Review Comment:
   ![high](https://www.gstatic.com/codereviewagent/high-priority.svg)
   
   `uv pip uninstall` does not support the `-y` (or `--yes`) flag. Unlike 
`pip`, `uv` uninstalls packages non-interactively by default. Including this 
flag will cause the command to fail with an 'unexpected argument' error, 
breaking the build.
   
   ```
       uv pip uninstall upgrade_ensurepip --system; \
   ```



##########
sdks/python/container/Dockerfile:
##########
@@ -109,7 +109,7 @@ COPY  target/go-licenses/* 
/opt/apache/beam/third_party_licenses/golang/
 
 COPY target/license_scripts /tmp/license_scripts/
 RUN if [ "$pull_licenses" = "true" ] ; then \
-      pip install 'pip-licenses<6' pyyaml tenacity && \
+      uv pip install 'pip-licenses<6' pyyaml tenacity --system && \

Review Comment:
   ![medium](https://www.gstatic.com/codereviewagent/medium-priority.svg)
   
   Adding `--no-cache` to keep the image size small in this conditional 
installation step.
   
   ```
         uv pip install --no-cache 'pip-licenses<6' pyyaml tenacity --system && 
\
   ```



##########
sdks/python/container/Dockerfile:
##########
@@ -47,11 +47,11 @@ RUN  \
        && \
     rm -rf /var/lib/apt/lists/* && \
 
-    pip install --upgrade pip setuptools wheel && \
+    pip install --upgrade pip setuptools wheel uv && \
 
     # Install required packages for Beam Python SDK and common dependencies 
used by users.
     # use --no-deps to ensure the list includes all transitive dependencies.
-    pip install --no-deps -r /tmp/base_image_requirements.txt 
--extra-index-url https://download.pytorch.org/whl/cpu && \
+    uv pip install --no-deps -r /tmp/base_image_requirements.txt 
--extra-index-url https://download.pytorch.org/whl/cpu --system 
--index-strategy "unsafe-best-match" && \

Review Comment:
   ![medium](https://www.gstatic.com/codereviewagent/medium-priority.svg)
   
   To minimize the Docker image size, it is recommended to use the `--no-cache` 
flag with `uv pip install`. Since `uv` maintains its own cache (typically in 
`/root/.cache/uv`) and this directory is not currently being cleaned up in this 
layer, adding this flag prevents unnecessary bloat in the image layers.
   
   ```
       uv pip install --no-cache --no-deps -r /tmp/base_image_requirements.txt 
--extra-index-url https://download.pytorch.org/whl/cpu --system 
--index-strategy "unsafe-best-match" && \
   ```



##########
sdks/python/container/Dockerfile:
##########
@@ -75,21 +75,21 @@ RUN  \
 
     # Install Apache Beam SDK. Use --no-deps and pip check to verify that all
     # necessary dependencies are specified in base_image_requirements.txt.
-    pip install --no-deps -v /opt/apache/beam/tars/apache-beam.tar.gz[gcp] && \
-    pip check || (echo "Container does not include required Beam dependencies 
or has conflicting dependencies. If Beam dependencies have changed, you need to 
regenerate base_image_requirements.txt files. See: 
https://s.apache.org/beam-python-requirements-generate"; && exit 1) && \
+    uv pip install --no-deps -v /opt/apache/beam/tars/apache-beam.tar.gz[gcp] 
--system && \
+    uv pip check || (echo "Container does not include required Beam 
dependencies or has conflicting dependencies. If Beam dependencies have 
changed, you need to regenerate base_image_requirements.txt files. See: 
https://s.apache.org/beam-python-requirements-generate"; && exit 1) && \
 
     # Log complete list of what exact packages and versions are installed.
-    pip freeze --all && \
+    uv pip freeze && \
 
     # Remove pip cache.
     rm -rf /root/.cache/pip && \
 
     # Update ensurepip to use most recent versions of setuptools and pip. This 
avoids some vulnerabilities which won't be fixed on older versions of python.
-    pip install upgrade_ensurepip; \
+    uv pip install upgrade_ensurepip --system; \

Review Comment:
   ![medium](https://www.gstatic.com/codereviewagent/medium-priority.svg)
   
   Adding `--no-cache` to this temporary installation to avoid unnecessary 
layer bloat.
   
   ```
       uv pip install --no-cache upgrade_ensurepip --system; \
   ```



##########
sdks/python/container/Dockerfile:
##########
@@ -75,21 +75,21 @@ RUN  \
 
     # Install Apache Beam SDK. Use --no-deps and pip check to verify that all
     # necessary dependencies are specified in base_image_requirements.txt.
-    pip install --no-deps -v /opt/apache/beam/tars/apache-beam.tar.gz[gcp] && \
-    pip check || (echo "Container does not include required Beam dependencies 
or has conflicting dependencies. If Beam dependencies have changed, you need to 
regenerate base_image_requirements.txt files. See: 
https://s.apache.org/beam-python-requirements-generate"; && exit 1) && \
+    uv pip install --no-deps -v /opt/apache/beam/tars/apache-beam.tar.gz[gcp] 
--system && \

Review Comment:
   ![medium](https://www.gstatic.com/codereviewagent/medium-priority.svg)
   
   Adding `--no-cache` here will help keep the image size small by preventing 
`uv` from storing downloaded wheels in the layer cache.
   
   ```
       uv pip install --no-cache --no-deps -v 
/opt/apache/beam/tars/apache-beam.tar.gz[gcp] --system && \
   ```



-- 
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]

Reply via email to