Skip to content

feat: define public generation invocation contract - #854

Open
andreatnvidia wants to merge 8 commits into
mainfrom
andreatnvidia/feat/public-generation-contract
Open

feat: define public generation invocation contract#854
andreatnvidia wants to merge 8 commits into
mainfrom
andreatnvidia/feat/public-generation-contract

Conversation

@andreatnvidia

@andreatnvidia andreatnvidia commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

📋 Summary

Defines a stable public generation contract for optional execution packages so they can configure, validate, invoke, and classify Data Designer runs without importing data_designer.engine. It also normalizes builder-loading failures and exposes public outcome metadata for complete, partial, resumed, and early-shutdown invocations.

🔗 Related Issue

Closes #851

Related to #850

🔄 Changes

  • Document the supported public imports, serialized builder boundary, validation ownership, model binding, runtime configuration, results, errors, and known limits.
  • Export InvalidConfigError, InvalidFileFormatError, and InvalidFilePathError from data_designer.config, and ArtifactStorageError from data_designer.interface.
  • Normalize missing, unreadable, and malformed local builder configuration failures to public error types.
  • Expose resolved dataset path, requested and actual record counts, partial and early-shutdown status, and requested and effective resume modes on DatasetCreationResults.
  • Preserve requested record counts when reconstructing completed workflow stages and cover collision, resume, partial, early-shutdown, and failure behavior.

🔍 Attention Areas

⚠️ Reviewers: Please pay special attention to the following:

🧪 Testing

  • make test passes - not run; targeted package suites were used.
  • .venv/bin/pytest packages/data-designer-config/tests packages/data-designer/tests -p no:cacheprovider (1772 passed, 1 skipped)
  • Unit tests added/updated.
  • E2E tests added/updated - N/A, no E2E behavior changed.
  • .venv/bin/ruff check --fix .
  • .venv/bin/ruff format .
  • Existing import-performance threshold.

✅ Checklist

  • Follows commit message conventions.
  • Commits are signed off (DCO Assistant passes).
  • Architecture docs updated with the public contract proposal.

@github-actions

Copy link
Copy Markdown
Contributor

Fern preview: https://nvidia-preview-pr-854.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.

@andreatnvidia
andreatnvidia marked this pull request as ready for review August 10, 2026 15:46
@andreatnvidia
andreatnvidia requested a review from a team as a code owner August 10, 2026 15:46
@greptile-apps

greptile-apps Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Defines a stable public generation interface so optional execution packages can configure and invoke Data Designer without importing engine internals.

  • Exports public configuration and artifact-storage error types.
  • Normalizes missing, unreadable, and malformed builder-input failures.
  • Adds dataset path, record-count, partial-run, early-shutdown, and resume metadata to generation results.
  • Preserves requested record counts when reconstructing completed workflow stages.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains; both previously reported missing-path classification issues are addressed by case-insensitive extension handling and conversion of parser failures for path-shaped inputs.

Important Files Changed

Filename Overview
packages/data-designer-config/src/data_designer/config/utils/io_helpers.py Missing supported-extension paths are now classified case-insensitively, including malformed filenames that fail YAML parsing.
packages/data-designer-config/src/data_designer/config/config_builder.py Builder loading now converts filesystem, decoding, and YAML parsing failures into public configuration errors.
packages/data-designer/src/data_designer/interface/results.py Results expose public invocation metadata and consistently report missing dataset artifacts through ArtifactStorageError.
packages/data-designer/src/data_designer/interface/data_designer.py Generation results now retain requested counts, shutdown state, and requested and effective resume modes.
packages/data-designer/src/data_designer/interface/composite_workflow.py Reconstructed completed stages retain the persisted requested record count for partial-result classification.
plans/850/data-designer-contract.md Documents the supported public invocation boundary, validation ownership, runtime configuration, outcomes, errors, and known limits.

Sequence Diagram

