chore: Data Designer e2e tests - #247
Merged
Merged
Conversation
Signed-off-by: Mike Knepper <mknepper@nvidia.com>
Signed-off-by: Mike Knepper <mknepper@nvidia.com>
Signed-off-by: Mike Knepper <mknepper@nvidia.com>
Signed-off-by: Mike Knepper <mknepper@nvidia.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
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:
📝 WalkthroughWalkthroughPR centralizes E2E fixtures (NGC API key, secret creation, nemo CLI runner) and adds storage ownership semantics. Storage configs declare whether backends own underlying data; fileset deletion conditionally deletes source data based on ownership and secret availability. New Data Designer E2E tests cover preview, job execution, seeding, and personas sampling with mock LLM providers. ChangesStorage ownership and deletion logic
E2E fixtures and Data Designer coverage
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 3
🤖 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 258-266: The runner currently returns the raw subprocess.run
CompletedProcess (the subprocess.run call) which hides non-zero exits; change
the call to fail fast by enabling subprocess.check behavior—either add
check=True to the subprocess.run invocation or capture the result and call
result.check_returncode() and re-raise subprocess.CalledProcessError so callers
get an exception on CLI failures; update the subprocess.run call site (the
CompletedProcess-returning call in e2e/conftest.py) accordingly and ensure
callers expect exceptions instead of relying on return codes.
In `@e2e/test_data_designer.py`:
- Around line 245-250: The current assertions using all(...) can pass vacuously
when no rows are produced; update the test around sdk.data_designer.preview
(preview_results) and _create_job_and_get_dataset (job_dataset) to first assert
that the produced datasets are non-empty (e.g., check preview_results.dataset
and job_dataset or assert len(_get_demo_ages(...)) > 0) and then run the
existing range checks using all(25 <= age <= 45 for age in _get_demo_ages(...));
reference preview_results, job_dataset, _get_demo_ages,
_create_job_and_get_dataset, sdk.data_designer.preview, workspace, and
config_builder when locating the assertions.
In `@services/core/files/src/nmp/core/files/api/endpoint_helpers.py`:
- Around line 375-377: The code calls
default_storage_config.copy_config(cache_prefix) and then
cache_storage.delete_all() without validating cache_prefix, allowing
path-traversal (e.g., '..' or absolute paths); before creating cache_scope,
validate/sanitize cache_prefix in the endpoint helper: reject or normalize
values that are empty, absolute (leading slash), contain parent-segments ('..')
or backslashes, or resolve outside the intended cache root (use a
PurePosixPath-like check), and raise/log an error or return a 4xx response
instead of proceeding; update the logic around
default_storage_config.copy_config(cache_prefix), storage_impl_factory(...) and
cache_storage.delete_all() to only run when the sanitized/validated cache_prefix
is safe.
🪄 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: fa393b3f-0e9e-4da1-b6b8-5d0bdf69eeba
📒 Files selected for processing (14)
e2e/conftest.pye2e/files/test_storage_backends.pye2e/test_data_designer.pypackages/nmp_common/src/nmp/common/files/storage_config.pyservices/core/files/src/nmp/core/files/api/endpoint_helpers.pyservices/core/files/src/nmp/core/files/api/v2/filesets/endpoints.pyservices/core/files/src/nmp/core/files/app/backends/huggingface.pyservices/core/files/src/nmp/core/files/app/backends/ngc.pyservices/core/files/src/nmp/core/files/app/backends/s3.pyservices/core/files/tests/test_endpoint_helpers.pyservices/core/files/tests/test_huggingface_backend.pyservices/core/files/tests/test_local_backend.pyservices/core/files/tests/test_ngc_backend.pyservices/core/files/tests/test_s3_backend.py
💤 Files with no reviewable changes (1)
- e2e/files/test_storage_backends.py
Contributor
|
Signed-off-by: Mike Knepper <mknepper@nvidia.com>
Signed-off-by: Mike Knepper <mknepper@nvidia.com>
mikeknep
force-pushed
the
aircore-729-dd-e2e/mknepper
branch
from
June 9, 2026 22:11
8c31455 to
c55ba30
Compare
Signed-off-by: Mike Knepper <mknepper@nvidia.com>
Signed-off-by: Mike Knepper <mknepper@nvidia.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
services/core/files/src/nmp/core/files/api/v2/filesets/endpoints.py (1)
417-423:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate source deletion on ownership before secret resolution.
Line 420–422 still resolves secrets and calls
delete_all()for external backends. That relies on backend no-op behavior instead of enforcing the ownership contract at the endpoint.Proposed fix
- try: - secrets = await resolve_storage_secrets_for_user(fileset.storage, workspace, sdk, auth_client) - storage = storage_impl_factory(fileset.storage, secrets) - await storage.delete_all() - except (SecretNotFoundError, SecretAccessDeniedError) as exc: + if fileset.storage.owns_storage_data: + try: + secrets = await resolve_storage_secrets_for_user(fileset.storage, workspace, sdk, auth_client) + storage = storage_impl_factory(fileset.storage, secrets) + await storage.delete_all() + except (SecretNotFoundError, SecretAccessDeniedError) as exc: # For backends we own (local, S3), the secret is required to delete the source # data; silently skipping that would orphan data we're responsible for. Surface # it. For external backends (NGC, HuggingFace) the source isn't ours to delete, # so a missing secret must not block removing the fileset - proceed. - if fileset.storage.owns_storage_data: logger.error( f"Cannot delete owned source data for fileset '{workspace}/{name}' " f"because its storage secret is unavailable: {exc}" ) raise HTTPException( HTTP_400_BAD_REQUEST, f"Cannot delete fileset '{workspace}/{name}': its storage secret is " f"unavailable, so the underlying data cannot be removed. Restore the " f"secret and retry. ({exc})", ) from exc - logger.warning( - f"Storage secret unavailable while deleting external fileset '{workspace}/{name}'; " - f"nothing to delete on the source, proceeding with entity deletion: {exc}" - ) + else: + logger.info( + f"Skipping source-data deletion for external fileset '{workspace}/{name}' " + f"(storage is not platform-owned)." + )🤖 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 `@services/core/files/src/nmp/core/files/api/v2/filesets/endpoints.py` around lines 417 - 423, The endpoint currently calls resolve_storage_secrets_for_user and storage_impl_factory then storage.delete_all for all backends; instead, first check ownership of the fileset/storage and only resolve secrets and call storage.delete_all for backends we own. Update the code around fileset.storage handling to perform an ownership gate (e.g., verify fileset is owned by the workspace or that the backend is not external) before invoking resolve_storage_secrets_for_user(fileset.storage, ...) and storage_impl_factory(...)/storage.delete_all(), leaving external backends as a no-op and avoiding secret resolution for them.
🤖 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.
Duplicate comments:
In `@services/core/files/src/nmp/core/files/api/v2/filesets/endpoints.py`:
- Around line 417-423: The endpoint currently calls
resolve_storage_secrets_for_user and storage_impl_factory then
storage.delete_all for all backends; instead, first check ownership of the
fileset/storage and only resolve secrets and call storage.delete_all for
backends we own. Update the code around fileset.storage handling to perform an
ownership gate (e.g., verify fileset is owned by the workspace or that the
backend is not external) before invoking
resolve_storage_secrets_for_user(fileset.storage, ...) and
storage_impl_factory(...)/storage.delete_all(), leaving external backends as a
no-op and avoiding secret resolution for them.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7ff0b38f-dfb2-488e-92f5-3d178d3e707c
📒 Files selected for processing (7)
e2e/test_data_designer.pypackages/nmp_common/src/nmp/common/files/storage_config.pyservices/core/files/src/nmp/core/files/api/v2/filesets/endpoints.pyservices/core/files/tests/test_huggingface_backend.pyservices/core/files/tests/test_local_backend.pyservices/core/files/tests/test_ngc_backend.pyservices/core/files/tests/test_s3_backend.py
💤 Files with no reviewable changes (5)
- services/core/files/tests/test_ngc_backend.py
- packages/nmp_common/src/nmp/common/files/storage_config.py
- services/core/files/tests/test_huggingface_backend.py
- services/core/files/tests/test_s3_backend.py
- e2e/test_data_designer.py
matthewgrossman
approved these changes
Jun 11, 2026
5 tasks
5 tasks
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
3 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.
The main thing
Restores the Data Designer e2e tests. Includes a test for workloads that require Nemotron Personas data, which previously was untested at the e2e level.
Also featuring
Fixes a Files service bug. Currently you cannot delete a Fileset that has an invalid (presumably: already deleted) secret, because a
StorageImplis a prerequisite to the actual data and entity deletion, and theStorageImplrequires secrets, so when we call out to Secrets service to fetch the secret we fail with aSecretNotFoundError.Second: currently when deleting a Fileset, we do not delete cached data, we only delete the "primary" data (when owned; we no-op for NGC and HF).<--This is expected behaviorSummary by CodeRabbit
Release Notes
Bug Fixes
Tests