Skip to content

Conversation

hdefazio
Copy link

@hdefazio hdefazio commented Oct 1, 2025

Description

  • Add a more robust check for the check-container-tool make target to catch empty CONTAINER_TOOL values
  • Change the run_e2e.sh script to use the CONTAINER_TOOL value, defaulting to docker if it is unset

How Has This Been Tested?

$ make check-container-tool 
✅ Container tool 'podman' found.

(this is correct for my system)

$ make test-e2e
✅ Container tool 'podman' found.
==== Building Docker image ghcr.io/llm-d/llm-d-inference-scheduler:dev ====
podman build \

Other cases:
Manually set CONTAINER_TOOL := ""

$ make check-container-tool 
❌ Error: No container tool detected. Please install docker or podman.
make: *** [Makefile:297: check-container-tool] Error 1

Manually set CONTAINER_TOOL := "docker"

$ make check-container-tool 
❌ Error: 'docker' is not installed or not in your PATH.
🔧 Try: sudo apt install docker OR brew install docker
make: *** [Makefile:297: check-container-tool] Error 1

(this is correct for my system)

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

Summary by CodeRabbit

  • New Features

    • Allow selection of container tool via CONTAINER_TOOL, propagated to sub-commands.
    • Added check-builder target to validate and report the selected builder.
  • Improvements

    • Enhanced container tool checks with clearer validation and guidance when the tool is missing or not in PATH.
  • Tests

    • E2E script now honors CONTAINER_TOOL (defaulting to docker), prints the chosen tool, and uses it for image operations.

Copy link
Member

@pierDipi pierDipi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

/cc @nirrozenbaum

@shmuelk
Copy link
Collaborator

shmuelk commented Oct 5, 2025

Please rebase this PR

@pierDipi
Copy link
Member

pierDipi commented Oct 9, 2025

/cc @shmuelk @nirrozenbaum

@hdefazio
Copy link
Author

@shmuelk @nirrozenbaum PTAL

@shmuelk
Copy link
Collaborator

shmuelk commented Oct 19, 2025

@hdefazio Please rebase your branch

@elevran elevran moved this to In review in llm-d-inference-scheduler Oct 19, 2025
@hdefazio
Copy link
Author

@shmuelk Done!

Copy link
Collaborator

@elevran elevran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hdefazio - thanks! please see question regarding L300 (not a blocker for merging).

/lgtm
/approve

@if [ -z "$(CONTAINER_TOOL)" ]; then \
echo "❌ Error: No container tool detected. Please install docker or podman."; \
exit 1; \
elif ! command -v $(CONTAINER_TOOL) >/dev/null 2>&1; then \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: Since L40 sets CONTAINER_TOOL using command -v ..., can the check in L300 ever fail?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes that's fair - this was the original check for the make target so I didn't want to remove it. Without this, the check basically just becomes "Is CONTAINER_TOOL an empty string"?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In review

Development

Successfully merging this pull request may close these issues.

4 participants