Add stable retrieval SDG recipe APIs - #54
Conversation
| output_path=output_path, | ||
| dataset_path=Path(result.artifact_storage.base_dataset_path), | ||
| dataset_name=resolved_dataset_name, | ||
| num_records=num_records, |
There was a problem hiding this comment.
Edge case: I wonder if it makes sense to use result.count_records() rather than the requested num_records? Is there a scenario where Data Designer would produce fewer rows than num_records. If it happens, the GenerationResult could disagree with the exported JSONL?
we could record both, with num_records as results.count_records() and add an extra field for the requested num_records?
There was a problem hiding this comment.
Generation reports actual and requested record counts separately.
| min_hops: int = Field(default=DEFAULT_MIN_HOPS, ge=1) | ||
| max_hops: int = Field(default=DEFAULT_MAX_HOPS, ge=1) | ||
| reasoning_counts: dict[str, NonNegativeInt] = Field(default_factory=lambda: dict(DEFAULT_REASONING_COUNTS)) | ||
| min_complexity: int = Field(default=DEFAULT_MIN_COMPLEXITY, ge=1) |
There was a problem hiding this comment.
do we need an upper bound le here, or is it valid to have any complexity of 1 or above?
There was a problem hiding this comment.
Added le=5 here
|
|
||
| max_artifacts_per_type: int = Field(default=DEFAULT_MAX_ARTIFACTS_PER_TYPE, ge=1) | ||
| num_pairs: int = Field(default=DEFAULT_NUM_PAIRS, ge=1) | ||
| query_counts: dict[str, NonNegativeInt] = Field(default_factory=lambda: dict(DEFAULT_QUERY_COUNTS)) |
There was a problem hiding this comment.
Might be worth adding a validator for query_counts and reasoning_counts to make sure the string keys provided match the expected set?
There was a problem hiding this comment.
Added validation for query/reasoning keys.
| model_config = ConfigDict(frozen=True) | ||
|
|
||
| max_artifacts_per_type: int = Field(default=DEFAULT_MAX_ARTIFACTS_PER_TYPE, ge=1) | ||
| num_pairs: int = Field(default=DEFAULT_NUM_PAIRS, ge=1) |
There was a problem hiding this comment.
Is there an assumption or requirement that num_pairs matches the sum of query_counts? It looks like it might based on the prompts. if so, maybe worth adding a validator to check this
There was a problem hiding this comment.
Default num_pairs is now 7. and add validation of the sum.
| resume=ResumeMode(config.resume), | ||
| ) | ||
|
|
||
| config.output_dir.mkdir(parents=True, exist_ok=True) |
There was a problem hiding this comment.
for slightly ealier failure it could be worth moving this before data_designer.create so we fail early if the output directory fails to be created for some reason (e.g. invalid, or inaccessible for some reason). we'd learn that before the run starts
There was a problem hiding this comment.
Output directory validation happens before generation starts.
oliverholworthy
left a comment
There was a problem hiding this comment.
The overall change looks like great refactor. I added a few minor suggestions that might improve config validation.
What
GenerationRunConfig,GenerationPipelineConfig, and immutable result metadata.run_conversion().num_pairsand use seven pairs by default.Why
The Nemotron embedding recipe currently consumes vendored retrieval SDG source and a separate conversion script. Replacing those copies with the released plugin requires stable package-level generation and conversion contracts, predictable artifacts, and reviewed conversion semantics. This change establishes that API and behavioral baseline before the recipe installs a released wheel.
Usage
How
The new public runner owns Data Designer setup, native resume, export, and stable result metadata. Explicit Pydantic-backed run and pipeline models reject unknown fields and replace the previous untyped
pipeline_kwargsboundary. Existing low-level pipeline defaults now share the same constants as the stable runner and CLI.Conversion remains backward-compatible with JSON, JSONL, Parquet, and legacy batch inputs, while ambiguous mixed-format directories fail with an actionable error. Identifier and seeded-split fixes prevent basename collisions and process-dependent split membership.
File-based config loading, default YAML files, resolved run snapshots, provenance, and resume fingerprints are intentionally reserved for a following PR. This PR does not add a plugin
llamaprofile; recipe-specific profile mapping remains owned by Nemotron.Validation
make test-plugin PLUGIN=data-designer-retrieval-sdg(106 passed)make lintmake validatemake checkgit diff --check