fix: move nss jobs into plugin, update docs#195
Conversation
Signed-off-by: mkornfield <mkornfield@nvidia.com>
|
Documentation preview is readyPreview: https://nvidia-nemo.github.io/nemo-platform/pr-preview/pr-195/pr-195/ Built from This preview is deployed from this PR branch, updates when docs changes are pushed, and will be removed when the PR closes. |
|
Lost in the diff? Review this PR in Change Stack to follow the change map from intent to exact ranges. 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 Safe Synthesizer plugin SDK (resources, job builder, job wrapper), job-config validators for pretrained-adapter reuse, tests, packaging/entry-point and manifest updates, and documentation clarifying local execution and Jobs API/SDK workflows. ChangesSafe Synthesizer SDK rollout
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@docs/safe-synthesizer/about/host-local-development.md`:
- Line 4: Add a blank line before the heading text "# Local and Subprocess
Execution" in the file to satisfy MD022; locate the heading string and insert
one empty line immediately above it so the heading is separated from prior
content.
In `@docs/safe-synthesizer/tutorials/safe-synthesizer-101.md`:
- Around line 77-79: The error message hardcodes product and plugin names;
replace the literal strings "Safe Synthesizer" and "safe-synthesizer" in the
quoted message with the appropriate Sphinx substitutions (e.g. |product| or the
project-specific substitution such as |safe_synthesizer_plugin|) so docs use
configured substitutions consistently; update the three concatenated string
pieces to use the substitution tokens (keeping the same wording and path
structure but substituting the product/plugin tokens) so Sphinx renders the
correct product name across builds.
- Around line 123-124: The current code assumes provider_name contains "/",
causing split("/", 1) to crash for unqualified names; update the logic around
provider_name, provider_workspace, and provider_id so both "provider" and
"workspace/provider" work: if "/" in provider_name then split into
provider_workspace and provider_id, otherwise set provider_id = provider_name
and provider_workspace = None (or an empty value), and call
client.inference.providers.retrieve(provider_id, workspace=provider_workspace)
only when provider_workspace is present (or pass None/omit the workspace
argument as appropriate).
In `@mkdocs.yml`:
- Around line 222-229: Remove the two hidden_docs.paths entries that reference
host-local-development.md and safe-synthesizer-101.md so those files are no
longer included in hidden_docs.paths; specifically edit the mkdocs.yml
hidden_docs.paths list to delete the items
"safe-synthesizer/tutorials/host-local-development.md" and
"safe-synthesizer/tutorials/safe-synthesizer-101.md" (or the exact matching path
strings present) so the hide_unready_docs.py hook no longer filters them out and
the nav entries for host-local-development.md and safe-synthesizer-101.md remain
usable.
In
`@plugins/nemo-safe-synthesizer/src/nemo_safe_synthesizer_plugin/sdk/job_builder.py`:
- Around line 159-163: The path branch currently forces all file-path sources
through pd.read_csv which will break parquet/json/jsonl inputs; update the
loader in the method using self._data_source (in job_builder.py / JobBuilder or
the method containing that if-block) to detect the file type via
Path(self._data_source).suffix.lower() and call the appropriate pandas reader:
pd.read_parquet for .parquet, pd.read_json(..., lines=True) for .jsonl,
pd.read_json for .json, and pd.read_csv for .csv (fall back to pd.read_csv for
unknown extensions or raise a clear error). Apply the same change to the other
identical branch around the later occurrence (the block at the 235-240 region).
In `@plugins/nemo-safe-synthesizer/src/nemo_safe_synthesizer_plugin/sdk/job.py`:
- Around line 98-103: The status-change prints execute unconditionally (using
current_status_info and previous_status_info), ignoring the verbose flag; wrap
the print block so it only executes when the function/class-level verbose is
True (e.g., if verbose: or if self.verbose:) and ensure the surrounding function
(where current_status_info/previous_status_info are compared) accepts or
references that verbose flag so behavior is unchanged when verbose is False.
🪄 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: a2f01659-19de-4e38-b14a-6be48c8f8b2b
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
docs/_snippets/nvidia-build-model-provider.mddocs/safe-synthesizer/.gitignoredocs/safe-synthesizer/about/host-local-development.mddocs/safe-synthesizer/about/jobs.mddocs/safe-synthesizer/about/reference.mddocs/safe-synthesizer/getting-started.mddocs/safe-synthesizer/llms.txtdocs/safe-synthesizer/tutorials/differential-privacy.mddocs/safe-synthesizer/tutorials/safe-synthesizer-101.mdmkdocs.ymlpackages/nemo_platform/pyproject.tomlplugins/nemo-safe-synthesizer/README.mdplugins/nemo-safe-synthesizer/pyproject.tomlplugins/nemo-safe-synthesizer/src/nemo_safe_synthesizer_plugin/api/v2/jobs/endpoints.pyplugins/nemo-safe-synthesizer/src/nemo_safe_synthesizer_plugin/sdk/config.pyplugins/nemo-safe-synthesizer/src/nemo_safe_synthesizer_plugin/sdk/http_utils.pyplugins/nemo-safe-synthesizer/src/nemo_safe_synthesizer_plugin/sdk/job.pyplugins/nemo-safe-synthesizer/src/nemo_safe_synthesizer_plugin/sdk/job_builder.pyplugins/nemo-safe-synthesizer/src/nemo_safe_synthesizer_plugin/sdk/resources.pyplugins/nemo-safe-synthesizer/src/nemo_safe_synthesizer_plugin/skills/safe-synthesizer/workflows/diagnose.mdplugins/nemo-safe-synthesizer/src/nemo_safe_synthesizer_plugin/skills/safe-synthesizer/workflows/results.mdplugins/nemo-safe-synthesizer/src/nemo_safe_synthesizer_plugin/skills/safe-synthesizer/workflows/run.mdplugins/nemo-safe-synthesizer/tests/unit/test_jobs.pyplugins/nemo-safe-synthesizer/tests/unit/test_sdk.pypyproject.tomlthird_party/licenses.jsonlthird_party/osv-licenses.jsonthird_party/requirements-main.txt
Signed-off-by: mkornfield <mkornfield@nvidia.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
these SDK files are just a port of something I deleted in the old place
anastasia-nesterenko
left a comment
anastasia-nesterenko
left a comment
There was a problem hiding this comment.
left some nit-picks
| SafeSynthesizerJobBuilder(client) | ||
| .with_data_source(df) | ||
| .with_classify_model_provider(provider_name) # Enable column classification | ||
| .with_replace_pii() # Enable PII replacement |
There was a problem hiding this comment.
nit: with with_replace_pii() deleted, should the section **What happens next:** be modified?
There was a problem hiding this comment.
yeah... I'll put this back (removed it to speed up testing)
There was a problem hiding this comment.
nit: old name usage
There was a problem hiding this comment.
nit: same old name usage
tylersbray
left a comment
tylersbray
left a comment
There was a problem hiding this comment.
agent (
pr-review): Full review summary
Solid PR overall — plugin SDK relocation, _runtime_job_config, and scheduler workspace fix are the right moves. Inline comments below are ordered roughly by severity.
Merge blockers / high priority
safe-synthesizer-101.md— classify provider configured but never submitted (missing.with_replace_pii())nvidia-build-model-provider.md—default/vssystem/workspace mismatch with platform-seed
Should fix in this PR
3. Stale "Host-Local Development and Testing" link titles in jobs.md, reference.md, about/index.md
4. Add docs/safe-synthesizer/sdk-resources.md (parity with anonymizer/evaluator/data-designer)
5. AsyncSafeSynthesizerJobsResource.get_logs should be async def
6. Expand tests for SafeSynthesizerJob and _runtime_job_config edge cases
Confirm intent / lower priority
7. mkdocs.yml nav pruning — jobs/reference/DP tutorial removed from published nav
8. job_builder.py — with_classify_model_provider() only applies when PII replacement enabled
9. job.py — broad except Exception in wait_for_completion
10. config.py — silent ImportError leaves empty __all__
11. packages/nemo_platform/BUNDLING.md still references removed safe_synthesizer_sdk vendoring (file not in this PR)
Looks good
enabled-pluginsaddition, SDK entry points,test_sdk.pyresource/builder coverage, scheduler workspace test
| @@ -164,7 +175,6 @@ builder = ( | |||
| SafeSynthesizerJobBuilder(client) | |||
| .with_data_source(df) | |||
| .with_classify_model_provider(provider_name) # Enable column classification | |||
There was a problem hiding this comment.
agent (
pr-review): Tutorial regression — classify provider never reaches job spec
Human spot-check: Step 6 verifies a classify model provider and Step 7 calls .with_classify_model_provider(), but .with_replace_pii() was removed from this builder chain. Users follow classification setup that the submitted job ignores.
Root cause: SafeSynthesizerJobBuilder._build_job_spec() only injects classify_model_provider when enable_replace_pii is true (job_builder.py L198–203). The API compiler reads it only from config.replace_pii.globals.classify.
Agent task — pick one:
- Option A (restore prior behavior): Add
.with_replace_pii()before.synthesize()on this chain. - Option B (synthesis-only tutorial): Remove/reword Step 6 and drop
.with_classify_model_provider()here.
Acceptance criteria:
# Option A: spec must include
config["enable_replace_pii"] is True
config["replace_pii"]["globals"]["classify"]["classify_model_provider"] == provider_name
# Option B: spec must NOT reference classify_model_provider in the main jobThere was a problem hiding this comment.
yeah I'll put it back
| self._enable_replace_pii = True | ||
| return self | ||
|
|
||
| def with_classify_model_provider(self, provider_name: str) -> Self: |
There was a problem hiding this comment.
agent (
pr-review):with_classify_model_provider()is a no-op without PII replacement
Human spot-check: Callers can chain .with_classify_model_provider("default/nvidia-build") on a synthesis-only job and believe classification is enabled. It is silently ignored unless .with_replace_pii() was also called.
Related: Same issue drives the safe-synthesizer-101.md tutorial regression.
Agent task — pick one:
- Doc-only: Add a docstring/note on
with_classify_model_provider()stating it requires.with_replace_pii(). - API fix: If classification should work independently of PII replacement, inject
classify_model_providerinto a synthesis-accessible config path (and updateendpoints.pycompiler). Confirm product intent first.
Acceptance criteria: Either explicit docs warning OR spec contains classify config without enable_replace_pii.
There was a problem hiding this comment.
with_replace_pii coming in hot
| @@ -1,8 +1,8 @@ | |||
| !!! note | |||
| The platform pre-configures a `system/nvidia-build` model provider during startup. | |||
| The platform pre-configures a `default/nvidia-build` model provider during startup. | |||
There was a problem hiding this comment.
agent (
pr-review):default/nvidia-buildmay be wrong for platform-seed deployments
Human spot-check: This shared snippet now says default/nvidia-build. That matches nemo setup (_auto_setup(client, "default")), but platform-seed still creates the provider in the system workspace (services/platform-seed/src/nmp/platform_seed/tasks/seed/run.py). Helm/K8s users may not find default/nvidia-build.
Agent task — pick one:
- Document both paths:
nemo setup→default/nvidia-build; platform-seed →system/nvidia-build. - Align platform-seed to
defaultworkspace (bigger change, out of scope?). - De-emphasize hard-coded workspace: tell users to run
nemo inference providers listand use whatever workspace/name exists.
Acceptance criteria: Snippet is accurate for both local setup and seeded deployments, or explicitly scopes itself to nemo setup users only.
There was a problem hiding this comment.
this confuses the frick out of me lol. I think I want to just do the nemo setup version lol
|
|
||
| from nemo_platform import NeMoPlatform | ||
| from nemo_platform.beta.safe_synthesizer.job_builder import SafeSynthesizerJobBuilder | ||
| from nemo_safe_synthesizer_plugin.sdk.job_builder import SafeSynthesizerJobBuilder |
There was a problem hiding this comment.
agent (
pr-review): Stale link titles in this file
Human spot-check: Import path update here is correct. Same file still uses the old page title in two places:
- L112:
[Host-Local Development and Testing](host-local-development.md) - L312: same label in Related topics
Nav/getting-started renamed this page to "Local and Subprocess Execution".
Agent task:
# Replace both occurrences:
[Local and Subprocess Execution](host-local-development.md)Also update docs/safe-synthesizer/about/reference.md L20 and docs/safe-synthesizer/about/index.md L109 (not touched in this PR).
There was a problem hiding this comment.
ah yeah I need to sweep these
|
|
||
| from nemo_platform import NeMoPlatform | ||
| from nemo_platform.beta.safe_synthesizer.job_builder import SafeSynthesizerJobBuilder | ||
| from nemo_safe_synthesizer_plugin.sdk.job_builder import SafeSynthesizerJobBuilder |
There was a problem hiding this comment.
agent (
pr-review): Stale link title on L20
Human spot-check: Import path update (this hunk) is correct. L20 still says:
[Host-Local Development and Testing](host-local-development.md)
Agent task:
[Local and Subprocess Execution](host-local-development.md)Mirror the same fix in jobs.md L112/L312 and about/index.md L109.
| return workspace, job_name | ||
|
|
||
|
|
||
| def _runtime_job_config(job_config: SafeSynthesizerJobConfig) -> dict[str, Any]: |
There was a problem hiding this comment.
agent (
pr-review): Add edge-case tests for_runtime_job_config
Human spot-check: _runtime_job_config correctly strips training.pretrained_model when pretrained_model_job is set — good fix. test_plugin_job_config_allows_pretrained_model_job_runtime_config covers the happy path.
Agent task — add unit tests in test_jobs.py:
pretrained_model_jobset butconfig.trainingmissing → no error, config passes through.pretrained_model_jobset butconfig.trainingis not a dict → no error.pretrained_model_jobunset →training.pretrained_modelpreserved.
Acceptance criteria: _runtime_job_config behavior is locked for non-dict/missing training shapes.
| @@ -320,19 +326,9 @@ nav: | |||
| - SDK Resources: data-designer/sdk-resources.md | |||
| - Migrating from Standalone Library: data-designer/migration.md | |||
| - Synthesize Safe Data: | |||
There was a problem hiding this comment.
agent (
pr-review): Confirm intentional nav pruning
Human spot-check: Published Synthesize Safe Data nav shrinks from full About + tutorials to only:
- Getting Started
- Local and Subprocess Execution
- Safe Synthesizer 101
Removed from nav (moved to hidden_docs): jobs.md, reference.md, differential-privacy.md, about/index.md, evaluation, PII replacement, etc.
Question for author: Is this deliberate scope reduction for the public docs site?
Agent task if intentional:
- Add cross-links from Getting Started / 101 to hidden pages for readers using
make live-with-unready. - Consider keeping Differential Privacy or Jobs in nav if still user-facing.
Agent task if unintentional: Restore About subsection and DP tutorial to nav.
There was a problem hiding this comment.
yeah I'll put MOAR in
| if log_key not in seen_log_keys: | ||
| print(new_log.message.strip()) | ||
| seen_log_keys.add(log_key) | ||
| except Exception: |
There was a problem hiding this comment.
agent (
pr-review): Narrow exception handling inwait_for_completion
Human spot-check: Log polling uses except Exception: which swallows all failures during status wait (network blips OK, but also programming errors).
Agent task — pick one:
- Catch
httpx.HTTPError(and maybeTimeoutException) only; re-raise others. - Keep broad catch but log at warning without
logger.exception(full traceback) unlessverboseand repeated failures.
Acceptance criteria: Transient log-fetch failures don't abort wait; unexpected exceptions still surface.
There was a problem hiding this comment.
ugh I remember this could hit weird timeouts and make the logs look garbage, but might be worth cleaning up
| "TimeSeriesParameters", | ||
| "TrainingHyperparams", | ||
| ] | ||
| except ImportError: |
There was a problem hiding this comment.
agent (
pr-review): SilentImportErrorhides missing dependency
Human spot-check: If nemo_safe_synthesizer is not installed, config.py sets __all__ = [] with no warning. Users get confusing ImportError from deeper imports.
Agent task — pick one:
- Re-raise
ImportErrorwith message: "Install nemo-safe-synthesizer to use SDK config types." - Emit
logger.warning(...)once in theexceptblock.
Acceptance criteria: Missing optional dep produces actionable error, not empty module.
There was a problem hiding this comment.
yeah I don't know how useful this guard is personally
| "nemo-evaluator-plugin", | ||
| "nemo-guardrails-plugin", | ||
| "nemo-auditor-plugin", | ||
| "nemo-safe-synthesizer-plugin", |
There was a problem hiding this comment.
agent (
pr-review): Good — required for SDK mount
Human spot-check: Adding nemo-safe-synthesizer-plugin to enabled-plugins is necessary so discover_sdk() exposes client.safe_synthesizer after a normal uv sync / bootstrap. Without this, tutorial/SDK examples fail unless users manually enable the plugin.
No change needed — flagging as intentional and correct.
tylersbray
left a comment
tylersbray
left a comment
There was a problem hiding this comment.
Left a few comments...
Signed-off-by: mkornfield <mkornfield@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/safe-synthesizer/sdk-resources.md (1)
10-57: ⚖️ Poor tradeoffAdd CLI examples alongside Python SDK examples.
Coding guidelines require both Python SDK and CLI examples in tab-sets. Document CLI equivalents for job creation and monitoring where available.
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 `@docs/safe-synthesizer/sdk-resources.md` around lines 10 - 57, Add CLI examples next to the Python SDK examples using tab-sets: keep the existing Python block (NeMoPlatform and SafeSynthesizerJobBuilder usage) and add a parallel CLI tab that shows equivalent commands for client.safe_synthesizer operations (create/list/retrieve/get_status/get_logs) and for the SafeSynthesizerJobBuilder flow (upload/data-source, synthesize, create-job), naming commands and flags that map to the SDK method signatures (create with --spec/--name/--workspace/--project/--timeout, list with --workspace, retrieve/get_status/get_logs with <name> and --workspace, plus CLI flags for classify model provider and replace-pii used in the builder). Ensure the docs mention the async namespace only for SDK and keep CLI examples in their own tab, matching the same examples/parameters and brief usage notes.
🤖 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 `@docs/safe-synthesizer/about/index.md`:
- Line 109: The Markdown list item "- **[Local and Subprocess
Execution](host-local-development.md)**" has three spaces after the list marker;
change it so there is exactly one space after the dash (i.e., "- **[Local and
Subprocess Execution](host-local-development.md)**") to conform to Markdown list
spacing conventions.
---
Nitpick comments:
In `@docs/safe-synthesizer/sdk-resources.md`:
- Around line 10-57: Add CLI examples next to the Python SDK examples using
tab-sets: keep the existing Python block (NeMoPlatform and
SafeSynthesizerJobBuilder usage) and add a parallel CLI tab that shows
equivalent commands for client.safe_synthesizer operations
(create/list/retrieve/get_status/get_logs) and for the SafeSynthesizerJobBuilder
flow (upload/data-source, synthesize, create-job), naming commands and flags
that map to the SDK method signatures (create with
--spec/--name/--workspace/--project/--timeout, list with --workspace,
retrieve/get_status/get_logs with <name> and --workspace, plus CLI flags for
classify model provider and replace-pii used in the builder). Ensure the docs
mention the async namespace only for SDK and keep CLI examples in their own tab,
matching the same examples/parameters and brief usage notes.
🪄 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: 8218c7dd-ef80-45a0-b5bd-e48ef1360886
📒 Files selected for processing (18)
docs/_snippets/nvidia-build-model-provider.mddocs/safe-synthesizer/about/index.mddocs/safe-synthesizer/about/jobs.mddocs/safe-synthesizer/about/reference.mddocs/safe-synthesizer/getting-started.mddocs/safe-synthesizer/sdk-resources.mddocs/safe-synthesizer/tutorials/safe-synthesizer-101.mdmkdocs.ymlpackages/nemo_platform/BUNDLING.mdplugins/nemo-safe-synthesizer/src/nemo_safe_synthesizer_plugin/api/v2/jobs/endpoints.pyplugins/nemo-safe-synthesizer/src/nemo_safe_synthesizer_plugin/job_config.pyplugins/nemo-safe-synthesizer/src/nemo_safe_synthesizer_plugin/sdk/config.pyplugins/nemo-safe-synthesizer/src/nemo_safe_synthesizer_plugin/sdk/job.pyplugins/nemo-safe-synthesizer/src/nemo_safe_synthesizer_plugin/sdk/job_builder.pyplugins/nemo-safe-synthesizer/src/nemo_safe_synthesizer_plugin/sdk/resources.pyplugins/nemo-safe-synthesizer/src/nemo_safe_synthesizer_plugin/tasks/safe_synthesizer/__main__.pyplugins/nemo-safe-synthesizer/tests/unit/test_jobs.pyplugins/nemo-safe-synthesizer/tests/unit/test_sdk.py
✅ Files skipped from review due to trivial changes (6)
- docs/safe-synthesizer/about/jobs.md
- packages/nemo_platform/BUNDLING.md
- plugins/nemo-safe-synthesizer/src/nemo_safe_synthesizer_plugin/tasks/safe_synthesizer/main.py
- docs/_snippets/nvidia-build-model-provider.md
- docs/safe-synthesizer/getting-started.md
- docs/safe-synthesizer/tutorials/safe-synthesizer-101.md
🚧 Files skipped from review as they are similar to previous changes (5)
- docs/safe-synthesizer/about/reference.md
- plugins/nemo-safe-synthesizer/src/nemo_safe_synthesizer_plugin/api/v2/jobs/endpoints.py
- mkdocs.yml
- plugins/nemo-safe-synthesizer/src/nemo_safe_synthesizer_plugin/sdk/resources.py
- plugins/nemo-safe-synthesizer/src/nemo_safe_synthesizer_plugin/sdk/job.py
Signed-off-by: mkornfield <mkornfield@nvidia.com>
Signed-off-by: mkornfield <mkornfield@nvidia.com>
4 participants
Summary by CodeRabbit