[namanraii]

Fixes #996

Summary

Drop flags were derived in a way that depended on the order columns were added to the config, so the same set of columns could produce different drop behavior depending on declaration order. This change derives drop flags from the full config, making them order-independent. Touches the config builder, visualization helper, engine compiler, and dataset builder.

Tests

Added regression tests covering different column orderings in:

• tests/config/test_config_builder.py
• tests/config/utils/test_visualization.py
• engine/.../dataset_builders/test_dataset_builder.py
• tests/interface/test_data_designer.py

Notes

test_import_performance fails locally on plain main as well, so that failure is pre-existing and unrelated to this change.

[github-actions]

All contributors have signed the DCO ✍️ ✅
_{Posted by the DCO Assistant Lite bot.}

[namanraii]

I have read the DCO document and I hereby sign the DCO.

[greptile-apps]

Confidence Score: 5/5

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

Summary

Derives processor-driven drop flags from the complete config, regardless of declaration order.

• Built configs mark processor targets as dropped, whatever order they were added in.
• Sample previews skip columns dropped by a processor.
• Dataset generation and analysis follow processor drop targets consistently.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Builder columns and processors] --> B[Build effective config]
  B --> C[Resolve processor drop targets]
  C --> D[Display records and build dataset]
  A --> E[Export authored column flags]
  E --> F[Reload builder]
  F --> B

Loading

_{Reviews (7) · Last reviewed commit: "test: avoid engine dependency in display..." · Reviewed by Greptile}

[nabinchha]

Thanks for putting this together, @namanraii — resolving drops from the complete configuration follows the direction agreed in #996.

Summary

This addresses declaration-order dependence, preserves explicitly declared drop flags during processor replacement, and prevents redundant default drops for glob targets. One save/reload/edit regression remains; that lifecycle was not specified in the original acceptance criteria, and the plan explicitly calls out the behavior.

Findings

Critical — Let's fix these before merge

packages/data-designer-config/src/data_designer/config/config_builder.py:434–436 — [P2] Preserve authored drop flags in builder exports

• What: build() now materializes processor-derived drop=True, and get_builder_config() / write_config() export that effective state. After from_config(), those derived flags become permanent declarations. Configure cleanup to drop tmp_a, save/reload the builder, then replace cleanup to drop only tmp_b: both columns remain dropped. The same edit on the original builder drops only tmp_b. A sampler-only generation run confirmed the output difference: the original builder emits ["keep", "tmp_a"], while the reloaded builder emits only ["keep"].
• Why: Saving and reloading changes subsequent editing behavior. The engine adds its default drop processor for the stale tmp_a flag, excluding a column the replacement processor no longer targets. I reproduced this against the PR and its parent: the parent clears tmp_a in both cases. Although the plan acknowledges the effective-to-explicit conversion, this is a user-visible compatibility regression worth resolving before merge.
• Suggestion: Keep effective flags in build(), but preserve authored column flags in get_builder_config() / write_config() so they can be recomputed after reload. This can be addressed without adding a public schema field. Add a save → reload → replace regression test, including an explicitly declared drop=True column that must remain dropped. Keep compiled run artifacts as effective execution snapshots: resume checks compare their fingerprints. Direct serialization of build() can be documented separately with those snapshot semantics.

What Looks Good

• Deriving flags on copies removes the original order dependence without mutating the builder's column declarations.
• The engine reuses its existing resolver for glob-aware default-drop suppression while preserving processor order and package layering.
• Sampler-only integration coverage checks both emitted columns and profiling with preserve_dropped_columns=False.

Residual Risk

Reviewed head ca3f9b6994ae875ab2c47937ac881ba1ac9ab3f7, verified unchanged at the end of the review. Validation across rounds: 1,193 tests passed across the complete config suite, related compiler/validation/processing tests, dataset builder tests, the affected interface test module, and all 78 composite workflow tests. Ruff lint and formatting checks passed on all eight changed Python files. The reload regression was separately reproduced against the PR and its parent, then verified in actual dataset generation. Additional targeted probes covered custom config copying and 50,000 builder state transitions without another confirmed defect. The full repository suite was not run.

Verdict

Needs changes — preserve authored flags in builder exports and add the reload-and-replace regression test. No additional confirmed findings.

---

This review was generated by an AI assistant.

[namanraii]

Thanks for catching this. get_builder_config() and write_config() now export the column drop flags as you wrote them, so a reloaded builder recomputes the processor drops. build() still returns the effective flags, and run artifacts are unchanged. I added a save, reload and replace test (YAML and JSON) that includes a column explicitly set to drop=True, and updated the plan doc to say that serializing build() output directly gives a snapshot.

[nabinchha]

Thanks for addressing the builder export regression, @namanraii!

Summary

Re-reviewed the full PR at 08099439b61030e670411c33a106acaf921e61b5. The previous save/reload/replace correctness finding is resolved; the following repository style-guide violations in added code need to be fixed before merge.

Findings

Warnings — Worth addressing

packages/data-designer-config/tests/config/test_config_builder.py:1037,1053,1060,1068,1077,1230 — Add type annotations to the new tests

• What: Six added config-builder tests omit parameter types and -> None. The new tests in packages/data-designer-engine/tests/engine/dataset_builders/test_dataset_builder.py:539 and packages/data-designer/tests/interface/test_data_designer.py:1440 do the same.
• Why: STYLEGUIDE.md requires annotations on all functions, explicitly including tests, and DEVELOPMENT.md requires typed fixture parameters and -> None. The current Ruff configuration does not enforce this requirement, so a clean lint result does not establish compliance.
• Suggestion: Annotate the fixture and parametrized arguments and add -> None to all eight added tests. This request is scoped to the added tests, rather than existing untyped tests elsewhere in the files.