sequenceDiagram
    participant Client as Optional execution package
    participant Config as DataDesignerConfigBuilder
    participant DD as DataDesigner
    participant Engine as Generation engine
    participant Results as DatasetCreationResults
    Client->>Config: from_config(serialized builder)
    Config-->>Client: Validated public builder
    Client->>DD: validate(builder)
    Client->>DD: create(builder, count, name, resume)
    DD->>Engine: Build and profile dataset
    Engine-->>DD: Artifacts and invocation outcome
    DD-->>Results: Construct public result metadata
    Results-->>Client: Dataset path, counts, status, resume modes
Loading

Reviews (6): Last reviewed commit: "fix: classify malformed config paths" | Re-trigger Greptile

Comment thread packages/data-designer-config/src/data_designer/config/utils/io_helpers.py Outdated
@nabinchha

Copy link
Copy Markdown
Contributor

Thanks for putting this together, @andreatnvidia!

Summary

This PR defines the optional-package invocation boundary, promotes the relevant public errors, and adds result metadata for record counts, partial completion, early shutdown, artifact resolution, and resume decisions. The implementation mostly matches that intent, but a valid inline-config case now breaks and a few documented public error/outcome guarantees are not yet enforced.

Findings

Critical — Let's fix these before merge

packages/data-designer-config/src/data_designer/config/utils/io_helpers.py:193 — Valid inline YAML ending in .json is treated as a missing file

  • What: The new suffix check runs before parsing and classifies any string whose final characters are .json as a file path. A valid inline builder whose last scalar is a JSON seed path (for example, a YAML document ending in path: /tmp/seed.json) now raises InvalidFilePathError. I reproduced this with DataDesignerConfigBuilder.from_config(...); the same payload is accepted on main.
  • Why: Inline YAML is an existing supported input to from_config(), so this breaks valid configurations while trying to normalize nonexistent JSON paths.
  • Suggestion: Distinguish a parsed mapping from a filename before raising FileNotFoundError—for example, parse string input first and apply the missing-path classification only when the parsed value is not a mapping and the original string looks like a supported filename. Please add a regression test with a valid inline builder ending in a .json field value (and ideally cover the existing .yaml/.yml ambiguity too).

Warnings — Worth addressing

packages/data-designer-config/src/data_designer/config/config_builder.py:129 — Undecodable local configs bypass the public error contract

  • What: The normalization catches OSError and yaml.YAMLError, but reading invalid UTF-8 raises UnicodeDecodeError, which escapes unchanged. I reproduced this with an existing .yaml file containing invalid UTF-8 bytes.
  • Why: The PR promises public error types for unreadable/malformed local builder configs; callers following that contract cannot handle this case without depending on another implementation exception.
  • Suggestion: Normalize UnicodeDecodeError (or the appropriate UnicodeError family) to InvalidFileFormatError and add a binary-file regression test.

packages/data-designer/src/data_designer/interface/results.py:94 — Missing result artifacts are reported as zero records

  • What: The new actual_num_records property delegates to count_records(), whose empty glob sums to 0. With an empty final dataset directory, I observed actual_num_records == 0, is_partial is True, and load_dataset() returning an empty frame instead of the ArtifactStorageError promised by the new contract.
  • Why: An embedding package can misclassify missing or moved artifacts as a legitimate partial/zero-record outcome, undermining the outcome metadata this PR introduces.
  • Suggestion: Have count_records()/actual_num_records raise public ArtifactStorageError when no final batches exist and normalize missing-dataset failures from load_dataset() at the public results boundary. If zero is intentionally supported, narrow the contract and add tests that make the distinction explicit.

fern/versions/latest/pages/concepts/architecture-and-performance.mdx:390 — Published early-shutdown guidance contradicts the new contract

  • What: Fern says crossing the threshold always raises DataDesignerEarlyShutdownError, while this PR formalizes two outcomes: zero records raises, but surviving records return a partial DatasetCreationResults with early_shutdown=True. The example also imports the error from the internal data_designer.interface.errors module, and no Fern page documents the newly public outcome fields.
  • Why: Users following the published guidance may wait for an exception and silently accept a partial return without checking it.
  • Suggestion: Update this page in the same PR to use the public root import and demonstrate is_partial/early_shutdown (plus the requested/effective resume fields and dataset_path where resume is documented).

