-
Notifications
You must be signed in to change notification settings - Fork 97
LCORE-1427: Normalize Tool and Inline RAG default behavior #2146
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
base: main
Are you sure you want to change the base?
Changes from all commits
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 |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ | |
| import time | ||
| import urllib.error | ||
| import urllib.request | ||
| import warnings | ||
| from collections.abc import Generator | ||
|
|
||
| import pytest | ||
|
|
@@ -18,13 +19,17 @@ | |
| CONTAINER_START_TIMEOUT = 300 # 5 minutes for container start | ||
| CONTAINER_STOP_TIMEOUT = 15 | ||
| CONTAINER_CLEANUP_TIMEOUT = 10 | ||
| IMAGE_CLEANUP_TIMEOUT = 30 | ||
| DANGLING_IMAGES_CLEANUP_TIMEOUT = 300 # 5 minutes for dangling images cleanup | ||
| HEALTH_CHECK_TIMEOUT = 5 | ||
| PORT_QUERY_TIMEOUT = 5 | ||
|
|
||
| # Retry constants | ||
| HEALTH_CHECK_MAX_ATTEMPTS = 30 | ||
| NETWORK_BINDING_MAX_ATTEMPTS = 5 | ||
|
|
||
| DEFAULT_LIGHTSPEED_LLAMA_STACK_IMAGE_NAME = "lightspeed-llama-stack:local" | ||
|
|
||
|
|
||
| @pytest.fixture(scope="session") | ||
| def container_runtime() -> str: | ||
|
|
@@ -52,6 +57,39 @@ def container_runtime() -> str: | |
| pytest.skip("No container runtime available") | ||
|
|
||
|
|
||
| @pytest.fixture(scope="session", autouse=True) | ||
| def cleanup_container_artifacts(container_runtime: str) -> Generator[None]: | ||
| """Remove container images and dangling layers after all tests complete. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| container_runtime (str): Container runtime to use. | ||
|
|
||
| Yields | ||
| ------ | ||
| None | ||
| """ | ||
| yield | ||
|
|
||
| try: | ||
| subprocess.run( | ||
| [container_runtime, "rmi", "-f", DEFAULT_LIGHTSPEED_LLAMA_STACK_IMAGE_NAME], | ||
| capture_output=True, | ||
| timeout=IMAGE_CLEANUP_TIMEOUT, | ||
| ) | ||
| except (subprocess.CalledProcessError, subprocess.TimeoutExpired) as e: | ||
| warnings.warn(f"Image cleanup failed: {e}") | ||
|
|
||
| try: | ||
| subprocess.run( | ||
| [container_runtime, "image", "prune", "-f"], | ||
| capture_output=True, | ||
| timeout=DANGLING_IMAGES_CLEANUP_TIMEOUT, | ||
| ) | ||
| except (subprocess.CalledProcessError, subprocess.TimeoutExpired) as e: | ||
| warnings.warn(f"Dangling image cleanup failed: {e}") | ||
|
|
||
|
|
||
|
Comment on lines
+60
to
+92
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. 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win Move session-scoped autouse fixture to Defining a 🧰 Tools🪛 ast-grep (0.44.1)[error] 74-78: Command coming from incoming request (subprocess-from-request) [error] 83-87: Command coming from incoming request (subprocess-from-request) 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| @pytest.fixture(scope="class") | ||
| def managed_container(container_runtime: str) -> Generator[str, None, None]: | ||
| """Start container once for entire test class with strict cleanup. | ||
|
|
@@ -102,7 +140,7 @@ class TestContainerBuild: | |
| """Test container image building with idempotency checks.""" | ||
|
|
||
| def _get_image_id( | ||
| self, runtime: str, image_name: str = "lightspeed-llama-stack:local" | ||
| self, runtime: str, image_name: str = DEFAULT_LIGHTSPEED_LLAMA_STACK_IMAGE_NAME | ||
| ) -> str: | ||
| """Get the unique, immutable Image ID (SHA256). | ||
|
|
||
|
|
@@ -145,7 +183,7 @@ def test_build_llama_stack_image(self, container_runtime: str) -> None: | |
|
|
||
| # Verify image is listed with correct tag | ||
| result = subprocess.run( | ||
| [container_runtime, "images", "lightspeed-llama-stack:local"], | ||
| [container_runtime, "images", DEFAULT_LIGHTSPEED_LLAMA_STACK_IMAGE_NAME], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=PORT_QUERY_TIMEOUT, | ||
|
|
@@ -548,7 +586,12 @@ def test_clean_removes_image_and_container(self, container_runtime: str) -> None | |
|
|
||
| # Verify image is removed | ||
| result = subprocess.run( | ||
| [container_runtime, "images", "-q", "lightspeed-llama-stack:local"], | ||
| [ | ||
| container_runtime, | ||
| "images", | ||
| "-q", | ||
| DEFAULT_LIGHTSPEED_LLAMA_STACK_IMAGE_NAME, | ||
| ], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=PORT_QUERY_TIMEOUT, | ||
|
|
||
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.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Missing
check=TruesuppressesCalledProcessErrorfor cleanup commands.Both cleanup steps attempt to catch
subprocess.CalledProcessErrorto log a warning on failure, butsubprocess.runwill never raise this exception unlesscheck=Trueis passed. Without it, failed commands silently return a non-zero exit code and the warnings are never triggered.tests/integration/container_lifecycle/test_container_lifecycle.py#L74-L81: Addcheck=Trueto thermisubprocess call.tests/integration/container_lifecycle/test_container_lifecycle.py#L83-L90: Addcheck=Trueto theimage prunesubprocess call.🛠️ Proposed fix
try: subprocess.run( [container_runtime, "rmi", "-f", DEFAULT_LIGHTSPEED_LLAMA_STACK_IMAGE_NAME], capture_output=True, timeout=IMAGE_CLEANUP_TIMEOUT, + check=True, ) except (subprocess.CalledProcessError, subprocess.TimeoutExpired) as e: warnings.warn(f"Image cleanup failed: {e}") try: subprocess.run( [container_runtime, "image", "pr prune", "-f"], capture_output=True, timeout=DANGLING_IMAGES_CLEANUP_TIMEOUT, + check=True, ) except (subprocess.CalledProcessError, subprocess.TimeoutExpired) as e: warnings.warn(f"Dangling image cleanup failed: {e}")🧰 Tools
🪛 ast-grep (0.44.1)
[error] 74-78: Command coming from incoming request
Context: subprocess.run(
[container_runtime, "rmi", "-f", DEFAULT_LIGHTSPEED_LLAMA_STACK_IMAGE_NAME],
capture_output=True,
timeout=IMAGE_CLEANUP_TIMEOUT,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
📍 Affects 1 file
tests/integration/container_lifecycle/test_container_lifecycle.py#L74-L81(this comment)tests/integration/container_lifecycle/test_container_lifecycle.py#L83-L90🤖 Prompt for AI Agents