Conversation
- simplify image build, no need make python wheel just call script - simplify make targets, make env varibale to pass in "suite" - remove cloud_provider check as it does not nothing but left in suite - update README - rename llmd_xks_check.py to llmd_xks_preflight.py Signed-off-by: Wen Zhou <wenzhou@redhat.com>
📝 WalkthroughWalkthroughThis PR refactors the validation container infrastructure by switching the base image to Red Hat UBI minimal with microdnf, renames the build target from "container" to "image", introduces a SUITE parameter for test selection, adds optional configuration mounting, updates all related documentation, and renames the validation module from llmd_xks_checks to llmd_xks_preflight. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
|
cc @kwozyman |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
validation/Makefile (2)
56-57: Auto-installing packages via barepip installmay pollute the system Python.If run outside a virtualenv,
pip install flake8/pip install autopep8installs globally, which may conflict with system-managed packages (especially on systems usingexternally-managed-environmentin Python 3.12+). Consider usingpip install --userorpython3 -m venv.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@validation/Makefile` around lines 56 - 57, The Makefile currently auto-installs flake8 with a bare pip install which can pollute system Python; update the installation step to avoid global installs by either invoking the installer via the active Python interpreter (e.g., python3 -m pip install --user ...) or by detecting/creating a virtualenv (python3 -m venv ...) before installing; change the lines around the flake8 invocation (the rule that runs the shell check for flake8 and calls pip install) to use "python3 -m pip install --user flake8" or to create/activate a venv and install into it so running flake8 --max-line-length=$(MAX_LINE_LENGTH) --exclude=build . no longer requires a global pip install.
42-44: The-itflags will fail in non-interactive environments (e.g., CI).The
runtarget uses--rm -it, which requires an interactive TTY. If this is ever invoked from a CI pipeline or a non-interactive shell, it will error or hang. Consider-ionly, or conditionally adding-t.Suggested fix
- $(CONTAINER_TOOL) run --rm -it --volume $(HOST_KUBECONFIG):/tmp/kubeconfig$(VOLUME_OPTS) $(CONFIG_MOUNT) -e KUBECONFIG=/tmp/kubeconfig $(CONTAINER_REPO):$(CONTAINER_TAG) -s $(SUITE) $(CONFIG_ARG) + $(CONTAINER_TOOL) run --rm -i --volume $(HOST_KUBECONFIG):/tmp/kubeconfig$(VOLUME_OPTS) $(CONFIG_MOUNT) -e KUBECONFIG=/tmp/kubeconfig $(CONTAINER_REPO):$(CONTAINER_TAG) -s $(SUITE) $(CONFIG_ARG)Or make it conditional:
INTERACTIVE := $(shell [ -t 0 ] && echo "-it" || echo "-i")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@validation/Makefile` around lines 42 - 44, The run target currently invokes $(CONTAINER_TOOL) with the interactive flags "-it", which will fail in non-interactive CI; change the invocation in the run target to avoid forcing a TTY by either using "-i" only or making the flags conditional (e.g., introduce an INTERACTIVE variable computed with a shell test like [ -t 0 ] to produce "-it" when a TTY exists and "-i" or empty otherwise), then replace the hardcoded "-it" in the run target invocation with that INTERACTIVE variable so the command works in both local and CI environments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@validation/README.md`:
- Around line 28-36: Update the Validations table in validation/README.md to
remove or update the stale "cloud_provider" row under "Suite: cluster" so it no
longer promises a PASSED/FAILED test; reference that this check is performed
automatically during initialization in the code path that modifies
self.tests["cluster"] (i.e., remove the cloud_provider entry or annotate it as
"auto-detected during init, not a reported test") and ensure the table text and
any headings match the current behavior of self.tests["cluster"] in the Python
script.
---
Nitpick comments:
In `@validation/Makefile`:
- Around line 56-57: The Makefile currently auto-installs flake8 with a bare pip
install which can pollute system Python; update the installation step to avoid
global installs by either invoking the installer via the active Python
interpreter (e.g., python3 -m pip install --user ...) or by detecting/creating a
virtualenv (python3 -m venv ...) before installing; change the lines around the
flake8 invocation (the rule that runs the shell check for flake8 and calls pip
install) to use "python3 -m pip install --user flake8" or to create/activate a
venv and install into it so running flake8 --max-line-length=$(MAX_LINE_LENGTH)
--exclude=build . no longer requires a global pip install.
- Around line 42-44: The run target currently invokes $(CONTAINER_TOOL) with the
interactive flags "-it", which will fail in non-interactive CI; change the
invocation in the run target to avoid forcing a TTY by either using "-i" only or
making the flags conditional (e.g., introduce an INTERACTIVE variable computed
with a shell test like [ -t 0 ] to produce "-it" when a TTY exists and "-i" or
empty otherwise), then replace the hardcoded "-it" in the run target invocation
with that INTERACTIVE variable so the command works in both local and CI
environments.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
validation/Containerfilevalidation/Makefilevalidation/README.mdvalidation/llmd_xks_preflight.pyvalidation/pyproject.toml
| make image | ||
| ``` | ||
|
|
||
| By default, the container is built on top of latest Fedora container image. If you have an **entitled Red Hat Enterprise Linux system**, you can use UBI9 (Universal Basic Image) as the base: | ||
| The container is built on top of UBI9 (Universal Base Image 9.5). | ||
|
|
||
| ```bash | ||
| FROM=registry.access.redhat.com/ubi9:latest make container | ||
| ``` | ||
|
|
||
| Notes: | ||
| * currently, only UBI version 9 (based on Red Hat Enterprise Linux 9) is supported | ||
| * while the base image itself can be pulled without registration, the container image will not build without a valid Red Hat entitlement -- if you are running a registered RHEL system, the entitlement is automatically passed to the container at build time | ||
|
|
||
| Regardless of base image, the resulting container image repository (name) and tag can be customized by using `CONTAINER_REPO` and `CONTAINER_TAG` environment variables: | ||
| The resulting container image repository (name) and tag can be customized by using `CONTAINER_REPO` and `CONTAINER_TAG` environment variables: | ||
|
|
||
| ```bash | ||
| CONTAINER_REPO=quay.io/myusername/llm-d-xks-preflight CONTAINER_TAG=mytag make container | ||
| FROM=registry.access.redhat.com/ubi9:latest CONTAINER_REPO=quay.io/myusername/llm-d-xks-preflight CONTAINER_TAG=mytag make container | ||
| CONTAINER_REPO=quay.io/myusername/llm-d-xks-preflight CONTAINER_TAG=mytag make image |
There was a problem hiding this comment.
Stale cloud_provider test entry in the Validations table (Line 67).
The cloud_provider row is still listed as a test under "Suite: cluster" in the Validations section (line 67), but this PR removes it from the self.tests["cluster"] dict in the Python script. Users reading this doc will expect a cloud_provider PASSED/FAILED result that will never appear. Consider removing or updating that row to clarify it's automatic detection during initialization, not a reported test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@validation/README.md` around lines 28 - 36, Update the Validations table in
validation/README.md to remove or update the stale "cloud_provider" row under
"Suite: cluster" so it no longer promises a PASSED/FAILED test; reference that
this check is performed automatically during initialization in the code path
that modifies self.tests["cluster"] (i.e., remove the cloud_provider entry or
annotate it as "auto-detected during init, not a reported test") and ensure the
table text and any headings match the current behavior of self.tests["cluster"]
in the Python script.
|
Hi @zdtsw
|
some thoughts from me:
No, I do not think building wheel does any harm, except it makes the build a bit complicated.
Yes, I think ubi is free to use, only when user wanna enable EPLE to install RH packages, they require to enable subscription. otherwise, it should work. most of upstream projects use ubi9, e.g https://github.com/llm-d/llm-d-kv-cache/blob/main/Dockerfile#L68
yes, correct. we need support it eventually. the reason i removed it for now is because i do not see it was called. but i agree with you, we need to get that part in place, so either remove it for now and add the implementation later along with support for CKS or do the implementation now so we keep it. |
|
I just tested, indeed you can build it on top of UBI, I really thought subscription is required for that. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aneeshkp, zdtsw 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 |
3 participants
Description
containertoimage, make env varibale to pass in "suite" to use only "run" target, work for both podman and docker, add support for config and install flake8/autopep8 if not exist in localHow Has This Been Tested?
Merge criteria:
Summary by CodeRabbit
New Features
Improvements
Chores