Separate converter 5 units. - #792
Open
Felipedino wants to merge 3 commits into
Open
Conversation
- Updated PrepareAndSplitUnit to require dataset_id and provide task_name. - Introduced SaveDatasetUnit for persisting datasets to disk. - Added comprehensive tests for SaveDatasetUnit and LoadDatasetUnit to ensure correct functionality. - Implemented contract tests for ApplyConverterUnit to validate context handling and converter behavior. - Enhanced unit contract tests to ensure all context keys are declared and properly managed.
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request refactors ConverterJob to execute dataset conversion via composable “units” (LoadDatasetUnit, ApplyConverterUnit, SaveDatasetUnit) rather than monolithic in-job logic, and adds contract-style tests to ensure unit context contracts remain reliable when units are reused/chained.
Changes:
- Refactored
ConverterJobto load/apply/save via unit execution with anExecutionContextand cache cleanup. - Added new units for applying a converter and saving datasets, and expanded
LoadDatasetUnitto support notebook-based loading while publishingdataset_id/dataset_path. - Added a contract audit test suite for units plus targeted unit and API regression tests for the new behavior.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/back/units/test_unit_contracts.py | Adds an AST-based audit ensuring units’ declared REQUIRES/PROVIDES match actual context usage. |
| tests/back/units/test_save_dataset_unit.py | Contract tests for SaveDatasetUnit behavior and error mapping. |
| tests/back/units/test_load_dataset_unit.py | Contract tests for LoadDatasetUnit (dataset vs notebook starting point) and published keys. |
| tests/back/units/test_fit_model_unit.py | Adds regression coverage ensuring FitModelUnit keeps per-instance state off the shared context. |
| tests/back/units/test_build_model_unit.py | Updates unit test wiring to include newly required context keys. |
| tests/back/units/test_apply_converter_unit.py | Extensive contract tests for converter application semantics (scope, chaining, errors). |
| tests/back/api/test_units_api.py | Extends units API schema assertions to include new units and updated load schema. |
| tests/back/api/test_converter_job.py | Adds end-to-end regression net for ConverterJob behavior after refactor. |
| DashAI/back/units/save_dataset_unit.py | Introduces SaveDatasetUnit to persist the context dataset to dataset_path. |
| DashAI/back/units/prepare_and_split_unit.py | Updates contract to declare dataset_id usage and provides task_name. |
| DashAI/back/units/load_dataset_unit.py | Enhances loading to support notebook working copies; publishes dataset, dataset_id, dataset_path. |
| DashAI/back/units/fit_model_unit.py | Moves optimizer/goal-metric caching to unit instance state; adds run_id requirement. |
| DashAI/back/units/build_model_unit.py | Declares run_id/task_name requirements and threads them into factory creation and errors. |
| DashAI/back/units/apply_converter_unit.py | Adds ApplyConverterUnit implementing scoped fit/transform with dataset replacement/merge semantics. |
| DashAI/back/job/converter_job.py | Refactors job execution to unit-based flow using ExecutionContext and cache clearing. |
| DashAI/back/initial_components.py | Registers new units in the initial component list. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…asetUnit - Introduced FitConverterUnit to fit a converter on a dataset without transforming it. - Introduced TransformDatasetUnit to apply an already fitted converter to a dataset. - Updated ApplyConverterUnit to utilize the new units for fitting and transforming. - Added ConverterScopeMixin for shared scope resolution logic between converter units. - Updated initial_components.py to include new units. - Added tests to ensure correct functionality of the new units and their interactions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request refactors the dataset converter job to use a unit-based execution model, improving modularity and maintainability. The previous implementation, which manually loaded datasets, instantiated converters, and handled transformations, is replaced with reusable units for loading datasets, applying converters, and saving datasets. This also removes a large block of custom code for applying converters and rebuilding datasets, delegating these responsibilities to the new units.
Refactoring to Unit-Based Execution
ConverterJobwith calls toLoadDatasetUnit,ApplyConverterUnit,SaveDatasetUnit,FitConverterUnit,TransformDataset, streamlining the workflow and reducing code complexity. [1] [2]_rebuild_dataset_with_transformed_columnsfunction and related manual code for applying converters and updating datasets, delegating this to the new units. [1] [2]Dependency and Import Updates
ApplyConverterUnit,LoadDatasetUnit,SaveDatasetUnit, andExecutionContexttoconverter_job.pyand registered these units ininitial_components.py. [1] [2] [3]Resource Management