250731 sync kserve/master into odh/master batch2#810
Conversation
Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com> Signed-off-by: Dan Sun <dsun20@bloomberg.net> Co-authored-by: Dan Sun <dsun20@bloomberg.net>
Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com> Signed-off-by: Dan Sun <dsun20@bloomberg.net> Co-authored-by: Dan Sun <dsun20@bloomberg.net>
Signed-off-by: Bartosz Majsak <bartosz.majsak@gmail.com>
Signed-off-by: ayush <ayush.sawant@nutanix.com> Co-authored-by: Sivanantham <90966311+sivanantha321@users.noreply.github.com>
Signed-off-by: Bartosz Majsak <bartosz.majsak@gmail.com> Co-authored-by: Sivanantham <90966311+sivanantha321@users.noreply.github.com>
Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com>
…ng (kserve#4591) Signed-off-by: Vincent Hou <shou73@bloomberg.net>
Signed-off-by: ayush <ayush.sawant@nutanix.com>
Signed-off-by: Vincent Hou <shou73@bloomberg.net>
WalkthroughThis change migrates Python dependency and environment management across the project from Poetry to the "uv" tool. All relevant Dockerfiles, Makefiles, CI workflows, scripts, and documentation are updated to use "uv" for installing, locking, and syncing dependencies. Poetry-specific plugins, configuration files, and helper scripts are removed. Project configuration files ( Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI Workflow
participant UV as uv Tool
participant PythonApp as Python Application
participant Docker as Docker Build
CI->>UV: Install uv via pip or shell script
CI->>UV: Create virtual environment (uv venv)
CI->>UV: Sync/install dependencies (uv sync)
UV->>PythonApp: Provide environment and dependencies
Docker->>UV: Use uv for dependency management in build
Docker->>PythonApp: Set PYTHONPATH, run application
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mholder6 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 33
🔭 Outside diff range comments (5)
hack/prepare-for-release.sh (2)
93-99: Incorrect variable expansion breaks empty-file guard
[ ! -s "yaml" ]literally tests a file named “yaml” instead of the loop variable.
Use"$yaml"(quoted) or drop the guard entirely if it is not needed.- if [ ! -s "yaml" ]; then + if [ ! -s "$yaml" ]; then
119-128: Fragile “for file in $(find …)” pattern – space & newline unsafeIterating over
$(find …)splits on IFS and will break on paths that contain
spaces, tabs or newlines. ShellCheck SC2044 flags this.Prefer
find … -print0 | while IFS= read -r -d '' file; do …or
find … -exec …instead.-for file in $(find python \( -name 'pyproject.toml' -o -name 'uv.lock' \)); do +find python \( -name 'pyproject.toml' -o -name 'uv.lock' \) -print0 | + while IFS= read -r -d '' file; do … -done + donepython/huggingfaceserver/tests/setup_vllm.sh (1)
20-31: Hard-coded venv path is brittle and may not exist
source /mnt/python/huggingfaceserver-cpu-venv/bin/activateassumes the runner
pre-creates that venv at exactly that location. A clean runner or non-CPU build
will fail early.Consider deriving the path from an environment variable or create the venv
on-the-fly if it is missing:: "${HF_VENV:=/mnt/python/huggingfaceserver-cpu-venv}" python -m venv "$HF_VENV" source "$HF_VENV/bin/activate"python/custom_tokenizer/pyproject.toml (1)
14-22: Non-standard table name
[dependency-groups]is not part of PEP 621. Replace with[project.optional-dependencies]for tooling compatibility.[project.optional-dependencies] test = [ "pytest>=7.4.4,<8.0.0", "mypy>=0.991,<1.0" ] dev = ["black[colorama]~=24.3.0"]python/paddleserver/paddleserver/inference_model/cat_dog.json (1)
1-300: Store large model artifacts outside the Git repoCommitting a full PIR model (~MBs) permanently inflates the repository size for every clone and slows down CI. Move the file to an object store / model registry (or Git-LFS) and pull it at build or test time instead.
♻️ Duplicate comments (6)
hack/prepare-for-release.sh (1)
131-140: Repeat of unsafe find-loop pattern in docs passSame robustness issue as above; refactor similarly for the docs loop to avoid
path-splitting bugs.
Also combine both refactors in one commit to keep script style consistent.python/aiffairness.Dockerfile (1)
8-13: Verify the uv installation script for security consistency.Same security verification needed as in artexplainer.Dockerfile - the uv installation via curl script should be verified for security.
python/aiffairness/pyproject.toml (1)
7-12: Same env-var path issue as pmmlserver.
"kserve @ file:///${PROJECT_ROOT}/../kserve"suffers from the same non-expanded variable problem and must be converted to a relative file URL +tool.uv.sourcesentry, otherwise installs outside the repo root will fail.python/sklearnserver/pyproject.toml (1)
7-12: Environment variable infile://URL breaks package resolution.
Please apply the same fix proposed forpmmlserver(file://../kserve+[tool.uv.sources]).python/custom_tokenizer.Dockerfile (1)
16-18: Same curl | sh risk as in xgb.DockerfileApply the same integrity-checking advice here when installing uv.
(See previous comment.)python/custom_transformer/pyproject.toml (1)
7-12: Same${PROJECT_ROOT}path placeholder issue as incustom_model.Use a relative path or an absolute path computed in the container build instead of relying on an env-var that may be unset.
🧹 Nitpick comments (36)
.github/.testcoverage.yml (1)
1-54: Good coverage configuration setup, but fix trailing spaces.The test coverage configuration is well-structured with appropriate thresholds and exclusions. However, there are trailing spaces on multiple lines that should be cleaned up.
Apply this diff to remove trailing spaces:
-# (mandatory) +# (mandatory) -# For cases where there are many coverage profiles, such as when running +# For cases where there are many coverage profiles, such as when running -# unit tests and integration tests separately, you can combine all those +# unit tests and integration tests separately, you can combine all those -# profiles into one. In this case, the profile should have a comma-separated list +# profiles into one. In this case, the profile should have a comma-separated list - # (optional; default 0) + # (optional; default 0) - # (optional; default 0) + # (optional; default 0) - # (optional; default 0) + # (optional; default 0) -# Holds regexp rules which will override thresholds for matched files or packages +# Holds regexp rules which will override thresholds for matched files or packages -# First rule from this list that matches file or package is going to apply +# First rule from this list that matches file or package is going to apply -# new threshold to it. If project has multiple rules that match same path, +# new threshold to it. If project has multiple rules that match same path, -# # Increase coverage threshold to 100% for `foo` package +# # Increase coverage threshold to 100% for `foo` package -# Holds regexp rules which will exclude matched files or packages +# Holds regexp rules which will exclude matched files or packages -# File name of go-test-coverage breakdown file, which can be used to +# File name of go-test-coverage breakdown file, which can be used to - # File name of go-test-coverage breakdown file which will be used to + # File name of go-test-coverage breakdown file which will be used torelease/RELEASE_PROCESS_v2.md (1)
54-54: Fix markdown formatting issues.Static analysis flagged some markdown formatting inconsistencies that should be addressed for better documentation quality.
Apply this diff to fix the formatting:
-4. Run `make uv-lock` to update pyproject.toml files for all packages. + 4. Run `make uv-lock` to update pyproject.toml files for all packages.And:
-4. Run `make uv-lock` to update pyproject.toml files for all packages. + * Run `make uv-lock` to update pyproject.toml files for all packages.Also applies to: 100-100
python/paddleserver/Makefile (1)
1-6: Good update; consider DRYing duplicate targets.Both
dev_installandinstall_dependenciesnow run the identicaluv sync --active --group testcommand. If they will always stay identical, factor the command into a single recipe (e.g., haveinstall_dependencies:depend ondev_install:) to avoid drift.test/scripts/gh-actions/setup-uv.sh (1)
23-28: Pin the uv version and avoid polluting the system interpreter
pip install uvinstalls whatever version happens to be current at run time
and installs it into the runner’s global site-packages.
For deterministic and isolated CI runs:-pip install uv +pip install --upgrade --no-cache-dir uv==0.2.4 # ← pick the repo-approved versionOptionally install with
pipxor immediately inside the fresh venv to keep the
host Python clean.python/kserve/README.md (2)
14-17: Quote the extras specifier to avoid shell globbing
pip install kserve[storage]may be expanded by some shells (especially zsh) because of the brackets. Wrap the requirement in single quotes:-pip install kserve[storage] +pip install 'kserve[storage]'
19-29: Rename the section or show the actualuvinvocationThe heading “UV” suggests a direct
uvcommand, yet the example still delegates tomake dev_install. Either:
- Keep the heading and show the explicit command sequence:
uv venv .venv uv sync --extra storage
- Or rename the subsection to “Developer setup” to match the
maketarget.This small tweak eliminates confusion for first-time contributors.
.github/workflows/python-publish.yml (1)
31-36: Remove redundantuv build–uv publishbuilds again
uv publishperforms an internal build; the precedinguv builddoubles CI time and creates unused artifacts. Prefer a single step:cd python/kserve uv publishhack/python-sdk/README.md (1)
28-48: Fix Markdown list indentation (MD007)The two-space indentation rule is violated on the nested list under step 4. Adjust indentation to satisfy linters:
4. Publish with uv to Test PyPI and verify things look right: - ```bash - uv publish \ - --publish-url https://test.pypi.org/legacy/ \ - --token your_testpypi_api_token_here \ - dist/* - ``` + ```bash + uv publish \ + --publish-url https://test.pypi.org/legacy/ \ + --token your_testpypi_api_token_here \ + dist/* + ```python/custom_tokenizer/pyproject.toml (1)
6-7: Version specifier order is reversedPEP 440 expects lower-bound first:
-requires-python = "<3.13,>=3.9" +requires-python = ">=3.9,<3.13"python/storage-initializer.Dockerfile (3)
6-24: Duplicate package installation layersThe image installs build tools twice (Lines 7-18 and 22-24), increasing size and build time. Combine into a single
microdnf installcommand.
37-41: Seconduv synclikely unnecessaryDependencies are synced before copying the source tree (Line 37). The repeat after copy (Line 40) usually changes nothing but lengthens the build. If no new optional extras are introduced, drop one call.
51-53:tomlino longer needed on Python 3.11
tomllibis in the stdlib from 3.11; keepingtomliadds an unused wheel. Delete the install and remove the TODO.Makefile.tools.mk (1)
26-26: Consider removing unused POETRY_VERSION variable.The
POETRY_VERSIONvariable appears to be unused after migrating to UV. Consider removing it to avoid confusion.-POETRY_VERSION ?= 1.8.3Makefile (1)
163-163: Declareprecommitas a.PHONYtarget.
Static analysis already warns; omitting the marker causes unnecessary rebuilds when a file namedprecommitappears.-.PHONY: clean +.PHONY: precommit cleanpython/xgb.Dockerfile (1)
12-13: Execute-from-web curl | sh without integrity checking
RUN curl -LsSf https://astral.sh/uv/install.sh | shpipes and executes a remote script every build with no hash / sig verification.
This is a supply-chain risk and can be blocked by enterprise registries.Typical hardening:
# 1. Fetch installer RUN curl -Lo /tmp/uv-install.sh https://astral.sh/uv/install.sh \ && echo "<expected-sha256> /tmp/uv-install.sh" | sha256sum -c - \ && sh /tmp/uv-install.sh \ && rm /tmp/uv-install.sh(or vendor the binary in-repo / cache).
python/custom_tokenizer.Dockerfile (1)
56-64: Redundantthird_partycopy; increases image size 2×
COPY third_party third_partycopies the directory from build context, then line 61 copies it again from the builder stage with correct ownership.
The first layer is unnecessary and remains root-owned.-# first copy – remove -COPY third_party third_party - # later copy with chown is sufficient COPY --from=builder --chown=kserve:kserve third_party third_party.github/workflows/python-test.yml (1)
238-244: ShellCheck: quote variables to avoid word-splitting
echo "/mnt/.../bin" >> $GITHUB_PATHand other unquoted vars trigger SC2086.
Quote to prevent unexpected globbing:echo "/mnt/python/huggingfaceserver-cpu-venv/bin" >> "$GITHUB_PATH"Do the same for subsequent
export VIRTUAL_ENV=…, etc.python/lgb.Dockerfile (1)
50-63:third_partyis copied twice; first copy likely unnecessary.
COPY third_party third_party(Line 50) pulls from the build context, immediately overwritten by the--from=buildercopy on Line 60. Drop the first copy to avoid cache misses and shrink layer count.python/error_404_isvc.Dockerfile (2)
26-32: Debugechocommands left in the Dockerfile.
RUN echo $(pwd)/echo $(ls)provide no runtime value and create extra layers.-RUN echo $(pwd) -RUN echo $(ls)Remove before merge.
28-31: Duplicateduv syncpattern — same optimisation aslgb.Dockerfile.You can copy the whole
error_404_isvcdirectory once, then run a singleuv syncto cut build time roughly in half..github/workflows/go.yml (1)
163-172: Two nearly identical steps can be merged to one.Instead of duplicating the whole step for
pull_requestandpull_request_target, keep one and use a compositeif::if: github.event_name == 'pull_request' || github.event_name == 'pull_request_target'Reduces maintenance overhead.
python/sklearn.Dockerfile (2)
21-27: Unnecessary seconduv syncfor kserve – adds ±50 s to build timeRunning
uv synconce after copying onlypyproject.toml/uv.lockis enough; package hashes don’t change when source is later copied. The second call just re-downloads wheels and invalidates the cache.Remove the second invocation to speed up layered builds.
28-34: Same duplication pattern forsklearnserverMirror the optimisation above for the
sklearnserversection to keep layers symmetrical and avoid redundant resolution.python/success_200_isvc.Dockerfile (4)
7-8: Use--no-install-recommendsto keep the builder image leanAdding it saves ≈ 100 MB on Debian-based slim images.
-RUN apt-get update && apt-get install -y gcc python3-dev curl && apt-get clean && \ +RUN apt-get update && apt-get install -y --no-install-recommends gcc python3-dev curl && apt-get clean && \
20-25: Seconduv syncafter source copy is redundantSee previous file; remove to reclaim build minutes.
28-31: Triple install in test resource layer
uv syncis run twice forsuccess_200_isvc, yet the lockfile didn’t change between copies. One run is sufficient; the second just thrashes the wheel cache.
52-53: Consider settingPYTHONPATHfor clarityAlthough
python -m success_200_isvc.modelworks from/, explicitness helps downstream debugging:-ENTRYPOINT ["python", "-m", "success_200_isvc.model"] +ENV PYTHONPATH=/success_200_isvc +ENTRYPOINT ["python", "-m", "success_200_isvc.model"]python/custom_transformer_grpc.Dockerfile (2)
26-33: Duplicateuv syncinvocations forkserveSame optimisation applies—drop the second call unless you rely on editable installs.
9-14: Minor size optimisationAdd
--no-install-recommends(present in other Dockerfiles) to keep the image symmetric.python/paddle.Dockerfile (2)
23-29: Doubleuv syncper componentRemove the second sync call for both
kserveandpaddleserverto cut build time roughly in half.
50-56: PATH construction: quotes are unnecessary
ENV PATH=${VIRTUAL_ENV}/bin:$PATHis simpler and avoids the strange mixed quoting style.-ENV PATH="${VIRTUAL_ENV}/bin:$PATH" +ENV PATH=${VIRTUAL_ENV}/bin:$PATHpython/custom_model_grpc.Dockerfile (1)
26-33: Remove redundant seconduv syncforkserveFollows the optimisation already described.
python/huggingface_server.Dockerfile (1)
89-105: Review FlashInfer installation logic for edge casesThe conditional installation logic looks correct but has some complexity that could benefit from verification:
- The CUDA version check uses a glob pattern
12.8*which should work for patch versions- The wheel URL has a hardcoded Python version assumption (
cp39-abi3)- Error handling could be improved if the wheel download fails
Consider adding error handling for the wheel download:
if [[ "$CUDA_VERSION" == 12.8* ]]; then \ - pip install https://download.pytorch.org/whl/cu128/flashinfer/flashinfer_python-${FLASHINFER_VERSION}%2Bcu128torch2.7-cp39-abi3-linux_x86_64.whl; \ + pip install https://download.pytorch.org/whl/cu128/flashinfer/flashinfer_python-${FLASHINFER_VERSION}%2Bcu128torch2.7-cp39-abi3-linux_x86_64.whl || \ + (echo "Failed to install FlashInfer wheel, falling back to source build" && \ + export TORCH_CUDA_ARCH_LIST="${FLASHINFER_CUDA_ARCH_LIST}" && \ + git clone --branch v${FLASHINFER_VERSION} --recursive https://github.com/flashinfer-ai/flashinfer.git && \ + cd flashinfer && python3 -m flashinfer.aot && pip install --no-build-isolation . && cd .. && rm -rf flashinfer); \ else \python/huggingface_server_cpu.Dockerfile (1)
82-111: Review complex vLLM CPU build processThe vLLM build process involves multiple steps that could be fragile:
- Git clone with specific version
- Install build requirements with
unsafe-best-matchstrategy- Install CPU requirements
- Build wheel
- Install wheel
- Cleanup
The
--index-strategy unsafe-best-matchflag could potentially introduce security or stability issues.Consider adding error handling and validation:
# Clone vLLM repo -RUN git clone --single-branch --branch v${VLLM_VERSION} https://github.com/vllm-project/vllm.git +RUN git clone --single-branch --branch v${VLLM_VERSION} https://github.com/vllm-project/vllm.git && \ + cd vllm && git verify-tag v${VLLM_VERSION} 2>/dev/null || echo "Warning: Tag verification failed" # Install built vLLM wheel -RUN uv pip install --no-cache vllm/dist/vllm-${VLLM_VERSION}*.whl +RUN ls vllm/dist/vllm-${VLLM_VERSION}*.whl && \ + uv pip install --no-cache vllm/dist/vllm-${VLLM_VERSION}*.whlpython/kserve/pyproject.toml (2)
2-6: Deduplicate the author listAll three entries have identical
namevalues. Consolidate into one canonical record to avoid confusing package metadata.
8-8: Avoid an upper bound inrequires-python
<3.13will block users on future Python releases without a compelling reason. Unless you already know 3.13 incompatibilities, drop the upper cap:-requires-python = "<3.13,>=3.9" +requires-python = ">=3.9"
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
test/e2e/conftest.py.bak (1)
1-82: Remove redundant backup fileconftest.py.bakIt looks like there’s already an active
test/e2e/conftest.pyin the directory. The.bakcopy is a stale backup and shouldn’t be part of the test suite.• Delete
test/e2e/conftest.py.bakto avoid confusion and keep the repo clean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
python/storage-initializer.Dockerfile(2 hunks)test/e2e/conftest.py.bak(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- python/storage-initializer.Dockerfile
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: precommit-check
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: build (3.10)
- GitHub Check: build (3.12)
- GitHub Check: build (3.11)
- GitHub Check: test
- GitHub Check: build (3.9)
- GitHub Check: test
🔇 Additional comments (3)
test/e2e/conftest.py.bak (3)
27-32: LGTM! Proper session-scoped logger configuration.The logger configuration correctly sets up propagation for both main and trace loggers at session scope, ensuring consistent logging across all tests.
40-52: LGTM! Proper async resource management.Both REST client fixtures correctly implement async context management with proper cleanup using
yieldandawait client.close(). The session scope is appropriate for shared client instances across tests.Also applies to: 55-67
70-81: LGTM! Well-structured pytest configuration.The command-line option and fixture setup for network layer selection is well-implemented with appropriate defaults and clear help text.
|
/rerun |
1 similar comment
|
/rerun |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
python/storage-initializer.Dockerfile (1)
26-27: Unpinneduvinstall script still executed viacurl | sh
Previous review flagged this exact supply-chain risk; the line remains unchanged. Please pin a specific release tarball and verify its checksum.
🧹 Nitpick comments (3)
python/storage-initializer.Dockerfile (3)
21-24: Redundant system-package installation
Lines 7-17 already installpython3.11-devel,gcc, etc. The secondmicrodnf installhere repeats most of them, needlessly increasing layer count and build time. Collapse the twomicrodnfinvocations into one.
35-40:uv syncruns twice – unnecessary work & layers
We first copypyproject.toml/uv.lockand runuv sync, then copy the source tree and runuv syncagain. One sync after all files are in place is sufficient; drop the first to speed up builds and avoid cache-busting on every code change to non-dependency files.
63-66: Runtime image installspython3.11-devel– not needed at run-time
*-develpackages almost double the layer size and are only required during build/compile. Consider keeping them in the builder stage only.- --enablerepo=ubi-9-baseos-rpms --enablerepo=ubi-9-appstream-rpms shadow-utils python3.11 python3.11-devel \ + --enablerepo=ubi-9-baseos-rpms --enablerepo=ubi-9-appstream-rpms shadow-utils python3.11 \
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
python/kserve/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
python/storage-initializer.Dockerfile(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: Storage Intializer Docker Publisher
python/storage-initializer.Dockerfile
[error] 19-19: Docker build failed at RUN pip install kserve: '/bin/sh: line 1: pip: command not found'. The pip command is missing in the build environment, causing exit code 127.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: precommit-check
- GitHub Check: build (3.10)
- GitHub Check: build (3.11)
- GitHub Check: test
- GitHub Check: build (3.9)
- GitHub Check: test
🔇 Additional comments (1)
python/storage-initializer.Dockerfile (1)
69-70: Third-party license artefacts no longer copied
The previously commented-outCOPY --from=builder third_party third_partyis required for license compliance. Verify whether the license files are still produced elsewhere; otherwise restore the copy step.
mholder6
force-pushed
the
250731_sync_upstream_batch2
branch
from
9f00c7d to
3925c3d
Compare
|
/rerun |
|
/lgtm |
|
/lgtm |
openshift-merge-bot
Bot
merged commit dd3f39a
into
opendatahub-io:master
github-project-automation
Bot
moved this from New/Backlog
to Done
in ODH Model Serving Planning
6 participants
What this PR does / why we need it:
Synced the kserve/master branch into odh/master branch [batch2]
[RHOAIENG-29333]
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
[project]format instead of Poetry-specific configuration.Chores
Documentation