What Looks Good

  • The lazy root exports preserve the package layering and import-time pattern while giving callers stable import locations.
  • The requested-versus-effective resume distinction is derived from the engine's resolved storage mode, and the tests cover successful resume, incompatible IF_POSSIBLE fallback, collisions, partial results, and reconstructed workflow stages.
  • The new tests are behavior-oriented; all current GitHub checks are green, and changed-file Ruff checks plus 309 focused config/interface tests pass locally.

Verdict

Needs changes — fix the inline-YAML regression, complete the promised public error normalization for malformed/missing artifacts, and align the published run-outcome documentation with the new contract.


This review was generated by an AI assistant.

Signed-off-by: Andre Manoel <amanoel@nvidia.com>
Comment thread packages/data-designer-config/src/data_designer/config/utils/io_helpers.py Outdated
@andreatnvidia

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed review, @nabinchha. I pushed cf0f0bdf to address each finding.

The config loader now parses inline YAML before deciding that a string is a missing file path, and invalid UTF-8 is normalized to InvalidFileFormatError. The public results boundary now raises ArtifactStorageError when final batch artifacts are missing or inaccessible. I also updated the Fern guidance to reflect both early-shutdown outcomes and to use the public imports and result metadata.

I added focused regression coverage for these cases. Ruff, the 839 config/interface tests, and the Fern docs checks pass locally.

Signed-off-by: Andre Manoel <amanoel@nvidia.com>
Signed-off-by: Andre Manoel <amanoel@nvidia.com>
@nabinchha

Copy link
Copy Markdown
Contributor

Thanks for the quick follow-up, @andreatnvidia!

Summary

The three new commits address the four findings from my earlier review and the subsequent Greptile path-classification comment, with focused tests and updated Fern guidance. Two edge cases still leave the public error boundary dependent on input shape or raw filesystem exceptions.

Findings

Warnings — Worth addressing

packages/data-designer-config/src/data_designer/config/utils/io_helpers.py:197 — Malformed inline configs ending in a supported extension are reported as missing files

  • What: On a YAML parser error, is_local_config_path is determined only from the string suffix. As a result, DataDesignerConfigBuilder.from_config("data_designer: [.json") raises InvalidFilePathError (caused by FileNotFoundError), while the same malformed inline document without that suffix raises InvalidFileFormatError.
  • Why: The new contract promises InvalidFileFormatError for malformed inline builder data, so classification currently changes based on the final characters of the malformed payload.
  • Suggestion: Could we distinguish path-shaped strings from inline documents before converting parser errors—using an explicit Path, absolute/path-separator evidence, or another unambiguous heuristic—and add a malformed-inline regression case ending in .json/.yaml?

packages/data-designer/src/data_designer/interface/results.py:158 — Record counting still leaks raw filesystem errors

  • What: _get_batch_files() now handles an empty glob, but the subsequent lazy.pq.read_metadata() calls are outside the new OSError normalization. I reproduced count_records() raising raw PermissionError from an unreadable batch_00000.parquet; actual_num_records and is_partial inherit the same behavior.
  • Why: load_dataset() now exposes ArtifactStorageError for inaccessible artifacts, but the other public outcome properties still require callers to know about filesystem exceptions.
  • Suggestion: Wrap OSError from metadata reads as ArtifactStorageError (and apply the same boundary consistently to export reads if inaccessible artifacts are part of that contract), with a regression test for an unreadable or disappearing batch.

What Looks Good

  • Valid inline mappings ending in .json/.yaml/.yml now load correctly, and invalid UTF-8 is normalized to the public format error.
  • Missing or empty final-batch directories now fail through a shared ArtifactStorageError path, and the Fern outcome/resume guidance matches the new API.
  • Both Greptile threads are resolved and outdated; all current CI checks are green, and 316 focused tests plus changed-file Ruff checks pass locally.

Verdict

Needs changes — clarify the remaining path-versus-inline parser ambiguity and finish normalizing inaccessible batch reads across the public result-counting boundary.


This review was generated by an AI assistant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Define the public Data Designer contract used by the optional Slurm package

2 participants