packages/data-designer-config/tests/config/test_config_builder.py:1038 — Move the non-capturing sampler helper out of the test

• What: test_build_drop_flags_do_not_depend_on_call_order() defines a nested sampler() function that captures no local state.
• Why: The Code Organization rules allow nested functions only for closures that genuinely need captured state.
• Suggestion: Move the helper to module scope with an action-verb name such as make_sampler_column, or use an appropriate fixture.

packages/data-designer-config/src/data_designer/config/config_builder.py:426 — Put the new private helper after the public methods

• What: _get_columns_with_processor_drops() is introduced before public methods such as add_profiler() and build().
• Why: The Code Organization rules require public methods before private helpers. The existing file already mixes those sections, but this newly added helper extends that ordering violation.
• Suggestion: Place the new helper after the public API methods. A broader reorganization of unrelated existing methods is not required for this request.

packages/data-designer-config/tests/config/test_config_builder.py:995 — Use an action verb for the drop-flags helper

• What: The new helper is named _built_drop_flags().
• Why: The Naming Conventions require function and method names to start with an action verb.
• Suggestion: Rename it to _get_built_drop_flags() and update its callers.

What Looks Good

• Authored flags now survive get_builder_config() / write_config() and reload, while build() still derives effective flags. The original generation-level regression now produces the same correct columns with the original and reloaded builder.
• The YAML and JSON regressions cover processor replacement while retaining explicitly declared drops.
• Compiled run artifacts remain effective execution snapshots, preserving resume fingerprint semantics and the package dependency direction.

Verdict

Needs changes — fix all four style items above before shipping. The previous correctness blocker is resolved; these remaining requests enforce the repository's documented conventions.

Validation of the current implementation: all three package suites were exercised, with 4,435 passed and 3 skipped in the full run. Six attribution-header failures were caused by my telemetry-disable test setting; all 14 related tests, including those six, passed with the expected default restored. Ruff lint and formatting passed for all eight changed Python files. Additional independent lifecycle, plugin, fingerprint, and rendering checks found no new correctness issue.

---

This review was generated by an AI assistant.

[namanraii]

Thanks for the re-review, @nabinchha! Addressed all four items in 04b9dba:

• Added full type annotations and -> None to all eight new tests, plus the new builder_with_columns fixture.
• Moved the nested sampler helper to module scope as make_sampler_column.
• Moved _get_columns_with_processor_drops() (and its companion _resolve_processor_dropped_column_names()) after the public methods, with no other reordering.
• Renamed _built_drop_flags to _get_built_drop_flags and updated all callers.

No behavior changes. Ruff lint and format pass, and the config (667), engine (2579) and interface (1194) suites pass.

[nabinchha]

Thanks for addressing the four style requests, @namanraii!

Summary

Re-reviewed 04b9dba49767be485030767d300dce23627273c0. All four posted requests are addressed, and the previous correctness fix remains sound. One small ordering mismatch remains when applying the same documented convention to the new test helper.

Findings

Warnings — Worth addressing

packages/data-designer-config/tests/config/test_config_builder.py:995 — Put the private test helper after public functions

• What: _get_built_drop_flags() still appears before the public make_sampler_column() helper and the public test functions. The production helpers were moved as requested, but the new private test helper retains the opposite order.
• Why: STYLEGUIDE.md specifies that public functions/methods precede private ones in both modules and classes. This is a small style-only mismatch, with no observed runtime effect; it remains relevant to the requested pre-merge style compliance.
• Suggestion: Move _get_built_drop_flags() below the public test functions/classes at the end of the module. No behavior change or broader reorganization is needed.

What Looks Good

• All eight added tests and the touched fixture now have parameter and return annotations.
• make_sampler_column() is at module scope, _get_built_drop_flags() uses an action verb, and both new builder helpers follow public methods.
• The latest commit preserves production method bodies and test coverage. Independent YAML/JSON reload-and-replace probes confirm authored drops remain preserved and former processor targets are restored.

Residual Risk

Validation: 248 tests passed across the four affected test modules; Ruff lint and formatting checks passed for all eight changed Python files. Independent AST comparison confirmed production code is unchanged apart from method ordering and test assertions/decorators remain equivalent after helper/annotation normalization. The full package suites were exercised in the prior review and were not repeated for these style-only edits.

Verdict

Needs changes — one final test-helper ordering cleanup for complete compliance with the documented style rule. All four previously posted requests are resolved; there are no new correctness findings.

---

This review was generated by an AI assistant.

[namanraii]

Thanks @nabinchha! Moved _get_built_drop_flags() to the end of test_config_builder.py, after all public tests, in 0c88d5a. Pure move, no behavior change. Ruff lint/format pass, and the config suite (667) plus all PR-touched test modules pass.

[nabinchha]

Thanks for addressing the review feedback, @namanraii!

Summary

Reviewed the final revision, including removal of the unnecessary implementation plan. The order-independent drop behavior, authored-config round trips, and requested style fixes are complete.

Findings

No remaining actionable findings. The previously reported correctness and style issues are resolved, and the scoped security review found no vulnerabilities introduced by these changes.

What Looks Good

• Builder exports preserve authored flags while execution resolves effective drops.
• Regression tests cover call order, processor replacement, explicit drops, and YAML/JSON reloads.
• The final commit deletes only the plan; the PR description no longer references it.

Verdict

Ship it. Approved at a7fab32d061655ee73d304eb2818707c5a0019bd. Prior test, lint, formatting, lifecycle, and security-review evidence remains applicable; no runtime code changed in the final plan-removal commit.

---

This review was generated by an AI assistant.