-
Notifications
You must be signed in to change notification settings - Fork 65
LCORE-497, LCORE-498, LCORE-499: Bundle most of ansible, assist-installer, default run.yaml dependencies, with pytorch and libs that depends on it. #292
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
d36cc67
ea978df
fd0e2c2
5aa5643
f73fe71
0c457fc
ae83068
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -18,6 +18,6 @@ jobs: | |||||
| with: | ||||||
| python-version: '3.12' | ||||||
| - name: Install dependencies | ||||||
| run: uv sync | ||||||
| run: uv sync --group llslibdev | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix: install dev tools (pyright) — use dev group, not llslibdev
- run: uv sync --group llslibdev
+ run: uv sync --group dev📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||
| - name: Run Pyright tests | ||||||
| run: uv run pyright src | ||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -24,7 +24,7 @@ jobs: | |||||
| - name: uv version | ||||||
| run: uv --version | ||||||
| - name: Install dependencies | ||||||
| run: uv pip install --python ${{ matrix.python-version }} -e . | ||||||
| run: uv pip install --python ${{ matrix.python-version }} --group llslibdev . | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Broken install command:
- run: uv pip install --python ${{ matrix.python-version }} --group llslibdev .
+ run: uv sync --group dev --group llslibdevNote: If your unit tests don’t need the 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||
| - name: Install pdm # Required for dynamic version test | ||||||
| run: uv pip install pdm | ||||||
| - name: Run unit tests | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,5 +1,5 @@ | ||||||||||
| # vim: set filetype=dockerfile | ||||||||||
| FROM registry.access.redhat.com/ubi9/python-312-minimal AS builder | ||||||||||
| FROM registry.access.redhat.com/ubi9/python-312 AS builder | ||||||||||
|
|
||||||||||
| ARG APP_ROOT=/app-root | ||||||||||
| ARG LSC_SOURCE_DIR=. | ||||||||||
|
|
@@ -11,16 +11,21 @@ ENV UV_COMPILE_BYTECODE=0 \ | |||||||||
|
|
||||||||||
| WORKDIR /app-root | ||||||||||
|
|
||||||||||
| # Install gcc - required by polyleven python package on aarch64 | ||||||||||
| # (dependency of autoevals, no pre-built binary wheels for linux on aarch64) | ||||||||||
| USER root | ||||||||||
| RUN dnf install -y --nodocs --setopt=keepcache=0 --setopt=tsflags=nodocs gcc | ||||||||||
|
|
||||||||||
| # Install uv package manager | ||||||||||
| RUN pip3.12 install uv | ||||||||||
| RUN pip3.12 install "uv==0.8.11" | ||||||||||
|
|
||||||||||
| # Add explicit files and directories | ||||||||||
| # (avoid accidental inclusion of local directories or env files or credentials) | ||||||||||
| COPY ${LSC_SOURCE_DIR}/src ./src | ||||||||||
| COPY ${LSC_SOURCE_DIR}/pyproject.toml ${LSC_SOURCE_DIR}/LICENSE ${LSC_SOURCE_DIR}/README.md ${LSC_SOURCE_DIR}/uv.lock ./ | ||||||||||
|
|
||||||||||
| RUN uv sync --locked --no-dev | ||||||||||
|
|
||||||||||
| # Bundle additional dependencies for library mode. | ||||||||||
| RUN uv sync --locked --no-dev --group llslibdev | ||||||||||
|
Comment on lines
+27
to
+28
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Contradiction with PR objective: this installs PyTorch into the image
This contradicts the stated PR goal: “no pytorch and libs that depends on it”. Please either:
Option A (keep llslibdev, exclude ML training libs from base): -RUN uv sync --locked --no-dev --group llslibdev
+RUN uv sync --locked --no-devThen in pyproject.toml (see separate comment), split the group: keep non-PyTorch deps in 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||
|
|
||||||||||
| # Final image without uv package manager | ||||||||||
| FROM registry.access.redhat.com/ubi9/python-312-minimal | ||||||||||
|
|
@@ -42,6 +47,11 @@ COPY --from=builder --chown=1001:1001 /app-root /app-root | |||||||||
| # this directory is checked by ecosystem-cert-preflight-checks task in Konflux | ||||||||||
| COPY --from=builder /app-root/LICENSE /licenses/ | ||||||||||
|
|
||||||||||
| # Add uv to final image for derived images to add additional dependencies | ||||||||||
| # with command: | ||||||||||
| # $ uv pip install <dependency> | ||||||||||
| RUN pip3.12 install "uv==0.8.11" | ||||||||||
|
|
||||||||||
| # Add executables from .venv to system PATH | ||||||||||
| ENV PATH="/app-root/.venv/bin:$PATH" | ||||||||||
|
|
||||||||||
|
|
||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix: install dev tools (pylint) — use dev group, not llslibdev
llslibdevdoesn’t include pylint; this step will fail to findpylintforuv run pylint. Install thedevgroup (which contains pylint). Also avoids pulling heavy ML deps unnecessarily into the linter job.📝 Committable suggestion
🤖 Prompt for AI Agents