feat(ci): Add e2e test suite, including barebones test_smoke.py - #125
Merged
matthewgrossman merged 24 commits intoJun 1, 2026
Conversation
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
…-readme-to-show-top-level-project-info Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
…-readme-to-show-top-level-project-info Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
…-readme-to-show-top-level-project-info Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a local-service E2E harness and fixtures, minimal SDK smoke tests, activates a Make target to run e2e pytest with marker gating, updates TESTING.md, and splits CI wheel handling into build, per-wheel smoke tests, and a python-e2e job. ChangesE2E Testing Infrastructure
CI wheel build and smoke tests
Sequence Diagram(s)sequenceDiagram
participant pytest as pytest session
participant _services as _services fixture
participant nemo as "nemo services run"
participant health as "/health/ready"
participant sdk as NeMoPlatform
participant tests as e2e tests
pytest->>_services: setup
alt NMP_BASE_URL set
_services-->>pytest: yield external URL
else
_services->>nemo: spawn subprocess (free port)
_services->>health: poll /health/ready
health-->>_services: HTTP 200
_services-->>pytest: yield local URL
end
pytest->>sdk: construct NeMoPlatform(base_url)
sdk->>tests: used by e2e tests (workspaces, health)
tests-->>pytest: run assertions
pytest->>_services: teardown (SIGTERM -> SIGKILL if needed)
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/nemo_platform_plugin/src/nemo_platform_plugin/README.md (1)
94-118:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEnd with a dedicated
Next Stepssection.The page ends with Guides/Links/License, but it is missing the required
Next Stepscross-link section.As per coding guidelines, "Include 'Next Steps' section at the end with cross-links to related documentation content".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nemo_platform_plugin/src/nemo_platform_plugin/README.md` around lines 94 - 118, The README currently ends with Guides/Links/License and is missing the required dedicated Next Steps section. Update the documentation in the README so the final section is named Next Steps and includes cross-links to related docs, using the existing guide/link structure as the anchor for where to place and what to reference. Keep the rest of the content intact and ensure Next Steps is the last section in the file.
🧹 Nitpick comments (5)
packages/nemo_platform/BUNDLING.md (2)
121-134: ⚡ Quick winEnd with a dedicated “Next Steps” section and cross-links.
Add a final “Next Steps” section linking to related docs (e.g., wrapper README, vendor tooling docs, release process docs).
As per coding guidelines, "Include 'Next Steps' section at the end with cross-links to related documentation content."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nemo_platform/BUNDLING.md` around lines 121 - 134, Add a final "Next Steps" section to BUNDLING.md (after the "Other vendoring (`make vendor`)" section) that provides concise cross‑links to related documentation: the wrapper README, vendor tooling docs (make vendor), and release/process docs; ensure the "Next Steps" header is present and include brief one‑line bullets or links to each referenced doc so readers can follow up on wrapper usage, vendoring specifics, and the release process.
1-134: 🏗️ Heavy liftSeparate procedural content from this reference page.
This file is primarily REFERENCE but includes a HOW-TO workflow (“Extracting a package to PyPI”). Split that workflow into a dedicated how-to page and cross-link it from here.
As per coding guidelines, "Each documentation page should fit ONE Diataxis quadrant; do not mix tutorials with reference tables or how-tos with architecture explanations; use cross-links instead."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nemo_platform/BUNDLING.md` around lines 1 - 134, The document mixes reference material with a procedural HOW-TO ("Extracting a package to PyPI"); extract that entire "Extracting a package to PyPI" section into a separate how-to page (e.g., a new HOWTO_EXTRACT_PACKAGE.md) and replace the removed section in BUNDLING.md with a one-line cross-link pointing to the new how-to; update any generated docs/TOC if present to include the new how-to and ensure the BUNDLING.md top-level heading and sections (e.g., "How bundling works" and "Dependency groups") remain purely reference content.packages/nemo_platform/README.md (1)
20-87: ⚡ Quick winAdd tabbed Python SDK + CLI examples for core flows.
This page currently shows CLI-only commands. Add parallel SDK and CLI snippets (tab-set) for install/setup and first API call.
As per coding guidelines, "Provide both Python SDK and CLI examples in tab-sets for consistency and to support multiple user workflows."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nemo_platform/README.md` around lines 20 - 87, Update the "Install" and "Where to go next" sections to include parallel tabbed examples showing both Python SDK and CLI usage: add a tab-set under "Install" with CLI install commands (existing pip examples) and equivalent Python SDK instructions (how to install and import the SDK and call setup programmatically, referencing the CLI command "nemo setup"), add a tab-set for the first API call showing the CLI curl/HTTP example already present and an equivalent Python snippet that uses the SDK client to call the inference endpoint (reference the HTTP endpoint string and show using the SDK's client class / method to call chat completions), and add matching SDK examples for verification steps like "nemo services status" using the SDK's service/status method; ensure headings/names match existing terms ("nemo setup", "nemo services status", the inference gateway URL) so reviewers can find and replace content easily.packages/nemo_platform_plugin/src/nemo_platform_plugin/README.md (1)
34-90: ⚡ Quick winUse tab-sets for Python SDK and CLI variants.
The walkthrough includes Python and CLI usage but not in tab-set form, so cross-workflow parity is harder to scan.
As per coding guidelines, "Provide both Python SDK and CLI examples in tab-sets for consistency and to support multiple user workflows".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nemo_platform_plugin/src/nemo_platform_plugin/README.md` around lines 34 - 90, The README's "A minimal plugin" walkthrough presents only one combined example rather than separate Python SDK and CLI tab-set examples; update the README.md to split the usage section into tab-sets showing both a "Python SDK" snippet (e.g., importing and instantiating MyService or demonstrating programmatic usage of get_routers) and a "CLI" snippet (e.g., pip install -e . and nemo services run with the GET curl request) so readers can easily toggle between workflows; ensure the tab headers clearly label "Python SDK" and "CLI" and include the existing code blocks (the pyproject.toml and src/nemo_my_plugin/service.py example returning RouterSpec) under the appropriate tabs.e2e/test_smoke.py (1)
22-37: ⚡ Quick winDuplicate test:
test_create_and_delete_workspaceandtest_list_workspacesare identical.Both tests execute the same assertions: list workspaces and verify the fixture workspace is present. The
workspacefixture already creates/deletes the workspace, so both tests validate identical behavior.♻️ Proposed fix: merge into single test
-def test_create_and_delete_workspace(workspace: str, sdk: NeMoPlatform): - """Workspace CRUD round-trips through the platform. - - Uses the ``workspace`` fixture which creates a unique workspace - and deletes it on teardown. - """ - page = sdk.workspaces.list() - names = [w.name for w in page.data] - assert workspace in names - - def test_list_workspaces(sdk: NeMoPlatform, workspace: str): - """Listing workspaces returns at least the test workspace.""" + """Workspace CRUD round-trips through the platform. + + The ``workspace`` fixture creates a unique workspace and deletes it on teardown. + This test verifies listing returns the created workspace. + """ page = sdk.workspaces.list() names = [w.name for w in page.data] assert workspace in names🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/test_smoke.py` around lines 22 - 37, The two tests test_create_and_delete_workspace and test_list_workspaces are duplicates (both call sdk.workspaces.list() and assert the workspace fixture is present); consolidate them into a single test (e.g., keep test_list_workspaces or rename to test_create_list_and_delete_workspace) and remove the other function, ensuring the remaining test still uses the workspace fixture and asserts workspace in [w.name for w in sdk.workspaces.list().data]; delete the redundant test_create_and_delete_workspace to avoid duplicated assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/conftest.py`:
- Around line 57-72: The subprocess.run invocation that builds the args list for
starting services in e2e/conftest.py currently calls "nemo" directly; change the
args list in the subprocess.run call (the one assigned to result) to invoke the
tool via "uv run" (e.g., replace the leading element(s) so the command begins
with "uv", "run", "nemo", followed by the same flags: "services", "start",
"--service-group", "all", "--port", str(port), "--instance", instance) while
keeping capture_output, text, and timeout unchanged.
- Around line 87-95: The subprocess.run call that builds the stop command
(assigned to stop_result) currently invokes "nemo services stop" directly;
update the argument list passed to subprocess.run (the list used where
stop_result is created) to prefix the command with "uv" and "run" so it becomes
["uv", "run", "nemo", "services", "stop", "--instance", instance, "--port",
str(port), ...], ensuring all existing flags are preserved.
- Around line 101-106: The teardown currently only logs a warning when
stop_result.returncode != 0 (using logger.warning) which leaves orphaned
processes; change this to fail loudly by raising an exception (or calling
pytest.exit/pytest.fail) instead of just logging. Locate the block referencing
stop_result, stop_result.returncode, stop_result.stderr and replace the
logger.warning call with a raised error that includes the exit code and stderr
(e.g., raise RuntimeError or call pytest.exit with a message including
stop_result.returncode and stop_result.stderr) so teardown fails the run when
nemo services stop fails.
In `@packages/nemo_platform_plugin/src/nemo_platform_plugin/README.md`:
- Around line 7-23: Add a top-level "Prerequisites" section to the README.md for
the nemo-platform-plugin package (before the existing overview/Install content)
that lists the minimal environment and tooling required to build and use
plugins: required Python version (e.g., Python 3.9+), a package
manager/installer (pip/poetry), the dependency on the nemo-platform package, and
any OS or tooling notes (Docker/CI expectations) relevant to plugin authors;
ensure the section is titled "Prerequisites" and placed above the current
introductory paragraph so readers see it first.
In `@packages/nemo_platform/README.md`:
- Around line 7-10: Replace the marketing intro sentence "Make the agents you
ship faster, more accurate, and safer." and the following broad claim paragraph
with specific, concrete capabilities offered by NeMo Platform: list measurable
outcomes and features such as CLI commands provided, Python SDK
functions/classes, web UI features, supported model types or evaluation metrics,
hardening/tuning utilities, and deployment/testing workflows; reference the
README intro paragraph and the "NeMo Platform" project name to locate the text
and ensure the rewrite uses concrete terms (e.g., “CLI for dataset management
and model export,” “Python SDK with ModelEvaluator and Tuner classes,” “web UI
for experiment comparison and metric dashboards,” “tools for quantization,
pruning, and CI validation”) instead of vague marketing language.
- Around line 1-91: The README currently mixes EXPLANATION, HOW-TO, and
REFERENCE; keep this file as a pure EXPLANATION/landing page (overview, goals,
key capabilities, links to next steps) and move runnable procedures and
command/reference blocks into separate HOW-TO and REFERENCE docs: extract the
"Install" and the bash/code snippets (pip install, nemo setup, verification
commands) into an Installation/how-to page, move "Operating the platform" and
the REST/API endpoint and CLI command examples into an Operations/Reference
page, and move the "Links" section into an index or docs navigation page; update
the README headings ("Install", "Where to go next", "Operating the platform",
"Links") to be brief pointers that link to the new docs and add a short “See
docs/…” cross-link list at the bottom of the README.
---
Outside diff comments:
In `@packages/nemo_platform_plugin/src/nemo_platform_plugin/README.md`:
- Around line 94-118: The README currently ends with Guides/Links/License and is
missing the required dedicated Next Steps section. Update the documentation in
the README so the final section is named Next Steps and includes cross-links to
related docs, using the existing guide/link structure as the anchor for where to
place and what to reference. Keep the rest of the content intact and ensure Next
Steps is the last section in the file.
---
Nitpick comments:
In `@e2e/test_smoke.py`:
- Around line 22-37: The two tests test_create_and_delete_workspace and
test_list_workspaces are duplicates (both call sdk.workspaces.list() and assert
the workspace fixture is present); consolidate them into a single test (e.g.,
keep test_list_workspaces or rename to test_create_list_and_delete_workspace)
and remove the other function, ensuring the remaining test still uses the
workspace fixture and asserts workspace in [w.name for w in
sdk.workspaces.list().data]; delete the redundant
test_create_and_delete_workspace to avoid duplicated assertions.
In `@packages/nemo_platform_plugin/src/nemo_platform_plugin/README.md`:
- Around line 34-90: The README's "A minimal plugin" walkthrough presents only
one combined example rather than separate Python SDK and CLI tab-set examples;
update the README.md to split the usage section into tab-sets showing both a
"Python SDK" snippet (e.g., importing and instantiating MyService or
demonstrating programmatic usage of get_routers) and a "CLI" snippet (e.g., pip
install -e . and nemo services run with the GET curl request) so readers can
easily toggle between workflows; ensure the tab headers clearly label "Python
SDK" and "CLI" and include the existing code blocks (the pyproject.toml and
src/nemo_my_plugin/service.py example returning RouterSpec) under the
appropriate tabs.
In `@packages/nemo_platform/BUNDLING.md`:
- Around line 121-134: Add a final "Next Steps" section to BUNDLING.md (after
the "Other vendoring (`make vendor`)" section) that provides concise cross‑links
to related documentation: the wrapper README, vendor tooling docs (make vendor),
and release/process docs; ensure the "Next Steps" header is present and include
brief one‑line bullets or links to each referenced doc so readers can follow up
on wrapper usage, vendoring specifics, and the release process.
- Around line 1-134: The document mixes reference material with a procedural
HOW-TO ("Extracting a package to PyPI"); extract that entire "Extracting a
package to PyPI" section into a separate how-to page (e.g., a new
HOWTO_EXTRACT_PACKAGE.md) and replace the removed section in BUNDLING.md with a
one-line cross-link pointing to the new how-to; update any generated docs/TOC if
present to include the new how-to and ensure the BUNDLING.md top-level heading
and sections (e.g., "How bundling works" and "Dependency groups") remain purely
reference content.
In `@packages/nemo_platform/README.md`:
- Around line 20-87: Update the "Install" and "Where to go next" sections to
include parallel tabbed examples showing both Python SDK and CLI usage: add a
tab-set under "Install" with CLI install commands (existing pip examples) and
equivalent Python SDK instructions (how to install and import the SDK and call
setup programmatically, referencing the CLI command "nemo setup"), add a tab-set
for the first API call showing the CLI curl/HTTP example already present and an
equivalent Python snippet that uses the SDK client to call the inference
endpoint (reference the HTTP endpoint string and show using the SDK's client
class / method to call chat completions), and add matching SDK examples for
verification steps like "nemo services status" using the SDK's service/status
method; ensure headings/names match existing terms ("nemo setup", "nemo services
status", the inference gateway URL) so reviewers can find and replace content
easily.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f9ffe961-94c8-4968-adb6-5ecb50fd12e2
📒 Files selected for processing (9)
MakefileTESTING.mdconftest.pye2e/conftest.pye2e/test_smoke.pypackages/nemo_platform/BUNDLING.mdpackages/nemo_platform/README.mdpackages/nemo_platform/pyproject.tomlpackages/nemo_platform_plugin/src/nemo_platform_plugin/README.md
Contributor
|
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
e2e/conftest.py (1)
106-113: ⚡ Quick winTeardown leaks child processes spawned by
nemo services run.
terminate()signals only the direct child. Any subprocesses it spawns survive, holding ports and breaking later runs. Start the process in its own session and signal the group.🛡️ Proposed fix
At Popen:
proc = subprocess.Popen( args, stdout=log_file, stderr=subprocess.STDOUT, text=True, + start_new_session=True, )At teardown:
- proc.terminate() + os.killpg(proc.pid, signal.SIGTERM) try: proc.wait(timeout=10) except subprocess.TimeoutExpired: logger.warning("Process did not exit after SIGTERM, sending SIGKILL") - proc.kill() + os.killpg(proc.pid, signal.SIGKILL) proc.wait(timeout=5)Requires
import signal.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/conftest.py` around lines 106 - 113, The teardown leaks descendant processes because proc.terminate() only signals the direct child; update the process creation to start in its own session (use preexec_fn=os.setsid when calling Popen) and in the teardown use os.killpg(proc.pid, signal.SIGTERM) instead of proc.terminate(), falling back to os.killpg(proc.pid, signal.SIGKILL) if the group does not exit; ensure you import signal and os and replace proc.terminate()/proc.kill() with signaling the process group via os.killpg while still using proc.wait() with timeouts as shown.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/conftest.py`:
- Around line 89-100: The child process currently sets stdout=subprocess.PIPE
(variable proc) and only reads it on failure, which can deadlock when the pipe
buffer fills; change the spawn in e2e/conftest.py to open a temporary file
(e.g., via tempfile.NamedTemporaryFile or TemporaryFile) and pass that file
handle as stdout and stderr for subprocess.Popen, then on the failure path
(where _wait_for_healthy(url) is false) seek/read the temp file contents and
include them in the pytest.fail message (use _HEALTH_TIMEOUT and pytest.fail as
before); ensure the temp file is properly closed/cleaned up after reading to
avoid resource leaks.
---
Nitpick comments:
In `@e2e/conftest.py`:
- Around line 106-113: The teardown leaks descendant processes because
proc.terminate() only signals the direct child; update the process creation to
start in its own session (use preexec_fn=os.setsid when calling Popen) and in
the teardown use os.killpg(proc.pid, signal.SIGTERM) instead of
proc.terminate(), falling back to os.killpg(proc.pid, signal.SIGKILL) if the
group does not exit; ensure you import signal and os and replace
proc.terminate()/proc.kill() with signaling the process group via os.killpg
while still using proc.wait() with timeouts as shown.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8bdaf109-6212-4931-8798-cea58799a529
📒 Files selected for processing (2)
Makefilee2e/conftest.py
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Contributor
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/ci.yaml (1)
330-340:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix: wheel installed via
uv tool installisn’t used byuv run pytest
uv tool installinstalls the wheel into an isolated tool venv, whileuv runexecutes in the project’s own env—so these e2e tests may not exercise the installed wheel at all. Install the wheel into the same environment that runs pytest (e.g., viauv pip install/ syncing project deps) and then runuv run pytest.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yaml around lines 330 - 340, The CI currently installs the wheel into an isolated tool venv via "uv tool install" but then runs tests with "uv run pytest" which uses the project env; change the install step so the wheel is installed into the same environment used by "uv run" (e.g., replace the "uv tool install --force --python 3.13 \"${WHEEL}[services]\"" call with a project env install such as "uv pip install --force --python 3.13 \"${WHEEL}[services]\"" or otherwise sync the project deps before running "uv run --frozen pytest"), ensuring the WHEEL variable is installed into the env that executes pytest so tests exercise the installed package.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yaml:
- Around line 325-329: The CI step "Download wheel" currently uses the tag
actions/download-artifact@v8; replace that tag with the corresponding full
commit SHA for the v8 release (pin the action to a full SHA) so the uses line
reads actions/download-artifact@<FULL_COMMIT_SHA>; ensure the same SHA
corresponds to the v8 release on the actions/download-artifact repository and
keep the step name ("Download wheel") and inputs (name:
nemo-platform-wheel-py3.13, path: ${{ runner.temp }}/wheelcheck) unchanged.
---
Outside diff comments:
In @.github/workflows/ci.yaml:
- Around line 330-340: The CI currently installs the wheel into an isolated tool
venv via "uv tool install" but then runs tests with "uv run pytest" which uses
the project env; change the install step so the wheel is installed into the same
environment used by "uv run" (e.g., replace the "uv tool install --force
--python 3.13 \"${WHEEL}[services]\"" call with a project env install such as
"uv pip install --force --python 3.13 \"${WHEEL}[services]\"" or otherwise sync
the project deps before running "uv run --frozen pytest"), ensuring the WHEEL
variable is installed into the env that executes pytest so tests exercise the
installed package.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8637fd94-cb2c-4dea-b3fa-898a34a91a77
📒 Files selected for processing (1)
.github/workflows/ci.yaml
…-readme-to-show-top-level-project-info Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Contributor
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 (2)
.github/workflows/ci.yaml (2)
336-346:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
NMP_DATA_DIRis on the wrong step.
uv tool installdoesn't readNMP_DATA_DIR; the "Run e2e tests" step that actually starts services has noNMP_DATA_DIRset. The smoke-test job sets it on its run step. Move it.Proposed fix
- name: Install nemo-platform from wheel shell: bash - env: - NMP_DATA_DIR: ${{ runner.temp }}/nemo-data run: | set -euo pipefail WHEEL="$(ls ${RUNNER_TEMP}/wheelcheck/*.whl)" uv tool install --force --python 3.13 "${WHEEL}[services]" - name: Run e2e tests run: | uv run --frozen pytest e2e -v --run-e2e --junitxml=report.xml env: + NMP_DATA_DIR: ${{ runner.temp }}/nemo-data _TYPER_FORCE_DISABLE_TERMINAL: "1"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yaml around lines 336 - 346, The NMP_DATA_DIR environment variable is set on the wrong step (the step that runs "uv tool install") and needs to be moved to the "Run e2e tests" step so the services started by the pytest run see it; remove NMP_DATA_DIR from the install step and add NMP_DATA_DIR: ${{ runner.temp }}/nemo-data under the env block for the step that runs 'uv run --frozen pytest e2e -v --run-e2e --junitxml=report.xml' (the "Run e2e tests" step which already has _TYPER_FORCE_DISABLE_TERMINAL set).
334-344:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftE2E job doesn’t fully test the wheel (SDK likely from source;
nemodepends on PATH).File: .github/workflows/ci.yaml (Lines 334-344)
- name: Install nemo-platform from wheel shell: bash env: NMP_DATA_DIR: ${{ runner.temp }}/nemo-data run: | set -euo pipefail WHEEL="$(ls ${RUNNER_TEMP}/wheelcheck/*.whl)" uv tool install --force --python 3.13 "${WHEEL}[services]" - name: Run e2e tests run: | uv run --frozen pytest e2e -v --run-e2e --junitxml=report.xml
e2e/conftest.pyimportsfrom nemo_platform import NeMoPlatforminside theuv run ... pytestenvironment, so the SDK used by tests will come from the repo/workspace environment, not theuv tool installwheel env.- The spawned services use
nemo_bin = shutil.which("nemo") or <venv>/nemo, so the service process will use the wheel only if theuv tool install’dnemois onPATH; otherwise it falls back to theuv runvenv’snemo.Confirm runtime origins (inside the e2e run):
import nemo_platform; print(nemo_platform.__file__)andshutil.which("nemo"). If the goal is true wheel end-to-end, run pytest in the same wheel-installed environment (or install the wheel into the pytest venv / disable editable workspace) so both SDK +nemocome from the wheel.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yaml around lines 334 - 344, The CI currently installs the wheel into a separate tool env but then runs pytest in the workflow runner (so e2e/conftest.py's import of NeMoPlatform and the spawned services' nemo_bin (shutil.which) resolve to the repo/uv venv, not the wheel); fix by running pytest inside the same wheel-installed environment or ensuring the wheel's bin is on PATH so both the SDK import (e2e/conftest.py / NeMoPlatform) and nemo executable (nemo_bin / shutil.which) come from the wheel—i.e., change the "Run e2e tests" step to invoke pytest via the tool/wheel environment (or install the wheel into the pytest venv and disable editable workspace) so the runtime origins are identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/test_smoke.py`:
- Around line 24-29: The test test_create_and_delete_workspace leaks workspaces
if an assertion or exception occurs; wrap the create/assert/delete sequence in a
try/finally so sdk.workspaces.delete(name) always runs: call
sdk.workspaces.create(...) to get ws, perform assertions inside the try, and in
the finally block call sdk.workspaces.delete(name) (or delete by ws.id if
available) to guarantee cleanup even on failure.
---
Outside diff comments:
In @.github/workflows/ci.yaml:
- Around line 336-346: The NMP_DATA_DIR environment variable is set on the wrong
step (the step that runs "uv tool install") and needs to be moved to the "Run
e2e tests" step so the services started by the pytest run see it; remove
NMP_DATA_DIR from the install step and add NMP_DATA_DIR: ${{ runner.temp
}}/nemo-data under the env block for the step that runs 'uv run --frozen pytest
e2e -v --run-e2e --junitxml=report.xml' (the "Run e2e tests" step which already
has _TYPER_FORCE_DISABLE_TERMINAL set).
- Around line 334-344: The CI currently installs the wheel into a separate tool
env but then runs pytest in the workflow runner (so e2e/conftest.py's import of
NeMoPlatform and the spawned services' nemo_bin (shutil.which) resolve to the
repo/uv venv, not the wheel); fix by running pytest inside the same
wheel-installed environment or ensuring the wheel's bin is on PATH so both the
SDK import (e2e/conftest.py / NeMoPlatform) and nemo executable (nemo_bin /
shutil.which) come from the wheel—i.e., change the "Run e2e tests" step to
invoke pytest via the tool/wheel environment (or install the wheel into the
pytest venv and disable editable workspace) so the runtime origins are
identical.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 501b1ccb-4f3f-4be7-9950-8a5ec1cecb64
📒 Files selected for processing (3)
.github/workflows/ci.yamlTESTING.mde2e/test_smoke.py
🚧 Files skipped from review as they are similar to previous changes (1)
- TESTING.md
…-readme-to-show-top-level-project-info Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
matthewgrossman
changed the title
test_smoke.py
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
mckornfield
approved these changes
Jun 1, 2026
matthewgrossman
enabled auto-merge
June 1, 2026 21:54
svvarom
self-requested a review
June 1, 2026 21:57
4 tasks
This was referenced Jun 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
nemo services runas a child process on a free port and runs smoke tests against it via the NeMoPlatform SDK--run-e2eskip logic so e2e tests are skipped by default in normal test runswheel-testjob intowheel-build(artifact) +wheel-test(install + validate), making the wheel reusable by downstream jobspython-e2e-testCI job that runs e2e tests on every PRTESTING.mdto document the new e2e workflowE2E harness (
e2e/conftest.py)background_processcontext manager: spawns a subprocess and guarantees SIGTERM/SIGKILL cleanup on exit (unlikePopen's built-in context manager which only waits)_servicesfixture: finds a free port, spawnsnemo services run, polls/health/ready, yields the base URL, and terminates on teardownNMP_BASE_URLenv var to skip service startup and test against an already-running instanceE2E_SERVICES_LOGenv var, defaults to tempdir) for CI artifact uploadSmoke tests (
e2e/test_smoke.py)test_health_ready/test_health_live— health endpoints return 200test_create_and_delete_workspace— workspace CRUD round-triptest_list_workspaces— workspace fixture shows up in listCI changes (
.github/workflows/ci.yaml)wheel-build: new build-only job, uploads wheel artifactswheel-test: now downloads the pre-built wheel instead of building inlinepython-e2e-test: runsmake test-e2efrom the workspace venv, dumps server logs inline, uploads artifactsTest plan
make test-e2epasses locally (4 tests, ~7s)uv run pytest e2e -vwithout--run-e2eskips all e2e testsuv run pytest -v(full suite) does not run e2e testspython-e2e-testjob passeswheel-testjob passes (now downloads fromwheel-build)🤖 Generated with Claude Code