ci: Move backend unit tests from dynamo/tests to dynamo/components/.../<backend>/test/
#3419
Conversation
Signed-off-by: Krishnan Prashanth <[email protected]>
Signed-off-by: Krishnan Prashanth <[email protected]>
Signed-off-by: Krishnan Prashanth <[email protected]>
Signed-off-by: Krishnan Prashanth <[email protected]>
Signed-off-by: Krishnan Prashanth <[email protected]>
Signed-off-by: Krishnan Prashanth <[email protected]>
Signed-off-by: Krishnan Prashanth <[email protected]>
Signed-off-by: Krishnan Prashanth <[email protected]>
Signed-off-by: KrishnanPrash <[email protected]>
Signed-off-by: Krishnan Prashanth <[email protected]>
WalkthroughAdds CI workflow changes splitting vLLM tests into unit and e2e pytest actions, refines test markers, and keeps e2e conditional on non-arm64. Introduces a pytest collection hook to skip backend tests when required frameworks are missing. Adds vLLM unit tests for custom Jinja template argument handling and a tests package init with headers. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant GH as GitHub Actions (vLLM job)
participant PyUnit as pytest-action (unit)
participant PyE2E as pytest-action (e2e)
Dev->>GH: Push/PR triggers workflow
rect rgb(230,240,255)
note right of GH: Build & push container image
GH->>GH: Docker tag & push
end
rect rgb(235,255,235)
GH->>PyUnit: Run unit tests (marks: unit and vllm and gpu_1)
PyUnit-->>GH: Results
end
alt matrix.platform.arch != 'arm64'
rect rgb(255,245,230)
GH->>PyE2E: Run e2e tests (marks: e2e and vllm and gpu_1 and not slow)
PyE2E-->>GH: Results
end
else arm64
GH-->>GH: Skip e2e step
end
sequenceDiagram
autonumber
participant PyTest as pytest collector
participant Hook as dynamo/conftest.py
participant FS as Test file path
participant Import as importlib.util
PyTest->>Hook: pytest_ignore_collect(FS)
Hook->>Hook: Map FS to required module (vllm/sglang/tensorrt_llm)
Hook->>Import: find_spec(module)
alt spec is None
Hook-->>PyTest: return True (skip collection)
else spec found
Hook-->>PyTest: return None (collect normally)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/container-validation-backends.yml (1)
80-90: Removegpu_1marker from the unit test stepTests in components/src/dynamo/vllm/tests/test_args.py don’t require GPU, so
pytest_marks: "unit and vllm and gpu_1"will break on arm64 runners. Change topytest_marks: "unit and vllm"or addif: ${{ matrix.platform.arch != 'arm64' }}.
🧹 Nitpick comments (5)
components/src/dynamo/conftest.py (2)
13-13: Mark the unused parameter as intentional.The
configparameter is required by the pytest hook signature but isn't used in this implementation. Prefix it with an underscore to indicate it's intentionally unused and silence the linter warning.Apply this diff:
-def pytest_ignore_collect(collection_path, config): +def pytest_ignore_collect(collection_path, _config):
21-28: Use pathlib for robust path matching.The substring matching approach (
if path_pattern in path_str) is fragile and could incorrectly match paths likebackup/vllm/tests_old/ormy_vllm/tests/. Usepathlib.Pathto match against actual path components.Apply this diff:
path_str = str(collection_path) + path = Path(collection_path) # Map backend test paths to required modules backend_requirements = { - "/vllm/tests/": "vllm", - "/sglang/tests/": "sglang", - "/trtllm/tests/": "tensorrt_llm", + ("vllm", "tests"): "vllm", + ("sglang", "tests"): "sglang", + ("trtllm", "tests"): "tensorrt_llm", } - for path_pattern, required_module in backend_requirements.items(): - if path_pattern in path_str: + for path_components, required_module in backend_requirements.items(): + # Check if path contains the backend/tests directory structure + if all(part in path.parts for part in path_components):Note: You'll need to add
from pathlib import Pathat the top of the file.components/src/dynamo/vllm/tests/test_args.py (3)
14-15: Fragile path computation with magic index.The line
Path(__file__).parents[5]uses a hardcoded index to traverse up the directory tree, which is brittle and will break if the test file moves or the repository structure changes. The magic number5has no explanation.Consider these alternatives:
Option 1: Use a relative path from the test file's location:
-TEST_DIR = Path(__file__).parents[5] / "tests" +TEST_DIR = Path(__file__).parent.parent.parent.parent.parent / "tests"(Still fragile, but more explicit about the traversal)
Option 2: Search upward for a marker file (preferred):
+def find_repo_root(start_path: Path) -> Path: + """Find repository root by searching for a marker file.""" + current = start_path + while current != current.parent: + if (current / "pyproject.toml").exists() or (current / ".git").exists(): + return current + current = current.parent + raise RuntimeError("Could not find repository root") + -TEST_DIR = Path(__file__).parents[5] / "tests" +TEST_DIR = find_repo_root(Path(__file__)) / "tests"Option 3: Use an environment variable for test fixtures:
+import os + -TEST_DIR = Path(__file__).parents[5] / "tests" +TEST_DIR = Path(os.getenv("DYNAMO_REPO_ROOT", Path(__file__).parents[5])) / "tests"
44-64: Consider verifying fixture file existence.The test assumes
JINJA_TEMPLATE_PATHpoints to an existing file but doesn't verify this. If the fixture is missing or the path computation in line 15 is wrong, the test will fail with a potentially confusing error message.Add a fixture existence check or use a temporary file:
Option 1: Verify fixture exists (simpler):
def test_custom_jinja_template_valid_path(monkeypatch): """Test that valid absolute path is stored correctly.""" + assert Path(JINJA_TEMPLATE_PATH).exists(), f"Test fixture not found: {JINJA_TEMPLATE_PATH}" monkeypatch.setattr(Option 2: Use a temporary file (more robust):
-def test_custom_jinja_template_valid_path(monkeypatch): +def test_custom_jinja_template_valid_path(monkeypatch, tmp_path): """Test that valid absolute path is stored correctly.""" + template_file = tmp_path / "custom_template.jinja" + template_file.write_text("{% mock content %}") + monkeypatch.setattr( sys, "argv", [ "dynamo.vllm", "--model", "Qwen/Qwen3-0.6B", "--custom-jinja-template", - JINJA_TEMPLATE_PATH, + str(template_file), ], ) config = parse_args() - assert config.custom_jinja_template == JINJA_TEMPLATE_PATH + assert config.custom_jinja_template == str(template_file)
18-21: Reconsider GPU marker for unit tests.These tests mock
sys.argvand callparse_args()to validate argument parsing logic. They don't perform any GPU operations or require GPU resources. The@pytest.mark.gpu_1marker seems incorrect for pure unit tests.This relates to the issue flagged in
.github/workflows/container-validation-backends.yml(lines 80-90). If these tests don't genuinely require GPU:@pytest.mark.unit @pytest.mark.vllm -@pytest.mark.gpu_1 @pytest.mark.pre_mergeApply the same change to lines 44-47 and 67-70. Alternatively, if the
gpu_1marker is used for organizational purposes (e.g., "tests that run in GPU containers"), document this in the test docstrings or a testing guide.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/container-validation-backends.yml(1 hunks)components/src/dynamo/conftest.py(1 hunks)components/src/dynamo/vllm/tests/__init__.py(1 hunks)components/src/dynamo/vllm/tests/test_args.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.13.3)
components/src/dynamo/conftest.py
13-13: Unused function argument: config
(ARG001)
⏰ 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). (6)
- GitHub Check: trtllm (amd64)
- GitHub Check: trtllm (arm64)
- GitHub Check: vllm (amd64)
- GitHub Check: vllm (arm64)
- GitHub Check: sglang
- GitHub Check: Build and Test - dynamo
🔇 Additional comments (3)
components/src/dynamo/vllm/tests/__init__.py (1)
1-2: LGTM!Standard package initialization with proper license headers.
components/src/dynamo/vllm/tests/test_args.py (1)
67-92: LGTM with minor note.The test correctly validates environment variable expansion in template paths. The assertions properly verify both that the variable was expanded (line 91) and that the result matches the expected path (line 92).
Note: This test shares the same fixture path dependency as
test_custom_jinja_template_valid_path. If you implement fixture verification there (see previous comment), consider applying it here as well.components/src/dynamo/conftest.py (1)
42-54: Inconsistent wrapper handling in conftest.py.
The detection of Dynamo’s wrapper package logs a warning yet doesn’t alter control flow, so wrapper specs still proceed to collection. Either:
- Skip wrappers by updating the
if spec is Nonecheck to also catchspec.origincontaining “/dynamo/”.- Remove the wrapper warning if wrappers are acceptable.
- Add a TODO/FIXME with an issue link if this is under investigation.
Which approach aligns with the intended behavior?
| print(f"[DEBUG] Found backend test path: {path_str}") | ||
| print(f"[DEBUG] Looking for module: {required_module}") | ||
|
|
||
| spec = importlib.util.find_spec(required_module) | ||
| print(f"[DEBUG] find_spec('{required_module}') = {spec}") | ||
|
|
||
| if spec is not None: | ||
| print(f"[DEBUG] Module origin: {spec.origin}") | ||
| print( | ||
| f"[DEBUG] Module submodule_search_locations: {spec.submodule_search_locations}" | ||
| ) | ||
|
|
||
| # Check if this is dynamo's wrapper or the real package | ||
| if spec.origin and "/dynamo/" in spec.origin: | ||
| print( | ||
| f"[DEBUG] WARNING: Found dynamo's wrapper package at {spec.origin}" | ||
| ) | ||
| print("[DEBUG] This may cause import errors!") | ||
|
|
||
| if spec is None: | ||
| print("[DEBUG] Module not found - SKIPPING collection") | ||
| return True # Module not available, skip this file | ||
| else: | ||
| print( | ||
| "[DEBUG] Module found - COLLECTING (but may fail if it's the wrapper)" | ||
| ) |
There was a problem hiding this comment.
Replace print statements with proper logging.
The function contains extensive debug output using print() statements, which will clutter test output and cannot be controlled via log levels. This appears to be temporary debugging code that should be removed or replaced with proper logging before merging.
Use pytest's logging instead:
import importlib.util
+import logging
+
+logger = logging.getLogger(__name__)
def pytest_ignore_collect(collection_path, _config):
"""Skip collecting backend test files if their framework isn't installed.
Checks for backend test directories and corresponding installed packages.
"""
path_str = str(collection_path)
# Map backend test paths to required modules
backend_requirements = {
"/vllm/tests/": "vllm",
"/sglang/tests/": "sglang",
"/trtllm/tests/": "tensorrt_llm",
}
for path_pattern, required_module in backend_requirements.items():
if path_pattern in path_str:
- print(f"[DEBUG] Found backend test path: {path_str}")
- print(f"[DEBUG] Looking for module: {required_module}")
+ logger.debug(f"Found backend test path: {path_str}")
+ logger.debug(f"Looking for module: {required_module}")
spec = importlib.util.find_spec(required_module)
- print(f"[DEBUG] find_spec('{required_module}') = {spec}")
+ logger.debug(f"find_spec('{required_module}') = {spec}")
if spec is not None:
- print(f"[DEBUG] Module origin: {spec.origin}")
- print(
- f"[DEBUG] Module submodule_search_locations: {spec.submodule_search_locations}"
- )
+ logger.debug(f"Module origin: {spec.origin}")
+ logger.debug(f"Module submodule_search_locations: {spec.submodule_search_locations}")
# Check if this is dynamo's wrapper or the real package
if spec.origin and "/dynamo/" in spec.origin:
- print(
- f"[DEBUG] WARNING: Found dynamo's wrapper package at {spec.origin}"
- )
- print("[DEBUG] This may cause import errors!")
+ logger.warning(f"Found dynamo's wrapper package at {spec.origin} - may cause import errors")
if spec is None:
- print("[DEBUG] Module not found - SKIPPING collection")
+ logger.info(f"Module {required_module} not found - skipping collection of {path_str}")
return True # Module not available, skip this file
- else:
- print(
- "[DEBUG] Module found - COLLECTING (but may fail if it's the wrapper)"
- )
+ logger.debug(f"Module {required_module} found - collecting {path_str}")
return None # Not a backend test or module available📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| print(f"[DEBUG] Found backend test path: {path_str}") | |
| print(f"[DEBUG] Looking for module: {required_module}") | |
| spec = importlib.util.find_spec(required_module) | |
| print(f"[DEBUG] find_spec('{required_module}') = {spec}") | |
| if spec is not None: | |
| print(f"[DEBUG] Module origin: {spec.origin}") | |
| print( | |
| f"[DEBUG] Module submodule_search_locations: {spec.submodule_search_locations}" | |
| ) | |
| # Check if this is dynamo's wrapper or the real package | |
| if spec.origin and "/dynamo/" in spec.origin: | |
| print( | |
| f"[DEBUG] WARNING: Found dynamo's wrapper package at {spec.origin}" | |
| ) | |
| print("[DEBUG] This may cause import errors!") | |
| if spec is None: | |
| print("[DEBUG] Module not found - SKIPPING collection") | |
| return True # Module not available, skip this file | |
| else: | |
| print( | |
| "[DEBUG] Module found - COLLECTING (but may fail if it's the wrapper)" | |
| ) | |
| import importlib.util | |
| import logging | |
| logger = logging.getLogger(__name__) | |
| def pytest_ignore_collect(collection_path, _config): | |
| """Skip collecting backend test files if their framework isn't installed. | |
| Checks for backend test directories and corresponding installed packages. | |
| """ | |
| path_str = str(collection_path) | |
| # Map backend test paths to required modules | |
| backend_requirements = { | |
| "/vllm/tests/": "vllm", | |
| "/sglang/tests/": "sglang", | |
| "/trtllm/tests/": "tensorrt_llm", | |
| } | |
| for path_pattern, required_module in backend_requirements.items(): | |
| if path_pattern in path_str: | |
| logger.debug(f"Found backend test path: {path_str}") | |
| logger.debug(f"Looking for module: {required_module}") | |
| spec = importlib.util.find_spec(required_module) | |
| logger.debug(f"find_spec('{required_module}') = {spec}") | |
| if spec is not None: | |
| logger.debug(f"Module origin: {spec.origin}") | |
| logger.debug(f"Module submodule_search_locations: {spec.submodule_search_locations}") | |
| # Check if this is dynamo's wrapper or the real package | |
| if spec.origin and "/dynamo/" in spec.origin: | |
| logger.warning( | |
| f"Found dynamo's wrapper package at {spec.origin} - may cause import errors" | |
| ) | |
| if spec is None: | |
| logger.info(f"Module {required_module} not found - skipping collection of {path_str}") | |
| return True # Module not available, skip this file | |
| logger.debug(f"Module {required_module} found - collecting {path_str}") | |
| return None # Not a backend test or module available |
🤖 Prompt for AI Agents
In components/src/dynamo/conftest.py around lines 29 to 54, replace all print()
debug statements with proper logging: import the logging module (if not present)
and create a module logger = logging.getLogger(__name__), then convert prints to
logger.debug(...) for normal debug lines and logger.warning(...) for the WARNING
line; remove any prints that were only for temporary debugging and ensure
messages remain informative and use string interpolation or f-strings passed to
the logger so pytest caplog and log-level controls can manage the output.
Overview:
As a part of the effort to restructure unit tests in Dynamo [Link to Context], this PR moves the unit vLLM test that lives within
dynamo/tests/unittocomponents/src/dynamo/vllm.Background:
/workspace. And pytest goes through two major steps before running the tests:Initially, attempted to introduce this structure as a part of #3362, but was running into python path and relative import errors.
Context for why we need conftest.py:: pytest_ignore_collect() [Link]
Since test_args.py, will indirectly perform
import vllm, we needpytest_ignore_collect()to determine if vllm is present in the environment before allowingtest_args.pyto be collected by pytest.But from
conftest.py's perspective, it cannot distiguish between the vllm folder/module (dynamo.vllm) on the same level and the vllm library in site-packages.Here is an example of what a build and test vllm job will error with:
It attempts to import the config submodule from
dynamo.vllmdue to the relative import.Here are further debug logs added to
pytest_ignore_collectas a reference for the claims made above: