[andreatnvidia]

Summary

Allow callers such as UserSim to obtain model facades from their own configs without creating artifact directories or requiring person assets. Existing calls keep default configs and synchronous clients; async mode is explicit.

Related Issue

Closes #1048

Changes

• Add keyword-only model_configs (a config list, YAML string, or file path) and client_concurrency_mode to DataDesigner.get_models.
• Use the existing model-registry factory with the instance's providers, secret resolver, shared admission controller, runtime config, and request event sink.
• Validate requested aliases before creating clients, reject duplicate config aliases, and document close() / aclose() cleanup.
• Protect standalone alias limits on the shared controller: reject conflicting provider/model/alias limits before creating clients or changing admission state. Pipeline-only concurrency reconfiguration keeps its existing behavior.
• Construct facades before committing request limits as an atomic batch. Failed setup closes only new clients and preserves existing caps, freezes, adaptive state, and cached facades, including when a competing registration appears during construction.
• Document that protected limits persist after facade cleanup and require set_run_config() or a new DataDesigner instance to change.
• Replace the resource-provider mock test with 25 public API cases covering sync and async requests, config loading, validation, retries, usage, shared admission limits, failed-construction cleanup, and absence of artifact/person-reader side effects.

Testing

• .venv/bin/pytest -q packages/data-designer/tests: 1,260 passed, 11 skipped.
• .venv/bin/pytest -q packages/data-designer-engine/tests: 2,876 passed, including batch registration and failed-construction regressions.
• Ruff lint and formatting checks on changed files.
• Self-review and independent Claude review of API compatibility, resource ownership, and factory wiring.
• Native loopback HTTP smoke: 20 scenarios covering standalone sync/async calls, retries, admission limits, transactional failure recovery, and dataset preview/create/acreate with reload/export.
• Live NVIDIA Inference smoke using openai/openai/gpt-5.4-nano: sync YAML config, async writer/judge flows, usage accounting, sibling cleanup, failed-setup retry, and two-row async dataset creation alongside a standalone call, followed by Parquet reload and JSONL export.
• NVIDIA Build live completions returned HTTP 403 (PERMISSION_DENIED) for both the current default nvidia/nemotron-3.5-lightning-30b-a3b and listed alternative nvidia/nemotron-nano-3-30b-a3b; Build completion compatibility remains unverified.

Checklist

• Follows commit message conventions.
• Commits are signed off (DCO).
• Linked issue is open and labeled triaged.
• Architecture docs: not applicable; existing factories and client lifecycle are unchanged.

[greptile-apps]

Confidence Score: 5/5

[Medium impact] The PR appears safe to merge; no new actionable issues were found.

Summary

Adds custom configs and explicit async clients to DataDesigner.get_models without creating dataset artifacts or loading person assets.

• Callers can build model facades directly from custom configs.
• Shared request limits stay consistent across standalone facades and dataset calls.

The previous unnumbered thread is not reposted. Conflicting limits are now rejected. andreatnvidia documents that limits remain after facade cleanup; that behavior is intentional. The thread was manually resolved without explanation.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["get_models"] --> B["Load configs and check aliases"]
    B --> C["Check providers and existing limits"]
    C --> D["Construct new facades"]
    D --> E["Check and register all limits under one lock"]
    E --> F["Return requested facades"]
    D -- Failure --> G["Close new facades and raise"]
    E -- Conflict --> G

Loading

_{Reviews (4) · Last reviewed commit: "Merge branch 'main' into andreatnvidia/f..." · Reviewed by Greptile}

[github-actions]

Fern preview: https://nvidia-preview-pr-1108.docs.buildwithfern.com/nemo/datadesigner

Fern previews include the docs-website version archive with PR changes synced into latest. Notebook tutorials are rendered without execution outputs in previews.

[svcnvidia-nemo-ci]

Reviewed all three changed files and traced configuration loading, facade/client lifecycle, and shared runtime wiring against the base. One correctness issue remains: independent custom-config calls can overwrite a live facade’s concurrency limit.

Static review only; tests were not executed. One delegated transport/lifecycle report was unavailable, so those paths were independently inspected.

Note: formal approval is not available for this request, so this review is posted as a comment only.

Review generated by codex. This was a full review. Request a full review with /review reset=true.

[nabinchha]

Thanks for putting this together, @andreatnvidia!

Summary

Adds custom model configurations and explicit async mode to DataDesigner.get_models(), without dataset artifact or person-reader setup. The admission-limit revision addresses the earlier overwrite feedback, but failed construction still leaves a persistent registration.

Findings

Critical — Let's fix these before merge

packages/data-designer/src/data_designer/interface/data_designer.py:703 — [P2] Roll back registrations when model construction fails

• What: The comprehension creates and freezes models one at a time. If a later model fails to construct, earlier registrations remain on the designer even though no facade dictionary was returned. I reproduced this with aliases a and b: a uses a valid provider and cap 1; b names an unknown provider. The call raises UnknownProviderError, but retrying only a with cap 2 then raises the new conflicting-limit ValueError.
• Why: A failed operation leaves an unowned frozen cap that can reject later valid calls or throttle other aliases sharing the provider/model. Recovery requires replacing the shared controller via set_run_config(), which can also separate existing live facades from subsequent work.
• Suggestion: Make construction and registration transactional: preflight provider configuration, close any clients constructed before failure, and roll back only registrations/freezes introduced by this call while preserving prior registrations. Add a regression that fails on a later alias, retries the earlier alias with another limit, and verifies both successful construction and unchanged pre-existing registrations.

What Looks Good

• Uses the existing registry factory and preserves shared admission state.
• Public API tests cover sync/async requests, config sources, retry behavior, and side-effect avoidance.
• Conflicting limits are rejected before construction in the ordinary reuse path.

Residual Risk

99 tests passed: 19 public API tests and 80 registry/admission tests, along with Ruff lint and formatting. The failure scenario above was reproduced offline without provider requests.

Verdict

Needs changes — fix partial-construction registration rollback before merge

---

This review was generated by an AI assistant.

[andreatnvidia]

Thanks for catching this, @nabinchha!

Fixed in ecc8a8b5. Construction now stages new facades before committing their admission limits as one atomic batch. Failed setup closes only newly created clients and preserves existing registrations, freezes, and cached facades.

Added regressions for failure on a later alias, retrying the earlier alias with a different cap, and preserving pre-existing limits and live facades.

[nabinchha]

Thanks for putting this together, @andreatnvidia!

Summary

Reviewed PR #1108 at ecc8a8b5a03a296bd2cff36da4f416910091be4d against 4bb221eb60e11feb40292e5e81445e12bd666826, including the linked follow-up comment. The custom-config and explicit async API matches the stated intent, and staged construction with atomic registration resolves the previous partial-construction blocker.

Findings

No new actionable findings.

What Looks Good

• Shared admission changes occur only after facade construction, with the entire registration batch revalidated under the controller lock.
• Failure cleanup preserves cached facades, active leases, adaptive limits, cooldowns, existing caps, and prior freeze state.
• Public API coverage and additional adversarial probes exercise config sources, concurrent setup, native sync/async cleanup, provider/generation variants, and observer failures.

Residual Risk

The expanded interface and engine-model suite passed: 2,057 passed, 11 skipped. Ruff lint and formatting passed for all eight changed files; the diff whitespace check also passed. Additional probes included 50 competing-registration trials, failed construction during live admission/cooldown state, cleanup and observer failures, and native OpenAI-compatible and Anthropic cleanup.

Live provider requests and the complete non-model engine suite were not rerun. Custom factories returning already-used clients were outside the validated staging assumptions.

Verdict

Ship it — the previous blocker is resolved, with no additional changes requested.

---

This review was generated by an AI assistant.