Replacing Monkey-Patching - #1421
Conversation
|
Code in this pull request still contains PEP8 errors, please write the Comment last updated at Fri, 29 May 2026 08:52:30 |
|
/fix-pep8 |
Lopa10ko
left a comment
There was a problem hiding this comment.
Main concern: The new context is not yet driving behavior – most of FEDOT/Industrial still relies on global singletons and direct imports, so the extension system is more of a configuration holder than a true execution context.
We want to create “no global side effects; context is passed explicitly” and (optional) protocols per extension point; those parts are currently incomplete.
No unit tests for the context yet :(
| import golem.core.tuning.optuna_tuner as OptunaImpl | ||
|
|
||
|
|
||
| class ExecutionContext: |
There was a problem hiding this comment.
ExecutionContext is created and mutated (via IndustrialExtension.apply), but it is not passed to the places that actually perform evaluation, splitting, tuning, etc.
all existing usages of IndustrialModels still look like:
self.repo = IndustrialModels().setup_repository()
with IndustrialModels(): ...these mutate OperationTypesRepository.__repository_dict__ and then continue to use the global/static APIs instead of going through the context attributes.
|
|
||
| def __getstate__(self): | ||
| return { | ||
| "industrial": True, |
There was a problem hiding this comment.
where does the context and IndustrialModels actually used in FEDOT API (not in FedotIndustrial API)?
context != industrial context, context might be non-industrial also.
|
|
||
| class ExecutionContext: | ||
| def __init__(self) -> None: | ||
| self.operation_registry = OperationTypesRepository() |
There was a problem hiding this comment.
(nit) here design is very concrete and tightly coupled
maybe you can try to introduce protocols or small interfaces for major extension points, for example:
OperationRegistryProtocol
MergerProtocol
SplitterProtocol
TunerFactoryProtocol / TunerProtocol
(nit) where possible, avoid eager instantiation of heavy components in __init__, instead use lazy properties or factories to avoid performance penalties and to make constructing a context cheaper
|
|
||
| def __getstate__(self): | ||
| return { | ||
| "industrial": True, |
There was a problem hiding this comment.
the industrial flag is stored but never used or set in __setstate__.
IMPORTANT: also any backend‑specific overrides made by IndustrialExtension.apply are lost upon unpickling, because __init__ recreates a default (non‑industrial) context.
| context.topo_features_fit = fit_topo_extractor_industrial | ||
| context.topo_features_transform = transform_topo_extractor_industrial | ||
| context.ts_smoothing_transform = transform_smoothing_industrial | ||
| context.optuna_optuna_tuner = DaskOptunaTuner |
There was a problem hiding this comment.
remove this duplicated line
see the 33rd line here:
context.optuna_optuna_tuner = DaskOptunaTuner if "dask" in self.backend else OptunaTuner| context.optuna_optuna_tuner = DaskOptunaTuner |
| get_industrial_search_space) | ||
| setattr(ApiComposer, "_get_default_mutations", | ||
| _get_default_industrial_mutations) | ||
| def __enter__(self) -> ExecutionContext: |
There was a problem hiding this comment.
this now returns an ExecutionContext, but previously the context manager was used only for side effects (switching repos and monkey patching).
many call sites still use with IndustrialModels(): and ignore the returned value
| self._replace_operation(to_industrial=False, backend=backend) | ||
| common_rules.append(has_no_resample) | ||
| self.context = ExecutionContext() | ||
| self.context.common_rules.append(has_no_resample) |
There was a problem hiding this comment.
this creates a new ExecutionContext every time and sets self.context.common_rules.append(has_no_resample)
this might not match expectations where callers think they are "restoring" a default state
| 'default_tags': []}}) | ||
| OperationTypesRepository.assign_repo('model', self.base_model_path) | ||
| self.setup_default_repository() | ||
| self.context = None |
There was a problem hiding this comment.
shouldn't it restore the context to FEDOT models along with context?
in setup_default_repository you create a fresh context, but after that call there is self.context = None
|
|
||
| class ExecutionContext: | ||
| def __init__(self) -> None: | ||
| self.operation_registry = OperationTypesRepository() |
| eval_n_jobs: int = 1, | ||
| do_unfit: bool = True): | ||
| do_unfit: bool = True, | ||
| context: Optional[ExecutionContext] = None): |
There was a problem hiding this comment.
why pass context here?
it isn't used in this constructor or the pipeline evaluation methods
|
|
||
| class LaggedImplementation(DataOperationImplementation): | ||
| def __init__(self, params: Optional[OperationParameters]): | ||
| def __init__(self, params: Optional[OperationParameters], context: Optional[ExecutionContext] = None): |
There was a problem hiding this comment.
why pass context into this operation alone?
the context requires refactoring into smaller, more manageable parts (for example, the lagged context or the data operation context would be more appropriate for specific operations that need certain overrides - not all overrides are necessary in these cases)
| split_func_dict = {DataTypesEnum.multi_ts: context.data_split__split_time_series, | ||
| DataTypesEnum.ts: context.data_split__split_time_series, | ||
| DataTypesEnum.table: context.data_split__split_any, | ||
| DataTypesEnum.image: context.data_split__split_any, | ||
| DataTypesEnum.text: context.data_split__split_any} |
There was a problem hiding this comment.
data split context - should be separate object within context or within context extension
| @@ -35,7 +38,8 @@ def __init__(self, outputs: List['OutputData'], data_type: DataTypesEnum = None) | |||
| raise ValueError('There are no common indices for outputs') | |||
|
|
|||
| # Find first output with the main target & resulting task | |||
| self.main_output = DataMerger.find_main_output(outputs) | |||
| # self.main_output = DataMerger.find_main_output(outputs) | |||
| self.main_output = self.context.merger_find_main_output(outputs) | |||
|
|
|||
| @staticmethod | |||
| def get(outputs: List['OutputData']) -> 'DataMerger': | |||
| @@ -71,7 +75,8 @@ def merge(self) -> 'InputData': | |||
|
|
|||
| common_predicts = self.find_common_predicts() | |||
| mergeable_predicts = self.preprocess_predicts(common_predicts) | |||
| merged_features = self.merge_predicts(mergeable_predicts) | |||
| # merged_features = self.merge_predicts(mergeable_predicts) | |||
| merged_features = self.context.merger_merge_predicts(mergeable_predicts) | |||
There was a problem hiding this comment.
data merger context within context or extension
|
|
||
|
|
||
| def industrial_context_factory(backend: str = "default"): | ||
| return IndustrialContext(backend=backend) |
There was a problem hiding this comment.
в импорте этого нет, появляется ошибка IndustrialContext is not defined.
Используется ли этот метод вообще? Почему этот и другие методы здесь являются фабриками (если хочется использовать паттерн фабрики - пожалуйста, но здесь он реализован не как фабрика) и зачем они здесь, если они просто создают инстансы классов (причем с параметрами по умолчанию в конструкторах)?
There was a problem hiding this comment.
манифест регистрирует эти методы как протоколы, протоколы отдают конкретные имлементации (напр. IndustrialSplitter), эти классы переопределяют нужные методы (внутри вызывают логику из индастриала) вместо их патчинга, так?
зачем эти ненужные добавочные слои в виде классов вида IndustrialSplitter, как и где это подгружается в контекст? есть контекст, в нем должно быть назначение этих методов (дефолтное FEDOT назначение работает), а как работает для Industrial?
см. дальше замечания по контексту в context.py
There was a problem hiding this comment.
опиши, пожалуйста, полный цикл жизни extension
| topo_features_factory, ts_smoothing_factory, api_composer_tune_factory) | ||
|
|
||
|
|
||
| FEDOT_INDUSTRIAL_MANIFEST = ExtensionManifest( |
There was a problem hiding this comment.
верно понимаю, что это общая входная точка для патченых методов?
как планируется использовать это?
| @@ -0,0 +1,109 @@ | |||
| import Protocol | |||
There was a problem hiding this comment.
Import "Protocol" could not be resolved....
|
|
||
| def _apply_protocols(self): | ||
| # Splitters | ||
| splitters = resolve_protocol_instance("splitters", backend=self.backend) |
| if context_name == "core": | ||
| return ExecutionContext(backend=backend) | ||
|
|
||
| from fedot.extensions.registry import get_registered_extension, get_registered_extensions |
There was a problem hiding this comment.
а Industrial extension где-то регистрируется?
b4791f7 to
2a4018b
Compare
Summary
ExecutionContextandIndustrialExtensionto switch base and industrial context__getstate__and__setstate__withinExecutionContextinitializer_idustrial_models.pyto apply changesNext Steps
Context
closes #1415