Refactor/td integration #1437#1439
Conversation
… parameters, sampling, and prediction intervals
…loading and error handling
…poser requirements
…ased loading and error handling
…on with schema validation and error handling
… handling in PipelineRunConfig
|
Code in this pull request still contains PEP8 errors, please write the Comment last updated at Mon, 13 Jul 2026 21:41:17 |
| self.tensor_data_config: Dict[str, Any] = resolve_tensor_data_config( | ||
| self.get('tensor_data_config'), | ||
| use_preprocessing_cache=self.get('use_preprocessing_cache', True), | ||
| ) | ||
|
|
There was a problem hiding this comment.
why after the parameters: dict = self._params_repository.check_and_set_default_params(input_params) ?
There was a problem hiding this comment.
yes, but why
self.tensor_data_config: Dict[str, Any] = resolve_tensor_data_config(
self.get('tensor_data_config'),
use_preprocessing_cache=self.get('use_preprocessing_cache', True),
)after the
parameters: dict = self._params_repository.check_and_set_default_params(input_params)
super().__init__(...)| if target is not None: | ||
| spec_kwargs['target'] = target | ||
|
|
||
| return TensorDataCreationRequest(backend_name=backend_name, spec_kwargs=spec_kwargs) |
There was a problem hiding this comment.
is the TensorDataCreationRequest necessary?
There was a problem hiding this comment.
Now, it one of the way for correct aggregation TDCreator's params. But may be refactored later.
| _ALLOWED_KEYS: Set[str] = _USER_CONFIGURABLE_DATA_SPEC_KEYS | _CREATOR_ONLY_KEYS | ||
|
|
||
| # TODO romankuklo: add "cuda:n" to the supported backends | ||
| _SUPPORTED_BACKENDS = frozenset({'cpu', 'gpu'}) |
There was a problem hiding this comment.
this should be initialized inside the Backend singleton
There was a problem hiding this comment.
Added name normalization methods into Backend [66b1924]
| def validate_tensor_data_config(config: Optional[Dict[str, Any]]) -> Optional[Dict[str, Any]]: | ||
| """ | ||
| Validate user-provided ``tensor_data_config`` for :class:`~fedot.api.api_utils.params.ApiParams`. | ||
|
|
||
| The config is a flat dictionary of options forwarded to | ||
| :meth:`~fedot.core.data.tensor_data.tensor_data_creator.TensorDataCreator.create` | ||
| (as ``DataSpec`` kwargs) plus ``backend_name``. Runtime values such as ``task``, | ||
| ``state``, and ``target`` must not be set here — they are injected when data | ||
| is created during ``fit`` / ``predict``. | ||
|
|
||
| Args: | ||
| config: User config dictionary or ``None``. | ||
|
|
||
| Returns: | ||
| A shallow copy of the validated config, or ``None`` when *config* is ``None``. | ||
|
|
||
| Raises: | ||
| ValueError: If *config* is not a dict or contains unknown / forbidden keys. | ||
| """ | ||
| if config is None: | ||
| return None | ||
| if not isinstance(config, dict): | ||
| raise ValueError('"tensor_data_config" must be a dictionary or None.') | ||
|
|
||
| unknown_keys = set(config) - _ALLOWED_KEYS | ||
| if unknown_keys: | ||
| raise ValueError( | ||
| f'Unknown keys in "tensor_data_config": {sorted(unknown_keys)}' | ||
| ) | ||
|
|
||
| forbidden_keys = set(config) & _RUNTIME_KEYS | ||
| if forbidden_keys: | ||
| raise ValueError( | ||
| 'Keys reserved for runtime injection must not appear in ' | ||
| f'"tensor_data_config": {sorted(forbidden_keys)}' | ||
| ) | ||
|
|
||
| # TODO romankuklo: should remove from here? | ||
| normalized = dict(config) | ||
| if 'backend_name' in normalized: | ||
| normalized['backend_name'] = _normalize_backend_name(normalized['backend_name']) | ||
|
|
||
| if 'use_cache' in normalized and not isinstance(normalized['use_cache'], bool): | ||
| raise ValueError('"tensor_data_config.use_cache" must be a boolean.') | ||
|
|
||
| return normalized |
| ) | ||
|
|
||
|
|
||
| def _prepare_fit_context_td(self, |
| if n_rows <= 0: | ||
| raise ValueError("n_rows must be positive") | ||
|
|
||
| if max_ohe_width < 0: | ||
| raise ValueError("max_ohe_width must be non-negative") | ||
|
|
||
| if max_ohe_width_ratio < 0: | ||
| raise ValueError("max_ohe_width_ratio must be non-negative") |
There was a problem hiding this comment.
validation through marshmallow
There was a problem hiding this comment.
use tuples within methods where applicable for (non-dirty) methods
| def test_api_params_builds_tensor_data_config_field_on_init(): | ||
| params = ApiParams({}, problem='classification', timeout=1) | ||
|
|
||
| assert params.tensor_data_config == { | ||
| 'backend_name': 'cpu', | ||
| 'use_cache': True, | ||
| } | ||
| assert params['tensor_data_config'] == params.tensor_data_config |
There was a problem hiding this comment.
FAILED tests/api/api_utils/test_api_params.py::test_api_params_builds_tensor_data_config_field_on_init - KeyError: 'tensor_data_config'
…d ensemble config, and sampling configuration
…nd composer requirements
…module and updating imports
…data, and pipelines
…data tests for improved error handling
| self._log = default_log('InputAnalyzer') | ||
|
|
||
| def give_recommendations(self, input_data: Union[InputData, MultiModalData], input_params=None) \ | ||
| def give_recommendations(self, input_data: Union[InputData, MultiModalData, TensorData], input_params=None) \ |
There was a problem hiding this comment.
should we remove the InputData support in this PR, or have you decided to keep it and address it in the next one?
There was a problem hiding this comment.
I'm going to keep InputData before test comparing with TensorData. Furthermore, the vast part of pipeline use ArrayType yet.
| def fit_tensor_data(self, | ||
| features: FeaturesType, | ||
| target: TargetType = 'target', | ||
| predefined_model: Union[str, Pipeline] = None) -> Pipeline: | ||
|
|
||
| MemoryAnalytics.start() | ||
|
|
||
| initial_timeout = self.params.timeout |
There was a problem hiding this comment.
would it be better for fit_tensor_data method to work with TensorData not the features-target pair?
There was a problem hiding this comment.
So you’re suggesting that the creation TensorData and preprocessing (obligatory) stages should be left to the user?
| ... | ||
| """ | ||
| DEFAULT_NAME = 'cpu' | ||
| GPU_ALIASES = frozenset({'gpu', 'cuda'}) |
There was a problem hiding this comment.
gpus on m-series macs can also be used in mps, so it might be much better not to enforce cuda from gpu user-specs.
instead, it would be better to determine what GPU capabilities are available and assess which ones are compatible with FEDOT
| # TODO romankuklo: make features as obligatory field | ||
| features: Optional[torch.Tensor] = None |
There was a problem hiding this comment.
make features obligatory in this PR
| from fedot.core.operations.operation_parameters import OperationParameters | ||
|
|
||
|
|
||
| class TorchLinearClassifier: |
There was a problem hiding this comment.
should be inherited or be a composite product of ModelImplementation, Operation, or Model - see which one suits better
| def fit_tensordata(self, | ||
| params: Optional[Union[OperationParameters, dict]], | ||
| data: TensorData, | ||
| predictions_cache: Optional[PredictionsCache] = None, | ||
| fold_id: Optional[int] = None, | ||
| descriptive_id: Optional[str] = None): |
There was a problem hiding this comment.
here, it's TensorData (see the comment in main.py regarding a similar consideration)
There was a problem hiding this comment.
But after automatic TensorData creating
| def fit_tensordata(self, | ||
| tensor_data: TensorData, | ||
| predictions_cache: Optional[PredictionsCache] = None, | ||
| fold_id: Optional[int] = None) -> TensorData: | ||
| """Runs training process in the node on TensorData. | ||
|
|
||
| Slice 1 supports only primary nodes without parents (e.g. single predefined model). | ||
| """ | ||
| self.log.debug( | ||
| f'Trying to fit pipeline node with operation: {self.operation} on TensorData') | ||
| tensor_data = self._get_tensor_data( | ||
| tensor_data=tensor_data, parent_operation='fit') | ||
|
|
||
| if self.fitted_operation is None: | ||
| with Timer() as t: | ||
| self.fitted_operation, operation_predict = self.operation.fit_tensordata( | ||
| params=self._parameters, | ||
| data=tensor_data, | ||
| predictions_cache=predictions_cache, | ||
| fold_id=fold_id, | ||
| descriptive_id=self.descriptive_id, | ||
| ) | ||
| self.fit_time_in_seconds = round(t.seconds_from_start, 3) | ||
| else: | ||
| operation_predict = self.operation.predict_tensordata( | ||
| fitted_operation=self.fitted_operation, | ||
| data=tensor_data, | ||
| params=self._parameters, | ||
| predictions_cache=predictions_cache, | ||
| fold_id=fold_id, | ||
| descriptive_id=self.descriptive_id, | ||
| is_fit_stage=True, | ||
| ) | ||
|
|
||
| if should_update_node_parameters(self.operation.operation_type, self.operation.metadata.tags): | ||
| self.update_params() | ||
| return operation_predict | ||
|
|
||
| def predict_tensordata(self, | ||
| tensor_data: TensorData, | ||
| output_mode: str = 'default', | ||
| predictions_cache: Optional[PredictionsCache] = None, | ||
| fold_id: Optional[int] = None) -> TensorData: | ||
| """Runs prediction process in the node on TensorData.""" | ||
| self.log.debug( | ||
| f'Trying to predict pipeline node with operation: {self.operation} on TensorData') | ||
| tensor_data = self._get_tensor_data( | ||
| tensor_data=tensor_data, parent_operation='predict') | ||
| with Timer() as t: | ||
| operation_predict = self.operation.predict_tensordata( | ||
| fitted_operation=self.fitted_operation, | ||
| data=tensor_data, | ||
| params=self._parameters, | ||
| output_mode=output_mode, | ||
| predictions_cache=predictions_cache, | ||
| fold_id=fold_id, | ||
| descriptive_id=self.descriptive_id, | ||
| is_fit_stage=False, | ||
| ) | ||
| self.inference_time_in_seconds = round(t.seconds_from_start, 3) | ||
| return operation_predict |
There was a problem hiding this comment.
reuse some methods, clean them up, and replace the * methods with your *_for_tensordata versions
| @@ -1,4 +1,8 @@ | |||
| { | |||
| "torch_linear": { | |||
| "learning_rate": 0.05, | |||
There was a problem hiding this comment.
thoughts on LR scheduler?
| from fedot.core.operations.evaluation.operation_implementations.models.torch import TorchLinearClassifier | ||
|
|
||
|
|
||
| class TorchTabularClassificationStrategy(EvaluationStrategy): |
There was a problem hiding this comment.
why only tabular? can the Torch Linear model NOT be data-agnostic?
| method=step.method.value if hasattr(step.method, "value") else str(step.method), | ||
| features_idx=step.features_idx, | ||
| ) | ||
| else: |
There was a problem hiding this comment.
shouldn't this be logged?
…refactor/td_integration
| logger.info( | ||
| f"Using CUDA stack with cudf/cupy and torch.device('{normalized_name}'). ", | ||
| ) | ||
| return |
There was a problem hiding this comment.
nit: not urgent, but from a style point of view, rather difficult to read code. frequent checks, ornate code... try to simplify the logic of checks using strategy or factory patterns
There was a problem hiding this comment.
maybe dict with strategies
There was a problem hiding this comment.
Done, added dict with strategies: c44ce8c
| ) | ||
|
|
||
| normalized = value.strip().lower() | ||
| if normalized in ('cpu', 'gpu', 'cuda', 'mps'): |
There was a problem hiding this comment.
no magic set here - ('cpu', 'gpu', 'cuda', 'mps') should be initialized once as a backend layer constant SUPPORTED_BACKENDS or smth simillar
| from fedot.validation.context import ValidationContext | ||
|
|
||
| BACKEND_CUDA_DEVICE_PATTERN = re.compile(r'^cuda:\d+$') | ||
| BACKEND_SUPPORTED_NAMES_HINT = 'cpu, gpu, cuda, mps, cuda:<device_index>' |
There was a problem hiding this comment.
this should be built in-place (in 24th line) from SUPPORTED_BACKENDS constant set (see comment at 28th line)
| @@ -391,7 +400,8 @@ def _parse_tasks(raw: Optional[str]) -> Optional[list[str]]: | |||
|
|
|||
|
|
|||
| def main() -> None: | |||
| parser = argparse.ArgumentParser(description="Run OpenML benchmark with FEDOT TabICL and update foundational column.") | |||
| parser = argparse.ArgumentParser( | |||
| description="Run OpenML benchmark with FEDOT TabICL and update foundational column.") | |||
| parser.add_argument("--classification-suite", type=int, default=DEFAULT_CLASSIFICATION_SUITE) | |||
There was a problem hiding this comment.
please configure your local linter to 120-symbols of code in one line, no need to format legacy code in that way
|
|
||
|
|
||
| def default_repository_name_for_data(data) -> str: | ||
| # TODO @romankuklo: delete multi_ts? |
There was a problem hiding this comment.
idk, could we support multi_ts through TensorData?
multivariate ts and multi_ts are not the same thing - seek guidance from @v1docq
| return valid_builders or [self.assumptions_generator.fallback_builder(self.ops_filter)] | ||
|
|
||
|
|
||
| # TODO: refactor for TensorData if needed |
There was a problem hiding this comment.
still a question to research
| operations_cache: Optional[OperationsCache] = None, | ||
| preprocessing_cache: Optional[PreprocessingCache] = None, | ||
| eval_n_jobs: int = -1) -> Pipeline: | ||
| def fit_assumption_and_check_correctness_with_tensordata( |
There was a problem hiding this comment.
why *_with_tensordata again? no point at providing 2 different signatures here and everywhere else, just refactor the old methods
| *initial_nodes, use_input_preprocessing=use_input_preprocessing) | ||
|
|
||
| @classmethod | ||
| def builder_for_tensordata(cls, |
There was a problem hiding this comment.
here also, just change logic inside actual builder_for_data, do not introduce a separate builder_for_tensordata - this becomes more difficult to handle in other PRs
There was a problem hiding this comment.
maybe just comment the old methods out for now, and introduce the new logic, but not as a separate *tensordata methods
| def test_regression_metric_tensordata_implementation(metric_class, expected_value): | ||
| target = np.array([1.0, 2.0, 4.0, 8.0]) | ||
| prediction = np.array([1.1, 1.8, 4.5, 7.5]) | ||
| features = np.array([0.8, 1.2, 2.1, 4.1]) | ||
| task = Task(TaskTypesEnum.regression) | ||
|
|
||
| tensor_reference, tensor_prediction = _build_tensor_metric_data( | ||
| target, prediction, task, features=features) | ||
|
|
||
| tensor_value = metric_class.metric(tensor_reference, tensor_prediction) | ||
|
|
||
| assert np.isclose(tensor_value, expected_value, rtol=1e-6, atol=1e-6) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| 'metric_class, prediction, expected_value', | ||
| [ | ||
| (Accuracy, np.array([0, 1, 1, 0, 1]), -0.6), | ||
| (F1, np.array([0, 1, 1, 0, 1]), -0.5), | ||
| (Precision, np.array([0, 1, 1, 0, 1]), -0.5), | ||
| (ROCAUC, np.array([0.1, 0.8, 0.7, 0.4, 0.9]), -0.6666666666666666), | ||
| (Logloss, np.array([0.1, 0.8, 0.7, 0.4, 0.9]), 0.6186249239125281), | ||
| ], |
There was a problem hiding this comment.
no point in writing tests inside test dir, since it tests legacy code, need rewriting in tests instead and deleting the old ones in test one by one
There was a problem hiding this comment.
Will be done later
| fold_id=fold_id, | ||
| descriptive_id=self.descriptive_id) | ||
|
|
||
| # Update parameters after operation fitting (they can be corrected) |
There was a problem hiding this comment.
do not delete previous comments and google-style docstrings for methods
Summary
Context
сloses #1437