diff --git a/docs/docs/in_depth/feature-chain-parser.md b/docs/docs/in_depth/feature-chain-parser.md index d48ce1772..cb430926b 100644 --- a/docs/docs/in_depth/feature-chain-parser.md +++ b/docs/docs/in_depth/feature-chain-parser.md @@ -317,7 +317,7 @@ def match_feature_group_criteria(cls, feature_name, options, data_access_collect return cls.match_parser_criteria(feature_name, options) ``` -`match_parser_criteria` calls the parser with the class's `PROPERTY_MAPPING` and patterns and turns a rejected option value into a non-match. Calling `FeatureChainParser` directly from a match hook lets that rejection escape as an exception and abort feature identification for every candidate. +`match_parser_criteria` calls the parser with the class's `PROPERTY_MAPPING` and patterns and turns a rejected option value into a non-match. Calling `FeatureChainParser` directly from a match hook lets that rejection escape as an exception; the engine contains it as a `match hook` near-miss for that candidate, but the rejection reason is the more useful one. Containment covers plugin raises only: a framework-owned raise, such as a forwarded option value contradicting the feature name, still aborts the whole resolution. ### 3. Modernize input_features Method diff --git a/docs/docs/in_depth/feature-group-matching.md b/docs/docs/in_depth/feature-group-matching.md index 5afeb4628..cd8d1a21b 100644 --- a/docs/docs/in_depth/feature-group-matching.md +++ b/docs/docs/in_depth/feature-group-matching.md @@ -23,7 +23,9 @@ def match_feature_group_criteria(cls, feature_name, options, data_access_collect return cls.match_parser_criteria(feature_name, options) ``` -Do not call `FeatureChainParser.match_configuration_feature_chain_parser` directly from a match hook: it raises on an option value the `PROPERTY_MAPPING` rejects, and an exception out of the hook aborts feature identification for every candidate, not just yours. `match_parser_criteria` turns that rejection into a non-match, and the reason still reaches the user in the "No feature groups found" error. +Do not call `FeatureChainParser.match_configuration_feature_chain_parser` directly from a match hook: it raises on an option value the `PROPERTY_MAPPING` rejects. An exception out of a match hook is contained as a `match hook` near-miss for that candidate instead of taking the whole resolution down, but a contained crash is a worse reason than a rejection. `match_parser_criteria` turns that rejection into a non-match, and the reason still reaches the user in the "No feature groups found" error. + +Containment covers plugin raises only: a framework-owned raise (a two-readers conflict, a forwarded value contradicting the feature name, a rejected effective-options build) still aborts the whole resolution, because it reports a misconfiguration you have to fix. The options view depends on the caller: feature resolution passes declared (pre-default) options, while filter matching runs after intake and passes the resolved feature's effective (post-default) options merged onto the filter feature's own. Matching logic that reads option values can see different values on the two paths. See [Applying declared defaults](property-mapping.md#applying-declared-defaults). diff --git a/mloda/core/abstract_plugins/components/feature_chainer/feature_chain_author_guards.py b/mloda/core/abstract_plugins/components/feature_chainer/feature_chain_author_guards.py index 6fff743c4..3fb2210d1 100644 --- a/mloda/core/abstract_plugins/components/feature_chainer/feature_chain_author_guards.py +++ b/mloda/core/abstract_plugins/components/feature_chainer/feature_chain_author_guards.py @@ -5,7 +5,7 @@ Depends on these parser-private names, so renaming one of them is a cross-module break: ``FeatureChainParser._can_skip_required_check``, ``._check_name_path_required_presence``, ``._merge_bindings``, -``._name_identifies_group``, ``._name_path_missing_required_keys``, and module-private ``_contained_raise_log_level``. +``._name_identifies_group``, and ``._name_path_missing_required_keys``. """ from __future__ import annotations @@ -21,13 +21,12 @@ from mloda.core.abstract_plugins.components.feature_chainer.feature_chain_parser import ( CHAIN_SEPARATOR, FeatureChainParser, - _contained_raise_log_level, option_key_is_present, record_match_rejection, ) from mloda.core.abstract_plugins.components.feature_chainer.parsed_feature_name import ParsedFeatureName from mloda.core.abstract_plugins.components.feature_chainer.property_spec import PropertySpec -from mloda.core.abstract_plugins.components.utils import safe_field +from mloda.core.abstract_plugins.components.utils import contained_raise_log_level, escalate_match_abort, safe_field logger = logging.getLogger(__name__) @@ -238,9 +237,14 @@ def check_required_when( # build_effective_options runs no user callback, so a raise from it is a framework defect (or a user # configuration error carrying actionable guidance) and must surface, not read as a non-match (#763). - effective_options = FeatureChainParser.build_effective_options( - feature_name, prefix_patterns, property_mapping, options - ) + # Marked so it survives the match seam, which otherwise contains every raise (#845). + try: + effective_options = FeatureChainParser.build_effective_options( + feature_name, prefix_patterns, property_mapping, options + ) + except Exception as exc: + escalate_match_abort(exc) + raise for key, spec in property_mapping.items(): if not isinstance(spec, PropertySpec): continue @@ -253,7 +257,7 @@ def check_required_when( is_required = bool(predicate(effective_options)) except Exception as exc: logger.log( - _contained_raise_log_level(exc), + contained_raise_log_level(exc), "required_when predicate %s for '%s' raised %s; treating feature group %s as a non-match.", getattr(predicate, "__name__", repr(predicate)), key, diff --git a/mloda/core/abstract_plugins/components/feature_chainer/feature_chain_parser.py b/mloda/core/abstract_plugins/components/feature_chainer/feature_chain_parser.py index e50afe26d..1e49ad291 100644 --- a/mloda/core/abstract_plugins/components/feature_chainer/feature_chain_parser.py +++ b/mloda/core/abstract_plugins/components/feature_chainer/feature_chain_parser.py @@ -15,7 +15,7 @@ from mloda.core.abstract_plugins.components.default_options_key import DefaultOptionKeys from mloda.core.abstract_plugins.components.feature_chainer.parsed_feature_name import ParsedFeatureName from mloda.core.abstract_plugins.components.feature_chainer.property_spec import PropertySpec, is_no_default -from mloda.core.abstract_plugins.components.utils import safe_field +from mloda.core.abstract_plugins.components.utils import contained_raise_log_level, safe_field logger = logging.getLogger(__name__) @@ -31,14 +31,6 @@ "mloda_match_rejection_reasons", default=None ) -# Exception classes a user callable raises when it merely cannot judge a value. -_EXPECTED_JUDGMENT_ERRORS: tuple[type[Exception], ...] = (TypeError, ValueError, AttributeError) - - -def _contained_raise_log_level(exc: BaseException) -> int: - """DEBUG for expected judgment failures, WARNING for classes that suggest a broken callable.""" - return logging.DEBUG if isinstance(exc, _EXPECTED_JUDGMENT_ERRORS) else logging.WARNING - def record_match_rejection(owner_name: str, reason: str) -> None: """Record a match rejection; the first reason per owner wins, and outside an active evaluation it is a no-op.""" @@ -190,7 +182,7 @@ def _validate_property_value( try: verdict = element_validator(found_property_val) except Exception as exc: - level = _contained_raise_log_level(exc) + level = contained_raise_log_level(exc) if level == logging.DEBUG: logger.debug( "element_validator for '%s' raised %s for value %r; treating value as rejected.", diff --git a/mloda/core/abstract_plugins/components/feature_chainer/feature_chain_parser_mixin.py b/mloda/core/abstract_plugins/components/feature_chainer/feature_chain_parser_mixin.py index c7f28749a..c20f4a070 100644 --- a/mloda/core/abstract_plugins/components/feature_chainer/feature_chain_parser_mixin.py +++ b/mloda/core/abstract_plugins/components/feature_chainer/feature_chain_parser_mixin.py @@ -73,13 +73,12 @@ CHAIN_SEPARATOR, INPUT_SEPARATOR, PropertyValueRejection, - _contained_raise_log_level, option_key_is_present, record_match_rejection, ) from mloda.core.abstract_plugins.components.feature_chainer.property_spec import PropertySpec from mloda.core.abstract_plugins.components.default_options_key import DefaultOptionKeys -from mloda.core.abstract_plugins.components.utils import safe_field +from mloda.core.abstract_plugins.components.utils import contained_raise_log_level, escalate_match_abort, safe_field logger = logging.getLogger(__name__) @@ -301,8 +300,8 @@ def match_feature_group_criteria( def match_parser_criteria(cls, feature_name: str | FeatureName, options: Options) -> bool: """Call the parser, turning a rejected option value or a malformed name into a non-match, never an exception. - The only safe way to reach the parser from an overridden ``match_feature_group_criteria``: an exception out - of a match hook aborts the identification of the feature for every candidate, not just this one. + The preferred way to reach the parser from an overridden ``match_feature_group_criteria``: a raise from a + match hook is contained as a ``match hook`` near-miss, but a rejection carries a better reason than a crash. """ try: return FeatureChainParser.match_configuration_feature_chain_parser( @@ -430,7 +429,8 @@ def _validate_forwarded_name_mismatch( if os.environ.get("MLODA_ALLOW_FORWARDED_NAME_MISMATCH", "").lower() in ("1", "true"): logger.warning(message) continue - raise ValueError(message) + # Marked: user misconfiguration; containing it would let a rival group win with the value ignored (#845). + raise escalate_match_abort(ValueError(message)) @classmethod def _first_rejecting_guard( @@ -456,7 +456,7 @@ def _first_rejecting_guard( try: rejected = not guard(value) except Exception as exc: - level = _contained_raise_log_level(exc) + level = contained_raise_log_level(exc) if level == logging.DEBUG: logger.debug("match_guard for '%s' raised %s for value %r", key, exc, value) else: diff --git a/mloda/core/abstract_plugins/components/input_data/api/api_input_data.py b/mloda/core/abstract_plugins/components/input_data/api/api_input_data.py index 06baf743a..394df3d36 100644 --- a/mloda/core/abstract_plugins/components/input_data/api/api_input_data.py +++ b/mloda/core/abstract_plugins/components/input_data/api/api_input_data.py @@ -24,6 +24,7 @@ def matches( _data_access_name = self.data_access_name() if not _data_access_name: + # Contained: a blank data_access_name is this reader's own defect (#845). raise ValueError(f"Data access name was not set for ApiInputData class {self.__class__.__name__}.") api_input_data_column_names = options[_data_access_name] diff --git a/mloda/core/abstract_plugins/components/input_data/base_input_data.py b/mloda/core/abstract_plugins/components/input_data/base_input_data.py index a2dba47b3..91c81e976 100644 --- a/mloda/core/abstract_plugins/components/input_data/base_input_data.py +++ b/mloda/core/abstract_plugins/components/input_data/base_input_data.py @@ -7,7 +7,7 @@ from mloda.core.abstract_plugins.components.options import Options -from mloda.core.abstract_plugins.components.utils import get_all_subclasses +from mloda.core.abstract_plugins.components.utils import escalate_match_abort, get_all_subclasses class BaseInputData(ABC): @@ -155,6 +155,7 @@ def feature_scope_data_access(cls, options: Options, feature_name: str) -> bool: @classmethod def deal_with_base_input_data_name_as_cls_or_str(cls, key: Any) -> str: + # Contained: this runs per candidate over every option key, so one odd key must not abort the run (#845). if hasattr(key, "get_class_name"): if not issubclass(key, BaseInputData): raise ValueError(f"Key {key} is not a subclass of BaseInputData.") @@ -220,8 +221,11 @@ def add_base_input_data_to_options( return already_cls_to_be_added, _ = existing_data - raise ValueError( - f"BaseInputData already set with different values. {cls_to_be_added} != {already_cls_to_be_added}" + # Marked: two conflicting readers for one feature is a user misconfiguration. + raise escalate_match_abort( + ValueError( + f"BaseInputData already set with different values. {cls_to_be_added} != {already_cls_to_be_added}" + ) ) options.add_to_group("BaseInputData", (cls_to_be_added, matched_data_access)) @@ -310,6 +314,7 @@ def is_final_reader(cls) -> bool: required = cls._final_reader_requires() for name in required: if not hasattr(anchor, name): + # Contained: this runs over every registered reader, so one broken plugin must not abort every run. raise ValueError( f"Required final-reader hook '{name}' is not defined on anchor class {anchor.__name__}." ) @@ -348,7 +353,10 @@ def _resolve_pinned_file(cls, data_access: Any, feature_names: list[str]) -> Opt return None for name in feature_names: if name not in column_map: - raise ValueError(f"Mixed batch: some features pinned, others not: {feature_names}") + # Marked: containing a half-pinned batch would hand the feature to a reader that ignores the pins. + raise escalate_match_abort( + ValueError(f"Mixed batch: some features pinned, others not: {feature_names}") + ) pinned_paths: set[str] = {files_registry[h] for h in pinned_handles} if len(pinned_paths) == 1: pinned_path: str = next(iter(pinned_paths)) @@ -364,7 +372,8 @@ def _resolve_pinned_file(cls, data_access: Any, feature_names: list[str]) -> Opt ] if len(valid_candidates) == 1: return valid_candidates[0] - raise ValueError(f"Features in batch are pinned to different files: {pinned_paths}") + # Marked: same as the mixed batch above. + raise escalate_match_abort(ValueError(f"Features in batch are pinned to different files: {pinned_paths}")) def _collect_filtered_subclasses(cls: Any, parent_class: Any) -> list[type[BaseInputData]]: diff --git a/mloda/core/abstract_plugins/components/match_data/match_data.py b/mloda/core/abstract_plugins/components/match_data/match_data.py index 805e16e76..a1900468c 100644 --- a/mloda/core/abstract_plugins/components/match_data/match_data.py +++ b/mloda/core/abstract_plugins/components/match_data/match_data.py @@ -2,6 +2,7 @@ from mloda.core.abstract_plugins.components.data_access_collection import DataAccessCollection from mloda.core.abstract_plugins.components.options import Options +from mloda.core.abstract_plugins.components.utils import escalate_match_abort import logging @@ -80,6 +81,7 @@ def match_data_access( """ We check for data access collection if any child classes match the data access. """ + # Contained: an unimplemented hook is this candidate's own defect (#845). raise NotImplementedError() @classmethod @@ -95,7 +97,10 @@ def add_base_input_data_to_options(cls, matched_data_access: Any, options: Optio if existing_data == matched_data_access: return - raise ValueError(f"{cls_name} already set with different values. {existing_data} != {matched_data_access}") + # Marked: two conflicting readers for one feature is a user misconfiguration. + raise escalate_match_abort( + ValueError(f"{cls_name} already set with different values. {existing_data} != {matched_data_access}") + ) options.add_to_group(cls_name, matched_data_access) @classmethod diff --git a/mloda/core/abstract_plugins/components/utils.py b/mloda/core/abstract_plugins/components/utils.py index eaab1068c..dfa10b778 100644 --- a/mloda/core/abstract_plugins/components/utils.py +++ b/mloda/core/abstract_plugins/components/utils.py @@ -8,6 +8,36 @@ T = TypeVar("T") +E = TypeVar("E", bound=BaseException) + +# Provenance marker for a framework-owned raise. Not an exception type: the object must stay exactly as raised. +MATCH_ABORT_FLAG = "_mloda_match_abort" + +# Exception classes a user callable raises when it merely cannot judge a value. +_EXPECTED_JUDGMENT_ERRORS: tuple[type[Exception], ...] = (TypeError, ValueError, AttributeError) + + +def contained_raise_log_level(exc: BaseException) -> int: + """DEBUG for expected judgment failures, WARNING for classes that suggest a broken callable.""" + return logging.DEBUG if isinstance(exc, _EXPECTED_JUDGMENT_ERRORS) else logging.WARNING + + +def escalate_match_abort(exc: E) -> E: + """Mark a framework-owned raise so the match seam re-raises it instead of containing it as a non-match.""" + # __dict__, not setattr: setattr raises on a frozen-dataclass exception, and failing to mark must not + # replace the exception being marked. + try: + exc.__dict__[MATCH_ABORT_FLAG] = True + except Exception: # noqa: BLE001 (marking is never worth losing the original raise) + logger.debug("Could not mark %s as a match abort.", type(exc).__name__) + return exc + + +def is_match_abort(exc: BaseException) -> bool: + """Is this raise marked as framework-owned, so the match seam must not contain it.""" + # __dict__, not getattr: a custom __getattr__ could raise inside the seam's except block or fake the marker. + return exc.__dict__.get(MATCH_ABORT_FLAG, False) is True + def safe_field( read: Callable[[], T], diff --git a/mloda/core/filter/global_filter.py b/mloda/core/filter/global_filter.py index 65c4d2355..df1b4b2af 100644 --- a/mloda/core/filter/global_filter.py +++ b/mloda/core/filter/global_filter.py @@ -120,6 +120,7 @@ def criteria( filter: SingleFilter, data_access_collection: Optional[DataAccessCollection] = None, ) -> bool: + # Uncontained: the #845 match seam does not reach here, but the group is already identified. return feature_group.match_feature_group_criteria( filter.filter_feature.name, filter.filter_feature.options, data_access_collection ) diff --git a/mloda/core/prepare/identify_feature_group.py b/mloda/core/prepare/identify_feature_group.py index 721c02208..6feb401b9 100644 --- a/mloda/core/prepare/identify_feature_group.py +++ b/mloda/core/prepare/identify_feature_group.py @@ -1,3 +1,4 @@ +import functools import inspect from collections.abc import Sequence from copy import deepcopy @@ -29,7 +30,12 @@ PropertyValueRejection, record_match_rejection, ) -from mloda.core.abstract_plugins.components.utils import safe_field, as_str +from mloda.core.abstract_plugins.components.utils import ( + as_str, + contained_raise_log_level, + is_match_abort, + safe_field, +) from mloda.core.abstract_plugins.compute_framework import ComputeFramework from mloda.core.abstract_plugins.feature_group import FeatureGroup from mloda.core.abstract_plugins.components.feature import Feature @@ -114,6 +120,7 @@ class IdentifyFeatureGroupClass: _abstract_matched_feature_groups: set[type[FeatureGroup]] _candidate_frameworks: dict[type[FeatureGroup], CandidateFrameworks] _match_rejections: dict[type[FeatureGroup], str] + _matcher_errors: dict[type[FeatureGroup], str] _eliminations: dict[type[FeatureGroup], Elimination] _data_access_collection: Optional[DataAccessCollection] # Per-evaluation memos of the hooks more than one reader wants. evaluate() builds a fresh instance, so @@ -127,6 +134,8 @@ def __init__(self, data_access_collection: Optional[DataAccessCollection] = None self._candidate_frameworks = {} # Reasons the first match pass recorded, keyed by candidate class. self._match_rejections = {} + # Contained matcher raises as text, keyed by candidate class: never the exception object. + self._matcher_errors = {} self._eliminations = {} self._domain_outcomes = {} self._declared_frameworks = {} @@ -309,6 +318,11 @@ def _filter_loop( # this reads it back for a criteria-FAILING candidate only; a matched/winning/abstract candidate is # never probed. Recorded regardless of domain/scope or of the overall outcome (a sibling may win). if not self._filter_feature_group_by_criteria(feature_group, feature, data_access_collection): + # A contained matcher raise is always a near-miss: the raise says nothing about name ownership. + matcher_error = self._matcher_errors.get(feature_group) + if matcher_error is not None: + self._record_elimination(feature_group, "matcher_error", matcher_error) + continue reason = self._value_rejection_reason(feature_group) if reason is not None: self._record_elimination(feature_group, "value_rejection", reason) @@ -425,21 +439,42 @@ def _filter_feature_group_by_criteria( feature: Feature, data_access_collection: Optional[DataAccessCollection], ) -> bool: - """A rejected option value is a non-match, whoever calls the parser: a candidate that overrides the match - hook and calls FeatureChainParser directly must not take the whole filter loop down. Only the rejection is - caught, so a plain ValueError (the forwarded-name-mismatch guidance) still reaches the user. - - The per-candidate recording window is what keys reasons by candidate class, so same-named classes cannot - collide (review fix). The try/finally guarantees the reset even when an unexpected exception propagates; - in that case nothing is attributed. The filter loop reads the recorded reason back at this candidate's - non-match point, so the value_rejection near-miss is captured without ever re-probing a rejection hook. + """A raise out of the match hook is a non-match for that candidate only, not a run-wide abort (#845). + + Policy: a candidate's own defect stays contained here, while escalate_match_abort marks the raises that + report a misconfiguration the user must fix, and those cross the seam untouched. A contained raise is + kept as text, never as an exception object whose traceback would pin the plugin class. The per-candidate + rejection window, reset in the finally, keys reasons by candidate class. """ token = MATCH_REJECTION_REASONS.set({}) + # Shallow copies, taken per candidate so an earlier match's write survives a later candidate's raise. + group_before = dict(feature.options.group) + context_before = dict(feature.options.context) try: matched = feature_group.match_feature_group_criteria(feature.name, feature.options, data_access_collection) - except PropertyValueRejection as exc: - logger.debug("%s rejected an option value while matching '%s': %s", feature_group, feature.name, exc) - record_match_rejection(feature_group.get_class_name(), str(exc)) + except Exception as exc: # noqa: BLE001 (contained: one broken matcher must not poison other features) + if is_match_abort(exc): + raise + # Only the contained branch rolls back: a matcher that returns True keeps its write, which is how a + # matched reader is linked through mloda. + feature.options.group.clear() + feature.options.group.update(group_before) + feature.options.context.clear() + feature.options.context.update(context_before) + if isinstance(exc, PropertyValueRejection): + logger.debug("%s rejected an option value while matching '%s': %s", feature_group, feature.name, exc) + record_match_rejection(feature_group.get_class_name(), str(exc)) + else: + # partial, not a lambda: exc binds eagerly, so no closure keeps it and its traceback alive. + reason = f"raised {type(exc).__name__}: {safe_field(functools.partial(str, exc), type(exc).__name__)}" + logger.log( + contained_raise_log_level(exc), + "%s %s while matching '%s'; treating it as a non-match.", + feature_group.get_class_name(), + reason, + feature.name, + ) + self._matcher_errors[feature_group] = reason matched = False finally: recorded = MATCH_REJECTION_REASONS.get() or {} diff --git a/mloda/core/prepare/resolution_failure_renderer.py b/mloda/core/prepare/resolution_failure_renderer.py index 5f3e65f4f..492a7fc37 100644 --- a/mloda/core/prepare/resolution_failure_renderer.py +++ b/mloda/core/prepare/resolution_failure_renderer.py @@ -40,6 +40,7 @@ def _prefix_name(feature_group: type[FeatureGroup]) -> str: _STAGE_LABELS: dict[EliminationStage, str] = { "value_rejection": "option value", + "matcher_error": "match hook", "domain": "domain", "scope": "scope", "capability": "compute framework", diff --git a/mloda/core/prepare/resolution_types.py b/mloda/core/prepare/resolution_types.py index f0b5eaadf..34602e386 100644 --- a/mloda/core/prepare/resolution_types.py +++ b/mloda/core/prepare/resolution_types.py @@ -18,6 +18,7 @@ class CandidateFrameworks: EliminationStage = Literal[ "value_rejection", + "matcher_error", "domain", "scope", "capability", diff --git a/tests/conftest.py b/tests/conftest.py index c2e7efe2c..b0901a56a 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -3,10 +3,25 @@ from typing import Any import pytest +from mloda.core.abstract_plugins.components.utils import get_all_subclasses +from mloda.core.abstract_plugins.feature_group import FeatureGroup from mloda.core.abstract_plugins.plugin_registry.plugin_registry import PluginRegistry from mloda.core.prepare import accessible_plugins from mloda.core.runtime.flight.runner_flight_server import ParallelRunnerFlightServer +from tests.registry_isolation import reclaim_leaked_feature_groups + + +# Defined first on purpose: autouse fixtures tear down in reverse, so this runs after the registry reset. +@pytest.fixture(autouse=True) +def _no_feature_group_registry_pollution(request: pytest.FixtureRequest) -> Any: + """Fail a test that leaves one of its own FeatureGroup subclasses registered (#845).""" + before = get_all_subclasses(FeatureGroup) + yield + module_name = request.module.__name__ + leaked = reclaim_leaked_feature_groups(before, module_name) + assert not leaked, f"Leaked FeatureGroup subclasses from {module_name}: {[c.__name__ for c in leaked]}" + def _clear_warned_unregistered() -> None: """Clear the warn-mode once-per-process dedup set if the implementation provides it.""" diff --git a/tests/registry_isolation.py b/tests/registry_isolation.py new file mode 100644 index 000000000..e44c0929d --- /dev/null +++ b/tests/registry_isolation.py @@ -0,0 +1,31 @@ +"""Shared FeatureGroup registry-isolation helper (#845). + +A FeatureGroup subclass defined inside a test sits in a reference cycle, so it stays in +``FeatureGroup.__subclasses__()`` until cyclic GC runs and a later test on the same worker trips over it. +""" + +from __future__ import annotations + +import gc + +from mloda.core.abstract_plugins.components.utils import get_all_subclasses +from mloda.core.abstract_plugins.feature_group import FeatureGroup + + +def reclaim_leaked_feature_groups(before: set[type[FeatureGroup]], module_name: str) -> list[type[FeatureGroup]]: + """Collect FeatureGroup subclasses created since `before`; return the ones from `module_name` that survived.""" + + def new_from_module() -> list[type[FeatureGroup]]: + """A fresh list each call, so no survivor is held across a collection and pins what it would reclaim.""" + return [cls for cls in get_all_subclasses(FeatureGroup) - before if cls.__module__ == module_name] + + # Cheap path: a full collection costs ~1s, so only pay it when something new appeared. The gate reads ANY + # new subclass, not only this module's; only the RETURN value stays filtered to module_name. + if not get_all_subclasses(FeatureGroup) - before: + return [] + gc.collect() + if not new_from_module(): + return [] + # Something is left, so pay the second collection: it reclaims what the first one only made collectable. + gc.collect() + return sorted(new_from_module(), key=lambda cls: cls.__name__) diff --git a/tests/registry_isolation_probe.py b/tests/registry_isolation_probe.py new file mode 100644 index 000000000..30fe10373 --- /dev/null +++ b/tests/registry_isolation_probe.py @@ -0,0 +1,17 @@ +"""Helper for the registry-isolation tests (#845): defines a FeatureGroup subclass OUTSIDE a test module. + +A class built here carries THIS module's ``__module__``, which is what the reclaim gate filters on. +""" + +from __future__ import annotations + +from mloda.core.abstract_plugins.feature_group import FeatureGroup + + +def define_helper_subclass() -> str: + """Define a FeatureGroup subclass here and return only its name; returning the class would pin it.""" + + class HelperMadeRegistryProbe845rFeatureGroup(FeatureGroup): + pass + + return HelperMadeRegistryProbe845rFeatureGroup.__name__ diff --git a/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_author_guards_module_split.py b/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_author_guards_module_split.py index 78302344e..d1c64148e 100644 --- a/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_author_guards_module_split.py +++ b/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_author_guards_module_split.py @@ -82,7 +82,6 @@ PARSER_KEPT_MODULE_LEVEL = ( "record_match_rejection", "option_key_is_present", - "_contained_raise_log_level", "PropertyValueRejection", ) diff --git a/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_property_mapping_allowed_values.py b/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_property_mapping_allowed_values.py index 094d57ee3..e5726522b 100644 --- a/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_property_mapping_allowed_values.py +++ b/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_property_mapping_allowed_values.py @@ -21,7 +21,6 @@ from __future__ import annotations -import gc from typing import Any import pytest @@ -35,26 +34,9 @@ from mloda.core.abstract_plugins.components.feature_chainer.property_spec import PropertySpec from mloda.core.abstract_plugins.components.feature_set import FeatureSet from mloda.core.abstract_plugins.components.options import Options -from mloda.core.abstract_plugins.components.utils import get_all_subclasses from mloda.core.abstract_plugins.feature_group import FeatureGroup -@pytest.fixture(autouse=True) -def _no_feature_group_registry_pollution() -> Any: - """Guarantee this module never leaks throwaway FeatureGroup subclasses. - - Copied from ``test_property_mapping_default_invariant.py``: tests below define - FeatureGroup subclasses to exercise ``FeatureGroup.__init_subclass__``. Those - class objects sit in reference cycles, so we force a collection after each test - and assert that none of this module's classes remain registered. - """ - yield - gc.collect() - gc.collect() - leaked = [c for c in get_all_subclasses(FeatureGroup) if c.__module__ == __name__] - assert not leaked, f"Leaked FeatureGroup subclasses from {__name__}: {[c.__name__ for c in leaked]}" - - class TestStrictMembershipViaAllowedValues: """Strict membership flows through the explicit ``allowed_values`` field. diff --git a/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_property_mapping_spec_shape.py b/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_property_mapping_spec_shape.py index a084a9d6b..a8a026c2d 100644 --- a/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_property_mapping_spec_shape.py +++ b/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_property_mapping_spec_shape.py @@ -22,7 +22,6 @@ class body): from __future__ import annotations -import gc from typing import Any import pytest @@ -35,21 +34,6 @@ class body): from mloda.core.abstract_plugins.feature_group import FeatureGroup -@pytest.fixture(autouse=True) -def _no_feature_group_registry_pollution() -> Any: - """Guarantee this module never leaks throwaway FeatureGroup subclasses. - - Mirrors ``test_property_spec_hard_break.py``: the tests below define FeatureGroup - subclasses, which linger in ``FeatureGroup.__subclasses__()`` until a GC cycle runs and - would otherwise be seen by tests that enumerate feature groups. - """ - yield - gc.collect() - gc.collect() - leaked = [c for c in get_all_subclasses(FeatureGroup) if c.__module__ == __name__] - assert not leaked, f"Leaked FeatureGroup subclasses from {__name__}: {[c.__name__ for c in leaked]}" - - def _spec(*args: Any, **kwargs: Any) -> PropertySpec: """Build a ``PropertySpec`` through an untyped seam. diff --git a/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_property_spec_builder.py b/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_property_spec_builder.py index 7ec8818e3..d7c670074 100644 --- a/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_property_spec_builder.py +++ b/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_property_spec_builder.py @@ -18,7 +18,6 @@ from __future__ import annotations import copy -import gc import pickle from typing import Any @@ -33,7 +32,6 @@ from mloda.core.abstract_plugins.components.feature_chainer.property_spec import _NoDefault, is_no_default from mloda.core.abstract_plugins.components.feature_set import FeatureSet from mloda.core.abstract_plugins.components.options import Options -from mloda.core.abstract_plugins.components.utils import get_all_subclasses from mloda.core.abstract_plugins.feature_group import FeatureGroup from mloda.provider import NO_DEFAULT, PropertySpec, property_spec @@ -48,21 +46,6 @@ def _build(*args: Any, **kwargs: Any) -> PropertySpec: return property_spec(*args, **kwargs) -@pytest.fixture(autouse=True) -def _no_feature_group_registry_pollution() -> Any: - """Guarantee this module never leaks throwaway FeatureGroup subclasses. - - Copied from ``test_property_mapping_default_invariant.py``. The round-trip test - defines a FeatureGroup subclass; this fixture forces a collection afterwards - and asserts none of this module's classes linger in the registry. - """ - yield - gc.collect() - gc.collect() - leaked = [c for c in get_all_subclasses(FeatureGroup) if c.__module__ == __name__] - assert not leaked, f"Leaked FeatureGroup subclasses from {__name__}: {[c.__name__ for c in leaked]}" - - class TestPropertySpecImport: """The helper is exported from the public provider surface.""" diff --git a/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_property_spec_hard_break.py b/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_property_spec_hard_break.py index 80addcde0..effc329ed 100644 --- a/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_property_spec_hard_break.py +++ b/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_property_spec_hard_break.py @@ -20,7 +20,6 @@ from __future__ import annotations -import gc import importlib from typing import Any @@ -33,25 +32,9 @@ from mloda.core.abstract_plugins.components.feature_chainer.property_spec import PropertySpec, property_spec from mloda.core.abstract_plugins.components.feature_set import FeatureSet from mloda.core.abstract_plugins.components.options import Options -from mloda.core.abstract_plugins.components.utils import get_all_subclasses from mloda.core.abstract_plugins.feature_group import FeatureGroup -@pytest.fixture(autouse=True) -def _no_feature_group_registry_pollution() -> Any: - """Guarantee this module never leaks throwaway FeatureGroup subclasses. - - Mirrors ``test_property_mapping_spec_schema.py``: the class-definition tests below - define FeatureGroup subclasses, which linger in ``FeatureGroup.__subclasses__()`` until - a GC cycle runs and would otherwise be seen by tests that enumerate feature groups. - """ - yield - gc.collect() - gc.collect() - leaked = [c for c in get_all_subclasses(FeatureGroup) if c.__module__ == __name__] - assert not leaked, f"Leaked FeatureGroup subclasses from {__name__}: {[c.__name__ for c in leaked]}" - - def _hardbreak694_needs_order_column(options: Options) -> bool: """Predicate: the order column is required when the aggregation is order-dependent.""" return options.get("hardbreak694_agg") in {"first", "last"} diff --git a/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_required_when_enforced_on_override.py b/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_required_when_enforced_on_override.py index f25d37b6f..c748c6bdd 100644 --- a/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_required_when_enforced_on_override.py +++ b/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_required_when_enforced_on_override.py @@ -8,7 +8,6 @@ from __future__ import annotations -import gc import re from typing import Any @@ -35,18 +34,6 @@ COMPILED_PATTERN = re.compile(r".*__([\w]+)_compiled$") -@pytest.fixture(autouse=True) -def _no_feature_group_registry_pollution() -> Any: - """Reclaim the throwaway FeatureGroup subclasses defined inside the tests below. - - They sit in reference cycles, so they linger in ``FeatureGroup.__subclasses__()`` until a GC - cycle runs, and other tests enumerate that registry. - """ - yield - gc.collect() - gc.collect() - - class CountingPredicate: """required_when predicate that records how often it ran: order_by is required for op_type 'first'.""" diff --git a/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_universal_optional_matcher.py b/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_universal_optional_matcher.py index be496e88c..c23905c88 100644 --- a/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_universal_optional_matcher.py +++ b/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_universal_optional_matcher.py @@ -37,7 +37,6 @@ from mloda.core.abstract_plugins.components.feature_chainer.feature_chain_parser_mixin import FeatureChainParserMixin from mloda.core.abstract_plugins.components.feature_name import FeatureName from mloda.core.abstract_plugins.components.options import Options -from mloda.core.abstract_plugins.components.utils import get_all_subclasses from mloda.core.abstract_plugins.feature_group import FeatureGroup from mloda.provider import PropertySpec from mloda_plugins.feature_group.experimental.aggregated_feature_group.base import AggregatedFeatureGroup @@ -81,22 +80,6 @@ def _universal_matcher_warnings( return records -@pytest.fixture(autouse=True) -def _no_feature_group_registry_pollution() -> Any: - """Guarantee this module never leaks throwaway FeatureGroup subclasses. - - Its tests define local FeatureGroup subclasses (some whose matcher raises or matches any name). Those - class objects sit in reference cycles, lingering in FeatureGroup.__subclasses__() until a GC cycle runs; - while they linger, other tests that enumerate via get_all_subclasses(FeatureGroup) (e.g. test_resolve_feature) - trip over them. Force a collection and assert none of this module's classes remain. - """ - yield - gc.collect() - gc.collect() - leaked = [c for c in get_all_subclasses(FeatureGroup) if c.__module__ == __name__] - assert not leaked, f"Leaked FeatureGroup subclasses from {__name__}: {[c.__name__ for c in leaked]}" - - class TestUniversalMatcherWarns: """The guard warns when an inherited config matcher matches any feature name with empty options.""" diff --git a/tests/test_core/test_abstract_plugins/test_intake_default_canonicalization.py b/tests/test_core/test_abstract_plugins/test_intake_default_canonicalization.py index c50524901..3be80eef8 100644 --- a/tests/test_core/test_abstract_plugins/test_intake_default_canonicalization.py +++ b/tests/test_core/test_abstract_plugins/test_intake_default_canonicalization.py @@ -8,7 +8,6 @@ from __future__ import annotations -import gc import logging from typing import Any, Callable @@ -17,31 +16,12 @@ from mloda.core.abstract_plugins.components.feature import Feature from mloda.core.abstract_plugins.components.feature_set import FeatureSet from mloda.core.abstract_plugins.components.options import Options -from mloda.core.abstract_plugins.components.utils import get_all_subclasses from mloda.core.abstract_plugins.feature_group import FeatureGroup from mloda.provider import DataCreator, PropertySpec from mloda.user import FeatureName, PluginCollector, mloda from mloda_plugins.compute_framework.base_implementations.python_dict.python_dict_framework import PythonDictFramework -@pytest.fixture(autouse=True) -def _no_feature_group_registry_pollution() -> Any: - """Guarantee this module never leaks throwaway FeatureGroup subclasses. - - The tests below define FeatureGroup subclasses inside factory functions. Those class - objects sit in reference cycles, so they linger in ``FeatureGroup.__subclasses__()`` - until a GC cycle runs. While they linger, other tests that enumerate feature groups via - ``get_all_subclasses(FeatureGroup)`` trip over them. After each test we force a - collection to reclaim the now-unreferenced classes and assert that none of this - module's classes remain registered, pinning the no-pollution contract. - """ - yield - gc.collect() - gc.collect() - leaked = [c for c in get_all_subclasses(FeatureGroup) if c.__module__ == __name__] - assert not leaked, f"Leaked FeatureGroup subclasses from {__name__}: {[c.__name__ for c in leaked]}" - - # PROPERTY_MAPPING keys for the throwaway probes; the idc_ prefix keeps them unique to this module. IDC_CTX_KEY = "idc_ctx_default" # context concrete default (twin merge, test 1) IDC_GRP_KEY = "idc_grp_default" # group concrete default (twin merge, test 2) diff --git a/tests/test_core/test_abstract_plugins/test_materialize_defaults_boundary.py b/tests/test_core/test_abstract_plugins/test_materialize_defaults_boundary.py index cd248b954..dafa01236 100644 --- a/tests/test_core/test_abstract_plugins/test_materialize_defaults_boundary.py +++ b/tests/test_core/test_abstract_plugins/test_materialize_defaults_boundary.py @@ -20,7 +20,6 @@ from __future__ import annotations -import gc from typing import Any, Callable from uuid import uuid4 @@ -30,7 +29,6 @@ from mloda.core.abstract_plugins.components.feature_set import FeatureSet from mloda.core.abstract_plugins.components.options import Options from mloda.core.abstract_plugins.components.parallelization_modes import ParallelizationMode -from mloda.core.abstract_plugins.components.utils import get_all_subclasses from mloda.core.abstract_plugins.feature_group import FeatureGroup from mloda.core.abstract_plugins.function_extender import Extender, ExtenderHook from mloda.provider import DataCreator, PropertySpec @@ -38,24 +36,6 @@ from mloda_plugins.compute_framework.base_implementations.python_dict.python_dict_framework import PythonDictFramework -@pytest.fixture(autouse=True) -def _no_feature_group_registry_pollution() -> Any: - """Guarantee this module never leaks throwaway FeatureGroup subclasses. - - The tests below define FeatureGroup subclasses inside factory functions. Those class - objects sit in reference cycles, so they linger in ``FeatureGroup.__subclasses__()`` - until a GC cycle runs. While they linger, other tests that enumerate feature groups via - ``get_all_subclasses(FeatureGroup)`` trip over them. After each test we force a - collection to reclaim the now-unreferenced classes and assert that none of this - module's classes remain registered, pinning the no-pollution contract. - """ - yield - gc.collect() - gc.collect() - leaked = [c for c in get_all_subclasses(FeatureGroup) if c.__module__ == __name__] - assert not leaked, f"Leaked FeatureGroup subclasses from {__name__}: {[c.__name__ for c in leaked]}" - - # PROPERTY_MAPPING keys for the throwaway probes; the mdb_ prefix keeps them unique to this module. MDB_CTX_KEY = "mdb_ctx_default" # context concrete default MDB_GRP_KEY = "mdb_grp_default" # group concrete default (context=False) diff --git a/tests/test_core/test_abstract_plugins/test_options_with_defaults.py b/tests/test_core/test_abstract_plugins/test_options_with_defaults.py index 9d7682e29..0db7066af 100644 --- a/tests/test_core/test_abstract_plugins/test_options_with_defaults.py +++ b/tests/test_core/test_abstract_plugins/test_options_with_defaults.py @@ -12,7 +12,6 @@ from __future__ import annotations -import gc from typing import Any import pytest @@ -20,7 +19,6 @@ from mloda.core.abstract_plugins.components.feature_chainer.property_spec import is_no_default from mloda.core.abstract_plugins.components.feature_set import FeatureSet from mloda.core.abstract_plugins.components.options import Options -from mloda.core.abstract_plugins.components.utils import get_all_subclasses from mloda.core.abstract_plugins.feature_group import FeatureGroup from mloda.provider import PropertySpec from mloda_plugins.feature_group.experimental.aggregated_feature_group.base import AggregatedFeatureGroup @@ -37,25 +35,6 @@ from mloda_plugins.feature_group.experimental.time_window.base import TimeWindowFeatureGroup -@pytest.fixture(autouse=True) -def _no_feature_group_registry_pollution() -> Any: - """Guarantee this module never leaks throwaway FeatureGroup subclasses. - - The tests below define FeatureGroup subclasses to exercise the author experience. - Those class objects sit in reference cycles, so they linger in - ``FeatureGroup.__subclasses__()`` until a GC cycle runs. While they linger, other - tests that enumerate feature groups via ``get_all_subclasses(FeatureGroup)`` trip - over them. After each test we force a collection to reclaim the now-unreferenced - classes and assert that none of this module's classes remain registered, pinning - the no-pollution contract. - """ - yield - gc.collect() - gc.collect() - leaked = [c for c in get_all_subclasses(FeatureGroup) if c.__module__ == __name__] - assert not leaked, f"Leaked FeatureGroup subclasses from {__name__}: {[c.__name__ for c in leaked]}" - - # PROPERTY_MAPPING keys for the throwaway feature group, one per default edge under test. CTX_KEY = "owd_ctx_default" # context concrete default GRP_KEY = "owd_grp_default" # group concrete default (context=False) diff --git a/tests/test_core/test_abstract_plugins/test_property_mapping_default_invariant.py b/tests/test_core/test_abstract_plugins/test_property_mapping_default_invariant.py index 371c6973a..cc6346670 100644 --- a/tests/test_core/test_abstract_plugins/test_property_mapping_default_invariant.py +++ b/tests/test_core/test_abstract_plugins/test_property_mapping_default_invariant.py @@ -16,7 +16,6 @@ from __future__ import annotations -import gc from typing import Any import pytest @@ -30,25 +29,6 @@ from mloda.provider import NO_DEFAULT, PropertySpec -@pytest.fixture(autouse=True) -def _no_feature_group_registry_pollution() -> Any: - """Guarantee this module never leaks throwaway FeatureGroup subclasses. - - The tests below define FeatureGroup subclasses to exercise the author experience. - Those class objects sit in reference cycles, so they linger in - ``FeatureGroup.__subclasses__()`` until a GC cycle runs. While they linger, other - tests that enumerate feature groups via ``get_all_subclasses(FeatureGroup)`` trip - over them. After each test we force a collection to reclaim the now-unreferenced - classes and assert that none of this module's classes remain registered, pinning - the no-pollution contract. - """ - yield - gc.collect() - gc.collect() - leaked = [c for c in get_all_subclasses(FeatureGroup) if c.__module__ == __name__] - assert not leaked, f"Leaked FeatureGroup subclasses from {__name__}: {[c.__name__ for c in leaked]}" - - def _spec(*args: Any, **kwargs: Any) -> PropertySpec: """Build a ``PropertySpec`` through an untyped seam. diff --git a/tests/test_core/test_api/test_resolve_feature.py b/tests/test_core/test_api/test_resolve_feature.py index cb4178a82..fc81ab0cf 100644 --- a/tests/test_core/test_api/test_resolve_feature.py +++ b/tests/test_core/test_api/test_resolve_feature.py @@ -295,6 +295,17 @@ def _forwarded_mismatch_options() -> Options: return options +def _matches_own_class_name(fg: type[FeatureGroup]) -> bool: + """Whether fg matches its own class name; a raising matcher counts as no match. + + The sweeps below call every registered matcher, so one broken candidate must not error the test (#868). + """ + try: + return bool(fg.match_feature_group_criteria(FeatureName(fg.get_class_name()), Options(), None)) + except Exception: + return False + + class TestResolvedFeatureDataclass: """Tests for the ResolvedFeature dataclass structure.""" @@ -371,7 +382,7 @@ def _find_unambiguous_feature_name() -> str: all_fgs = get_all_subclasses(FeatureGroup) for fg in all_fgs: - if not fg.match_feature_group_criteria(FeatureName(fg.get_class_name()), Options(), None): + if not _matches_own_class_name(fg): continue result = resolve_feature(fg.get_class_name()) if result.feature_group is not None: @@ -439,7 +450,7 @@ def test_resolve_feature_candidates_contains_all_matches(self) -> None: for fg in all_fgs: if inspect.isabstract(fg): continue - if not fg.match_feature_group_criteria(FeatureName(fg.get_class_name()), Options(), None): + if not _matches_own_class_name(fg): continue candidates = resolve_feature(fg.get_class_name()).candidates if candidates: diff --git a/tests/test_core/test_filter/test_filter_criteria_effective_options.py b/tests/test_core/test_filter/test_filter_criteria_effective_options.py index 74e4f180a..ec84f6db1 100644 --- a/tests/test_core/test_filter/test_filter_criteria_effective_options.py +++ b/tests/test_core/test_filter/test_filter_criteria_effective_options.py @@ -8,32 +8,19 @@ from __future__ import annotations -import gc from typing import Any, Optional -import pytest from mloda.core.abstract_plugins.components.data_access_collection import DataAccessCollection from mloda.core.abstract_plugins.components.feature import Feature from mloda.core.abstract_plugins.components.feature_set import FeatureSet from mloda.core.abstract_plugins.components.options import Options -from mloda.core.abstract_plugins.components.utils import get_all_subclasses from mloda.core.abstract_plugins.feature_group import FeatureGroup from mloda.provider import DataCreator, PropertySpec from mloda.user import FeatureName, FilterType, GlobalFilter, PluginCollector, mloda from mloda_plugins.compute_framework.base_implementations.python_dict.python_dict_framework import PythonDictFramework -@pytest.fixture(autouse=True) -def _no_feature_group_registry_pollution() -> Any: - """Guarantee this module never leaks its throwaway FeatureGroup subclass (see the intake test's twin fixture).""" - yield - gc.collect() - gc.collect() - leaked = [c for c in get_all_subclasses(FeatureGroup) if c.__module__ == __name__] - assert not leaked, f"Leaked FeatureGroup subclasses from {__name__}: {[c.__name__ for c in leaked]}" - - # PROPERTY_MAPPING key/default for the throwaway probe; the pfc_ prefix keeps it unique to this module. PFC_KEY = "pfc_criteria_key" PFC_DEFAULT = "pfc_default_val" diff --git a/tests/test_core/test_filter/test_filter_feature_explicit_none_intake.py b/tests/test_core/test_filter/test_filter_feature_explicit_none_intake.py index 5e541b7c4..8c6b390d1 100644 --- a/tests/test_core/test_filter/test_filter_feature_explicit_none_intake.py +++ b/tests/test_core/test_filter/test_filter_feature_explicit_none_intake.py @@ -13,32 +13,17 @@ from __future__ import annotations -import gc from typing import Any, NamedTuple -import pytest - from mloda.core.abstract_plugins.components.feature import Feature from mloda.core.abstract_plugins.components.feature_set import FeatureSet from mloda.core.abstract_plugins.components.options import Options -from mloda.core.abstract_plugins.components.utils import get_all_subclasses from mloda.core.abstract_plugins.feature_group import FeatureGroup from mloda.provider import DataCreator, PropertySpec from mloda.user import FeatureName, FilterType, GlobalFilter, PluginCollector, mloda from mloda_plugins.compute_framework.base_implementations.python_dict.python_dict_framework import PythonDictFramework -@pytest.fixture(autouse=True) -def _no_feature_group_registry_pollution() -> Any: - """Guarantee this module never leaks its throwaway FeatureGroup subclasses: they sit in reference - cycles, so without a forced collection other tests enumerating feature groups trip over them.""" - yield - gc.collect() - gc.collect() - leaked = [c for c in get_all_subclasses(FeatureGroup) if c.__module__ == __name__] - assert not leaked, f"Leaked FeatureGroup subclasses from {__name__}: {[c.__name__ for c in leaked]}" - - # PROPERTY_MAPPING keys/defaults for the throwaway probes; the fen_ prefix keeps them unique to this module. FEN_GRP_KEY = "fen_grp_key" FEN_GRP_DEFAULT = "fen_grp_default_val" diff --git a/tests/test_core/test_prepare/test_contained_raise_option_isolation.py b/tests/test_core/test_prepare/test_contained_raise_option_isolation.py new file mode 100644 index 000000000..09e8a2679 --- /dev/null +++ b/tests/test_core/test_prepare/test_contained_raise_option_isolation.py @@ -0,0 +1,214 @@ +"""R3: a contained matcher raise must leave no partial mutation on the shared Feature.options (#845 follow-up). + +``feature.options`` is ONE mutable object shared by every candidate's match hook, and a matcher that +returns True must keep its write (that is how a matched reader is linked through mloda), so the rollback +is per candidate, not a whole-loop snapshot. Doubles carry an ``845`` suffix and are dropped per test. +""" + +import gc +from collections.abc import Callable +from dataclasses import dataclass +from functools import partial +from typing import Optional, TypeVar + +from mloda.core.abstract_plugins.components.data_access_collection import DataAccessCollection +from mloda.core.abstract_plugins.components.feature import Feature +from mloda.core.abstract_plugins.components.feature_name import FeatureName +from mloda.core.abstract_plugins.components.options import Options +from mloda.core.abstract_plugins.compute_framework import ComputeFramework +from mloda.core.abstract_plugins.feature_group import FeatureGroup +from mloda.core.prepare.accessible_plugins import FeatureGroupEnvironmentMapping +from mloda.core.prepare.identify_feature_group import IdentifyFeatureGroupClass + + +SHARED_FEATURE = "shared_option_isolation_feat_845r" +SIDE_EFFECT_KEY = "side_effect_845r" +SIDE_EFFECT_VALUE = "written_by_the_raising_candidate_845r" +LINKED_KEY = "linked_reader_845r" +LINKED_VALUE = "written_by_the_matching_candidate_845r" +RAISE_MESSAGE = "boom_845r_matcher_raised_after_writing" +RAISING_CLASS_NAME = "MutatingRaiseFG845r" +CLEAN_CLASS_NAME = "CleanNameOwnerFG845r" +LINKING_CLASS_NAME = "MutatingMatchFG845r" + +T = TypeVar("T") + + +class OptionIsolationFw845r(ComputeFramework): + """Dummy compute framework for the option-isolation tests.""" + + +def _capture(call: Callable[[], T]) -> tuple[Optional[T], Optional[str]]: + """Run call, returning (value, None) or (None, 'Type: message'). No traceback is retained.""" + try: + return call(), None + except Exception as exc: # noqa: BLE001 (an escape, or its absence, is the fact under test) + return None, f"{type(exc).__name__}: {exc}" + + +def _make_mutating_raise_fg() -> type[FeatureGroup]: + """Candidate that writes to the shared options and then raises an UNMARKED exception.""" + # Class objects are cyclic; collect leftovers from earlier tests before defining a twin. + gc.collect() + + class MutatingRaiseFG845r(FeatureGroup): + """Stands in for a plugin matcher that configures the feature before it breaks.""" + + @classmethod + def feature_names_supported(cls) -> set[str]: + return {SHARED_FEATURE} + + @classmethod + def compute_framework_rule(cls) -> set[type[ComputeFramework]] | None: + return {OptionIsolationFw845r} + + @classmethod + def match_feature_group_criteria( + cls, + feature_name: FeatureName | str, + options: Options, + data_access_collection: Optional[DataAccessCollection] = None, + ) -> bool: + if str(feature_name) != SHARED_FEATURE: + return False + options.add_to_group(SIDE_EFFECT_KEY, SIDE_EFFECT_VALUE) + raise RuntimeError(RAISE_MESSAGE) + + def input_features(self, options: Options, feature_name: FeatureName) -> Optional[set[Feature]]: + return None + + return MutatingRaiseFG845r + + +def _make_clean_owner_fg() -> type[FeatureGroup]: + """Rival candidate that claims the same feature name cleanly and writes nothing.""" + gc.collect() + + class CleanNameOwnerFG845r(FeatureGroup): + """The winner whose options must not carry the eliminated candidate's write.""" + + @classmethod + def feature_names_supported(cls) -> set[str]: + return {SHARED_FEATURE} + + @classmethod + def compute_framework_rule(cls) -> set[type[ComputeFramework]] | None: + return {OptionIsolationFw845r} + + def input_features(self, options: Options, feature_name: FeatureName) -> Optional[set[Feature]]: + return None + + return CleanNameOwnerFG845r + + +def _make_mutating_match_fg() -> type[FeatureGroup]: + """Candidate that writes to the shared options and RETURNS TRUE, the linked-reader pattern.""" + gc.collect() + + class MutatingMatchFG845r(FeatureGroup): + """Stands in for the match hook that links its matched reader through the shared options.""" + + @classmethod + def feature_names_supported(cls) -> set[str]: + return {SHARED_FEATURE} + + @classmethod + def compute_framework_rule(cls) -> set[type[ComputeFramework]] | None: + return {OptionIsolationFw845r} + + @classmethod + def match_feature_group_criteria( + cls, + feature_name: FeatureName | str, + options: Options, + data_access_collection: Optional[DataAccessCollection] = None, + ) -> bool: + if str(feature_name) != SHARED_FEATURE: + return False + if LINKED_KEY not in options: + options.add_to_group(LINKED_KEY, LINKED_VALUE) + return True + + def input_features(self, options: Options, feature_name: FeatureName) -> Optional[set[Feature]]: + return None + + return MutatingMatchFG845r + + +@dataclass(frozen=True) +class _OptionsSnapshot: + """Plain-data readout of one evaluation. Holds no class and no exception object.""" + + escaped: Optional[str] + identified_names: tuple[str, ...] + option_keys: tuple[str, ...] + side_effect_value: Optional[str] + linked_value: Optional[str] + + +def _evaluate(builders: tuple[Callable[[], type[FeatureGroup]], ...]) -> _OptionsSnapshot: + """Evaluate the shared feature against the given candidates, in order, and read the options out.""" + candidates = [build() for build in builders] + try: + feature = Feature(SHARED_FEATURE) + plugins: FeatureGroupEnvironmentMapping = {candidate: {OptionIsolationFw845r} for candidate in candidates} + result, escaped = _capture(partial(IdentifyFeatureGroupClass.evaluate, feature, plugins, None)) + identified = () if result is None else tuple(sorted(fg.get_class_name() for fg in result.identified)) + snapshot = _OptionsSnapshot( + escaped=escaped, + identified_names=identified, + option_keys=tuple(sorted(str(key) for key in feature.options.keys())), + side_effect_value=feature.options.get(SIDE_EFFECT_KEY), + linked_value=feature.options.get(LINKED_KEY), + ) + del result + del plugins + del feature + return snapshot + finally: + candidates.clear() + del candidates + gc.collect() + + +class TestContainedRaiseLeavesNoPartialMutation: + """An eliminated candidate must not configure the feature that won.""" + + def test_write_before_a_contained_raise_is_rolled_back(self) -> None: + """The raising candidate's write must not survive into the winning group's options.""" + snapshot = _evaluate((_make_mutating_raise_fg, _make_clean_owner_fg)) + + assert snapshot.escaped is None + assert snapshot.identified_names == (CLEAN_CLASS_NAME,) + assert snapshot.side_effect_value is None, ( + f"a contained raise must leave no partial mutation, found {SIDE_EFFECT_KEY}={snapshot.side_effect_value}" + ) + assert SIDE_EFFECT_KEY not in snapshot.option_keys + + +class TestMatchingMatcherKeepsItsMutation: + """The load-bearing counterpart: a matcher that returns True keeps what it wrote.""" + + def test_write_from_a_matching_candidate_survives(self) -> None: + """This is how a matched reader is linked through mloda, so the rollback must not touch it.""" + snapshot = _evaluate((_make_mutating_match_fg,)) + + assert snapshot.escaped is None + assert snapshot.identified_names == (LINKING_CLASS_NAME,) + assert snapshot.linked_value == LINKED_VALUE + assert LINKED_KEY in snapshot.option_keys + + +class TestRollbackIsPerCandidate: + """A whole-loop snapshot would also undo an earlier winner's write, so the rollback is per candidate.""" + + def test_earlier_matching_write_survives_a_later_contained_raise(self) -> None: + """The matching candidate runs first and keeps its write; only the later raising one is rolled back.""" + snapshot = _evaluate((_make_mutating_match_fg, _make_mutating_raise_fg)) + + assert snapshot.escaped is None + assert snapshot.identified_names == (LINKING_CLASS_NAME,) + assert snapshot.linked_value == LINKED_VALUE, "a later contained raise must not undo an earlier match's write" + assert snapshot.side_effect_value is None, ( + f"a contained raise must leave no partial mutation, found {SIDE_EFFECT_KEY}={snapshot.side_effect_value}" + ) diff --git a/tests/test_core/test_prepare/test_forwarded_mismatch_stays_loud.py b/tests/test_core/test_prepare/test_forwarded_mismatch_stays_loud.py new file mode 100644 index 000000000..83378141e --- /dev/null +++ b/tests/test_core/test_prepare/test_forwarded_mismatch_stays_loud.py @@ -0,0 +1,171 @@ +"""R1: the forwarded-name-mismatch guard must stay loud at the ENGINE seam (#845 follow-up). + +The damaging case is a SECOND group claiming the same name: contained, the mismatch candidate is dropped, +the rival wins and the forwarded value is silently ignored. Every existing test for this guard calls the +matcher directly, so these pins go through the resolution seam. Doubles are dropped per test. +""" + +import gc +from collections.abc import Callable +from dataclasses import dataclass +from functools import partial +from typing import Optional, TypeVar + +from mloda.core.abstract_plugins.components.feature import Feature +from mloda.core.abstract_plugins.components.feature_chainer.feature_chain_parser_mixin import FeatureChainParserMixin +from mloda.core.abstract_plugins.components.feature_chainer.property_spec import property_spec +from mloda.core.abstract_plugins.components.feature_name import FeatureName +from mloda.core.abstract_plugins.components.options import Options +from mloda.core.abstract_plugins.compute_framework import ComputeFramework +from mloda.core.abstract_plugins.feature_group import FeatureGroup +from mloda.core.abstract_plugins.components.data_access_collection import DataAccessCollection +from mloda.core.prepare.accessible_plugins import FeatureGroupEnvironmentMapping + +from tests.test_core.test_prepare.identify_seam import evaluate_or_raise + + +PROBE_TYPE_KEY = "forward_probe_type_845r" +LOUD_FEATURE = "value__median_loudprobe845r" +MISMATCH_CLASS_NAME = "ForwardMismatchLoudFG845r" +RIVAL_CLASS_NAME = "RivalNameOwnerFG845r" +RAISE_TYPE_NAME = "ValueError" + +T = TypeVar("T") + + +class LoudProbeFw845r(ComputeFramework): + """Dummy compute framework for the forwarded-mismatch seam tests.""" + + +def _capture(call: Callable[[], T]) -> tuple[Optional[T], Optional[str]]: + """Run call, returning (value, None) or (None, 'Type: message'). No traceback is retained.""" + try: + return call(), None + except Exception as exc: # noqa: BLE001 (an escape, or its absence, is the fact under test) + return None, f"{type(exc).__name__}: {exc}" + + +def _forwarded_mismatch_options() -> Options: + """Options whose forwarded probe type ('sum') contradicts the name-parsed one ('median').""" + options = Options(group={PROBE_TYPE_KEY: "sum"}) + options.inherited_group_keys = frozenset({PROBE_TYPE_KEY}) + return options + + +def _make_mismatch_fg() -> type[FeatureGroup]: + """Chain-parsed candidate whose matcher raises the framework forwarded/name mismatch ValueError.""" + # Class objects are cyclic; collect leftovers from earlier tests before defining a twin. + gc.collect() + + class ForwardMismatchLoudFG845r(FeatureChainParserMixin, FeatureGroup): + """Binds the probe type from the feature name, so a contradicting forwarded value is a mismatch.""" + + PREFIX_PATTERN = r".*__([\w]+)_loudprobe845r$" + + PROPERTY_MAPPING = { + PROBE_TYPE_KEY: property_spec( + "Probe operation subtype.", + strict=True, + allowed_values={"median": "Median value", "sum": "Sum of values"}, + context=True, + ), + } + + @classmethod + def compute_framework_rule(cls) -> set[type[ComputeFramework]] | None: + return {LoudProbeFw845r} + + return ForwardMismatchLoudFG845r + + +def _make_rival_fg() -> type[FeatureGroup]: + """Rival candidate claiming the same feature name cleanly, with no interest in the forwarded option.""" + gc.collect() + + class RivalNameOwnerFG845r(FeatureGroup): + """Matches the probe feature name unconditionally: the group that would silently win today.""" + + @classmethod + def compute_framework_rule(cls) -> set[type[ComputeFramework]] | None: + return {LoudProbeFw845r} + + @classmethod + def match_feature_group_criteria( + cls, + feature_name: FeatureName | str, + options: Options, + data_access_collection: Optional[DataAccessCollection] = None, + ) -> bool: + return str(feature_name) == LOUD_FEATURE + + def input_features(self, options: Options, feature_name: FeatureName) -> Optional[set[Feature]]: + return None + + return RivalNameOwnerFG845r + + +@dataclass(frozen=True) +class _SeamSnapshot: + """Plain-data readout of one seam evaluation. Holds no class and no exception object.""" + + escaped: Optional[str] + identified_names: tuple[str, ...] + + +def _evaluate_at_seam(options: Options, with_rival: bool) -> _SeamSnapshot: + """Resolve the probe feature through the engine seam and read the outcome out as plain data.""" + mismatch_fg = _make_mismatch_fg() + rival_fg = _make_rival_fg() if with_rival else None + try: + feature = Feature(LOUD_FEATURE, options=options) + plugins: FeatureGroupEnvironmentMapping = {mismatch_fg: {LoudProbeFw845r}} + if rival_fg is not None: + plugins[rival_fg] = {LoudProbeFw845r} + result, escaped = _capture(partial(evaluate_or_raise, feature, plugins, None)) + identified = () if result is None else tuple(sorted(fg.get_class_name() for fg in result.identified)) + snapshot = _SeamSnapshot(escaped=escaped, identified_names=identified) + del result + del plugins + del feature + return snapshot + finally: + del mismatch_fg + del rival_fg + gc.collect() + + +class TestForwardedMismatchStaysLoudAtTheSeam: + """The mismatch guard is framework-owned: its raise must cross the match seam, not be contained.""" + + def test_mismatch_raise_reaches_the_caller(self) -> None: + """A contradicting forwarded value aborts the resolution with the guard's own ValueError.""" + snapshot = _evaluate_at_seam(_forwarded_mismatch_options(), with_rival=False) + + assert snapshot.escaped is not None, "the forwarded-name-mismatch guard must not be contained" + assert snapshot.escaped.startswith(f"{RAISE_TYPE_NAME}: "), ( + f"the guard's own ValueError must reach the caller, got: {snapshot.escaped}" + ) + assert PROBE_TYPE_KEY in snapshot.escaped + assert "forwarded" in snapshot.escaped.lower() + assert snapshot.identified_names == () + + def test_mismatch_is_not_dropped_when_a_rival_claims_the_name(self) -> None: + """A rival matching the same name must not swallow the mismatch and win with the value ignored.""" + snapshot = _evaluate_at_seam(_forwarded_mismatch_options(), with_rival=True) + + assert snapshot.identified_names != (RIVAL_CLASS_NAME,), ( + "the rival must not silently win while the forwarded value is ignored" + ) + assert snapshot.escaped is not None, "a rival candidate must not hide the forwarded-name-mismatch abort" + assert snapshot.escaped.startswith(f"{RAISE_TYPE_NAME}: "), ( + f"the guard's own ValueError must reach the caller, got: {snapshot.escaped}" + ) + assert PROBE_TYPE_KEY in snapshot.escaped + assert "forwarded" in snapshot.escaped.lower() + + def test_probe_group_resolves_without_a_conflicting_forwarded_option(self) -> None: + """Control: only the mismatch case aborts; the chain-parsed group is otherwise an ordinary winner.""" + snapshot = _evaluate_at_seam(Options(), with_rival=False) + + assert snapshot.escaped is None + assert snapshot.identified_names == (MISMATCH_CLASS_NAME,) diff --git a/tests/test_core/test_prepare/test_match_abort_escalation.py b/tests/test_core/test_prepare/test_match_abort_escalation.py new file mode 100644 index 000000000..7e2122432 --- /dev/null +++ b/tests/test_core/test_prepare/test_match_abort_escalation.py @@ -0,0 +1,244 @@ +"""os-005: a framework-owned raise escapes the match seam; an unmarked raise stays contained (#845). + +The provenance marker must preserve the exception object exactly, because callers assert on its original +type at the matcher boundary. ``resolve_feature`` keeps its never-raises contract: a marked raise reaches +``ResolvedFeature.error`` instead of propagating. Doubles are dropped per test. +""" + +import gc +from collections.abc import Callable +from dataclasses import dataclass +from functools import partial +from typing import Optional, TypeVar + +from mloda.core.abstract_plugins.components.data_access_collection import DataAccessCollection +from mloda.core.abstract_plugins.components.feature import Feature +from mloda.core.abstract_plugins.components.feature_name import FeatureName +from mloda.core.abstract_plugins.components.options import Options +from mloda.core.abstract_plugins.components.utils import escalate_match_abort, is_match_abort +from mloda.core.abstract_plugins.compute_framework import ComputeFramework +from mloda.core.abstract_plugins.feature_group import FeatureGroup +from mloda.core.prepare.accessible_plugins import FeatureGroupEnvironmentMapping +from mloda.core.prepare.identify_feature_group import IdentifyFeatureGroupClass +from mloda.steward import resolve_feature + + +MARKED_MESSAGE = "boom_845e_framework_owned_raise" +UNMARKED_MESSAGE = "boom_845e_plugin_owned_raise" +MARKED_FEATURE = "match_abort_marked_feat_845e" +UNMARKED_FEATURE = "match_abort_unmarked_feat_845e" +MARKED_CLASS_NAME = "MarkedRaiseFG845e" +UNMARKED_CLASS_NAME = "UnmarkedRaiseFG845e" +RAISE_TYPE_NAME = "ValueError" +MATCHER_ERROR_STAGE = "matcher_error" + +T = TypeVar("T") + + +class MatchAbortFw845e(ComputeFramework): + """Dummy compute framework for the match-abort escalation tests.""" + + +def _capture(call: Callable[[], T]) -> tuple[Optional[T], Optional[str]]: + """Run call, returning (value, None) or (None, 'Type: message'). No traceback is retained.""" + try: + return call(), None + except Exception as exc: # noqa: BLE001 (an escape, or its absence, is the fact under test) + return None, f"{type(exc).__name__}: {exc}" + + +def _make_marked_raise_fg() -> type[FeatureGroup]: + """Candidate whose matcher raises a MARKED ValueError for its own feature name.""" + # Class objects are cyclic; collect leftovers from earlier tests before defining a twin. + gc.collect() + + class MarkedRaiseFG845e(FeatureGroup): + """Stands in for framework-owned code raising inside the match hook.""" + + @classmethod + def feature_names_supported(cls) -> set[str]: + return {MARKED_FEATURE} + + @classmethod + def compute_framework_rule(cls) -> set[type[ComputeFramework]] | None: + return {MatchAbortFw845e} + + @classmethod + def match_feature_group_criteria( + cls, + feature_name: FeatureName | str, + options: Options, + data_access_collection: Optional[DataAccessCollection] = None, + ) -> bool: + if str(feature_name) != MARKED_FEATURE: + return False + raise escalate_match_abort(ValueError(MARKED_MESSAGE)) + + def input_features(self, options: Options, feature_name: FeatureName) -> Optional[set[Feature]]: + return None + + return MarkedRaiseFG845e + + +def _make_unmarked_raise_fg() -> type[FeatureGroup]: + """Twin of the marked double, raising the same ValueError type WITHOUT the marker.""" + gc.collect() + + class UnmarkedRaiseFG845e(FeatureGroup): + """Stands in for a plugin matcher that simply breaks: contained as a non-match (#845).""" + + @classmethod + def feature_names_supported(cls) -> set[str]: + return {UNMARKED_FEATURE} + + @classmethod + def compute_framework_rule(cls) -> set[type[ComputeFramework]] | None: + return {MatchAbortFw845e} + + @classmethod + def match_feature_group_criteria( + cls, + feature_name: FeatureName | str, + options: Options, + data_access_collection: Optional[DataAccessCollection] = None, + ) -> bool: + if str(feature_name) != UNMARKED_FEATURE: + return False + raise ValueError(UNMARKED_MESSAGE) + + def input_features(self, options: Options, feature_name: FeatureName) -> Optional[set[Feature]]: + return None + + return UnmarkedRaiseFG845e + + +@dataclass(frozen=True) +class _ContainedSnapshot: + """Plain-data readout of one contained evaluation. Holds no class and no exception object.""" + + escaped: Optional[str] + failure_kind: Optional[str] + eliminated_names: tuple[str, ...] + stage: Optional[str] + reason: Optional[str] + + +def _evaluate_marked_raise() -> Optional[str]: + """Evaluate the marked double's own feature; return the escaping raise as 'Type: message', else None.""" + broken_fg = _make_marked_raise_fg() + try: + feature = Feature(MARKED_FEATURE) + plugins: FeatureGroupEnvironmentMapping = {broken_fg: {MatchAbortFw845e}} + result, escaped = _capture(partial(IdentifyFeatureGroupClass.evaluate, feature, plugins, None)) + del result + del plugins + return escaped + finally: + del broken_fg + gc.collect() + + +def _evaluate_unmarked_raise() -> _ContainedSnapshot: + """Evaluate the unmarked double's own feature and read the containment out as plain data.""" + broken_fg = _make_unmarked_raise_fg() + try: + feature = Feature(UNMARKED_FEATURE) + plugins: FeatureGroupEnvironmentMapping = {broken_fg: {MatchAbortFw845e}} + result, escaped = _capture(partial(IdentifyFeatureGroupClass.evaluate, feature, plugins, None)) + if result is None: + return _ContainedSnapshot(escaped, None, (), None, None) + + elimination = result.eliminations.get(broken_fg) + snapshot = _ContainedSnapshot( + escaped=None, + failure_kind=result.failure_kind, + eliminated_names=tuple(sorted(fg.get_class_name() for fg in result.eliminations)), + stage=None if elimination is None else str(elimination.stage), + reason=None if elimination is None else str(elimination.reason), + ) + del elimination + del result + del plugins + return snapshot + finally: + del broken_fg + gc.collect() + + +class TestEscalateMatchAbortPreservesTheException: + """The marker is provenance only: the exception object, its type, message and args stay untouched.""" + + def test_escalate_returns_the_same_object_and_marks_it(self) -> None: + """escalate_match_abort marks in place: same identity, same type, same message, same args.""" + exc = ValueError(MARKED_MESSAGE) + + marked = escalate_match_abort(exc) + + assert marked is exc + assert type(marked) is ValueError + assert str(marked) == MARKED_MESSAGE + assert marked.args == (MARKED_MESSAGE,) + assert is_match_abort(marked) is True + + def test_escalate_preserves_a_keyerror_unchanged(self) -> None: + """A KeyError stays a KeyError with its own str()/args, so pytest.raises(KeyError, ...) still holds.""" + exc = KeyError(MARKED_MESSAGE) + + marked = escalate_match_abort(exc) + + assert marked is exc + assert type(marked) is KeyError + assert str(marked) == str(KeyError(MARKED_MESSAGE)) + assert marked.args == (MARKED_MESSAGE,) + assert is_match_abort(marked) is True + + def test_unmarked_exception_is_not_a_match_abort(self) -> None: + """An ordinary exception is unmarked, so the seam keeps containing it.""" + assert is_match_abort(ValueError(UNMARKED_MESSAGE)) is False + + +class TestMatchAbortCrossesTheMatchSeam: + """A marked raise escapes evaluate(); an unmarked one is still contained as a matcher_error near-miss.""" + + def test_marked_matcher_raise_propagates_out_of_evaluate(self) -> None: + """The seam re-raises the marked exception with its original type and message.""" + escaped = _evaluate_marked_raise() + + assert escaped == f"{RAISE_TYPE_NAME}: {MARKED_MESSAGE}", ( + "a framework-owned raise must cross the match seam unchanged, not be contained as a non-match" + ) + + def test_unmarked_matcher_raise_stays_contained(self) -> None: + """The #845 containment is unchanged for an unmarked raise: skipped, recorded as matcher_error.""" + snapshot = _evaluate_unmarked_raise() + + assert snapshot.escaped is None + assert snapshot.failure_kind == "none" + assert snapshot.eliminated_names == (UNMARKED_CLASS_NAME,) + assert snapshot.stage == MATCHER_ERROR_STAGE + assert snapshot.reason is not None + assert RAISE_TYPE_NAME in snapshot.reason + assert UNMARKED_MESSAGE in snapshot.reason + + +class TestResolveFeatureStillNeverRaises: + """The debug path degrades the escaping raise into ResolvedFeature.error instead of propagating.""" + + def test_marked_raise_reaches_resolve_feature_error(self) -> None: + """resolve_feature reports the marked message as its error, with no winner and no no-match text.""" + marked_fg = _make_marked_raise_fg() + try: + result = resolve_feature(MARKED_FEATURE) + winner_name = result.feature_group.get_class_name() if result.feature_group is not None else None + error = result.error + del result + finally: + del marked_fg + gc.collect() + + assert winner_name is None + assert error is not None + assert MARKED_MESSAGE in error + assert "No feature groups found" not in error, ( + "a framework-owned raise must not be converted into the standard no-match error" + ) diff --git a/tests/test_core/test_prepare/test_match_abort_marker_robustness.py b/tests/test_core/test_prepare/test_match_abort_marker_robustness.py new file mode 100644 index 000000000..675190e24 --- /dev/null +++ b/tests/test_core/test_prepare/test_match_abort_marker_robustness.py @@ -0,0 +1,209 @@ +"""R2: ``is_match_abort`` must not fail open on a hostile exception (#845 follow-up). + +A ``__getattr__`` that raises would blow the marker read up inside the seam's own ``except`` block, and a +permissively truthy one would fake framework provenance, so the marker is read from ``exc.__dict__`` +(every ``BaseException`` has one, including a ``__slots__ = ()`` subclass). Both directions are pinned at +the helper level AND through the seam, where a raise out of the ``except`` block is uncontained. +""" + +import gc +from collections.abc import Callable +from dataclasses import dataclass +from functools import partial +from typing import Any, Optional, TypeVar + +from mloda.core.abstract_plugins.components.data_access_collection import DataAccessCollection +from mloda.core.abstract_plugins.components.feature import Feature +from mloda.core.abstract_plugins.components.feature_name import FeatureName +from mloda.core.abstract_plugins.components.options import Options +from mloda.core.abstract_plugins.components.utils import escalate_match_abort, is_match_abort +from mloda.core.abstract_plugins.compute_framework import ComputeFramework +from mloda.core.abstract_plugins.feature_group import FeatureGroup +from mloda.core.prepare.accessible_plugins import FeatureGroupEnvironmentMapping +from mloda.core.prepare.identify_feature_group import IdentifyFeatureGroupClass + + +HOSTILE_MESSAGE = "boom_845r_hostile_attribute_access" +PERMISSIVE_MESSAGE = "boom_845r_permissive_attribute_access" +SLOTTED_MESSAGE = "boom_845r_slotted_framework_raise" +HOSTILE_FEATURE = "hostile_marker_feat_845r" +PERMISSIVE_FEATURE = "permissive_marker_feat_845r" +HOSTILE_CLASS_NAME = "HostileMarkerFG845r" +PERMISSIVE_CLASS_NAME = "PermissiveMarkerFG845r" +MATCHER_ERROR_STAGE = "matcher_error" + +T = TypeVar("T") + + +class MarkerProbeFw845r(ComputeFramework): + """Dummy compute framework for the marker-robustness tests.""" + + +def _is_dunder(name: str) -> bool: + """Dunder lookups stay untouched so the interpreter's own machinery keeps working on these doubles.""" + return name.startswith("__") and name.endswith("__") + + +class HostileGetattrError845r(Exception): + """Exception whose attribute access raises, modelling a proxying or broken exception class.""" + + def __getattr__(self, name: str) -> Any: + if _is_dunder(name): + raise AttributeError(name) + raise RuntimeError(f"hostile attribute access for '{name}'") + + +class PermissiveGetattrError845r(Exception): + """Exception whose attribute access is truthy for everything, modelling a mock-like exception class.""" + + def __getattr__(self, name: str) -> Any: + if _is_dunder(name): + raise AttributeError(name) + return True + + +class SlottedFrameworkError845r(Exception): + """Framework-owned raise declaring no instance layout of its own; BaseException still supplies __dict__.""" + + __slots__ = () + + +def _capture(call: Callable[[], T]) -> tuple[Optional[T], Optional[str]]: + """Run call, returning (value, None) or (None, 'Type: message'). No traceback is retained.""" + try: + return call(), None + except Exception as exc: # noqa: BLE001 (an escape, or its absence, is the fact under test) + return None, f"{type(exc).__name__}: {exc}" + + +def _make_raising_fg(class_name: str, feature_name: str, exc_factory: Callable[[], Exception]) -> type[FeatureGroup]: + """Build a candidate whose matcher raises the given exception for its own feature name.""" + # Class objects are cyclic; collect leftovers from earlier tests before defining a twin. + gc.collect() + + class MarkerProbeFG845r(FeatureGroup): + """Raises an exception whose attribute protocol is what the marker read must survive.""" + + @classmethod + def feature_names_supported(cls) -> set[str]: + return {feature_name} + + @classmethod + def compute_framework_rule(cls) -> set[type[ComputeFramework]] | None: + return {MarkerProbeFw845r} + + @classmethod + def match_feature_group_criteria( + cls, + name: FeatureName | str, + options: Options, + data_access_collection: Optional[DataAccessCollection] = None, + ) -> bool: + if str(name) != feature_name: + return False + raise exc_factory() + + def input_features(self, options: Options, feature_name_arg: FeatureName) -> Optional[set[Feature]]: + return None + + MarkerProbeFG845r.__name__ = class_name + MarkerProbeFG845r.__qualname__ = class_name + return MarkerProbeFG845r + + +@dataclass(frozen=True) +class _ContainmentSnapshot: + """Plain-data readout of one seam evaluation. Holds no class and no exception object.""" + + escaped: Optional[str] + failure_kind: Optional[str] + eliminated_names: tuple[str, ...] + stage: Optional[str] + reason: Optional[str] + + +def _evaluate_raising_matcher( + class_name: str, feature_name: str, exc_factory: Callable[[], Exception] +) -> _ContainmentSnapshot: + """Evaluate the double's own feature name and read the containment out as plain data.""" + broken_fg = _make_raising_fg(class_name, feature_name, exc_factory) + try: + feature = Feature(feature_name) + plugins: FeatureGroupEnvironmentMapping = {broken_fg: {MarkerProbeFw845r}} + result, escaped = _capture(partial(IdentifyFeatureGroupClass.evaluate, feature, plugins, None)) + if result is None: + return _ContainmentSnapshot(escaped, None, (), None, None) + + elimination = result.eliminations.get(broken_fg) + snapshot = _ContainmentSnapshot( + escaped=None, + failure_kind=result.failure_kind, + eliminated_names=tuple(sorted(fg.get_class_name() for fg in result.eliminations)), + stage=None if elimination is None else str(elimination.stage), + reason=None if elimination is None else str(elimination.reason), + ) + del elimination + del result + del plugins + return snapshot + finally: + del broken_fg + gc.collect() + + +class TestIsMatchAbortReadsProvenanceOnly: + """The marker read answers from what escalate_match_abort wrote, never from an attribute hook.""" + + def test_hostile_getattr_is_not_a_match_abort(self) -> None: + """A raising __getattr__ must read as unmarked, not blow up the read itself.""" + assert is_match_abort(HostileGetattrError845r(HOSTILE_MESSAGE)) is False + + def test_permissive_getattr_is_not_a_match_abort(self) -> None: + """A truthy-for-everything __getattr__ must not be mistaken for framework provenance.""" + assert is_match_abort(PermissiveGetattrError845r(PERMISSIVE_MESSAGE)) is False + + def test_marking_still_works_on_a_slotted_exception(self) -> None: + """Control: a genuinely marked raise is still framework-owned, even with __slots__ = ().""" + exc = SlottedFrameworkError845r(SLOTTED_MESSAGE) + + marked = escalate_match_abort(exc) + + assert marked is exc + assert is_match_abort(marked) is True + assert is_match_abort(SlottedFrameworkError845r(SLOTTED_MESSAGE)) is False + + +class TestHostileExceptionStaysContainedAtTheSeam: + """A raising __getattr__ must not turn one broken candidate into a poisoned resolution.""" + + def test_hostile_matcher_raise_does_not_escape_evaluate(self) -> None: + """The marker read runs inside the seam's except block, so it must never raise there.""" + snapshot = _evaluate_raising_matcher( + HOSTILE_CLASS_NAME, HOSTILE_FEATURE, lambda: HostileGetattrError845r(HOSTILE_MESSAGE) + ) + + assert snapshot.escaped is None, ( + f"reading the marker must not let a new exception escape the seam, got: {snapshot.escaped}" + ) + assert snapshot.failure_kind == "none" + assert snapshot.eliminated_names == (HOSTILE_CLASS_NAME,) + assert snapshot.stage == MATCHER_ERROR_STAGE + + +class TestPermissiveExceptionIsNotEscalated: + """A permissive attribute hook must not buy a plugin raise framework provenance.""" + + def test_permissive_matcher_raise_stays_contained(self) -> None: + """Without a real escalate_match_abort call the raise is contained as a matcher_error near-miss.""" + snapshot = _evaluate_raising_matcher( + PERMISSIVE_CLASS_NAME, PERMISSIVE_FEATURE, lambda: PermissiveGetattrError845r(PERMISSIVE_MESSAGE) + ) + + assert snapshot.escaped is None, ( + f"an unmarked raise must stay contained whatever its __getattr__ answers, got: {snapshot.escaped}" + ) + assert snapshot.failure_kind == "none" + assert snapshot.eliminated_names == (PERMISSIVE_CLASS_NAME,) + assert snapshot.stage == MATCHER_ERROR_STAGE + assert snapshot.reason is not None + assert "PermissiveGetattrError845r" in snapshot.reason diff --git a/tests/test_core/test_prepare/test_match_data_conflict_stays_loud.py b/tests/test_core/test_prepare/test_match_data_conflict_stays_loud.py new file mode 100644 index 000000000..6b6a4884e --- /dev/null +++ b/tests/test_core/test_prepare/test_match_data_conflict_stays_loud.py @@ -0,0 +1,162 @@ +"""R4: the MatchData two-readers conflict must stay loud at the match seam (#845 follow-up). + +``MatchData.add_base_input_data_to_options`` raises the same "already set with different values" conflict +its ``BaseInputData`` twin already marks, and it is reachable from the match hook. The setup mirrors +``tests/test_plugins/feature_group/input_data/test_read.py::TestTwoReader``: one feature already carries a +feature-scope connection while the run also offers a global-scope one. Doubles are dropped per test. +""" + +import gc +from collections.abc import Callable +from dataclasses import dataclass +from functools import partial +from typing import Any, Optional, TypeVar + +from mloda.core.abstract_plugins.components.data_access_collection import DataAccessCollection +from mloda.core.abstract_plugins.components.feature import Feature +from mloda.core.abstract_plugins.components.feature_name import FeatureName +from mloda.core.abstract_plugins.components.match_data.match_data import MatchData +from mloda.core.abstract_plugins.components.options import Options +from mloda.core.abstract_plugins.compute_framework import ComputeFramework +from mloda.core.abstract_plugins.feature_group import FeatureGroup +from mloda.core.prepare.accessible_plugins import FeatureGroupEnvironmentMapping +from mloda.core.prepare.identify_feature_group import IdentifyFeatureGroupClass + + +CONFLICT_FEATURE = "match_data_conflict_feat_845r" +CONFLICT_CLASS_NAME = "ConflictMatchDataFG845r" +RIVAL_CLASS_NAME = "MatchDataRivalFG845r" +FEATURE_SCOPE_ACCESS = "feature_scope_conn_845r" +GLOBAL_SCOPE_ACCESS = "global_scope_conn_845r" +CONFLICT_TEXT = "already set with different values" +RAISE_TYPE_NAME = "ValueError" + +T = TypeVar("T") + + +class MatchDataFw845r(ComputeFramework): + """Dummy compute framework for the MatchData conflict tests.""" + + +def _capture(call: Callable[[], T]) -> tuple[Optional[T], Optional[str]]: + """Run call, returning (value, None) or (None, 'Type: message'). No traceback is retained.""" + try: + return call(), None + except Exception as exc: # noqa: BLE001 (an escape, or its absence, is the fact under test) + return None, f"{type(exc).__name__}: {exc}" + + +def _make_conflicting_match_data_fg() -> type[FeatureGroup]: + """Candidate whose global-scope access contradicts the feature-scope one already in the options.""" + # Class objects are cyclic; collect leftovers from earlier tests before defining a twin. + gc.collect() + + class ConflictMatchDataFG845r(FeatureGroup, MatchData): + """Declines the feature-scope connection, then resolves a different global-scope one.""" + + @classmethod + def feature_names_supported(cls) -> set[str]: + return {CONFLICT_FEATURE} + + @classmethod + def compute_framework_rule(cls) -> set[type[ComputeFramework]] | None: + return {MatchDataFw845r} + + @classmethod + def match_data_access( + cls, + feature_name: str, + options: Options, + data_access_collection: Optional[DataAccessCollection] = None, + framework_connection_object: Optional[Any] = None, + ) -> Any: + if str(feature_name) != CONFLICT_FEATURE: + return None + # Only the global scope resolves, so the feature-scope value stays in the options unclaimed. + if data_access_collection is None: + return None + return GLOBAL_SCOPE_ACCESS + + def input_features(self, options: Options, feature_name: FeatureName) -> Optional[set[Feature]]: + return None + + return ConflictMatchDataFG845r + + +def _make_rival_fg() -> type[FeatureGroup]: + """Rival candidate claiming the same feature name cleanly, so a contained conflict would let it win.""" + gc.collect() + + class MatchDataRivalFG845r(FeatureGroup): + """The group that would silently win while the two-readers misconfiguration is swallowed.""" + + @classmethod + def feature_names_supported(cls) -> set[str]: + return {CONFLICT_FEATURE} + + @classmethod + def compute_framework_rule(cls) -> set[type[ComputeFramework]] | None: + return {MatchDataFw845r} + + def input_features(self, options: Options, feature_name: FeatureName) -> Optional[set[Feature]]: + return None + + return MatchDataRivalFG845r + + +@dataclass(frozen=True) +class _ConflictSnapshot: + """Plain-data readout of one evaluation. Holds no class and no exception object.""" + + escaped: Optional[str] + identified_names: tuple[str, ...] + + +def _evaluate_conflict(with_rival: bool) -> _ConflictSnapshot: + """Evaluate the conflicting feature at the seam and read the outcome out as plain data.""" + conflict_fg = _make_conflicting_match_data_fg() + rival_fg = _make_rival_fg() if with_rival else None + try: + options = Options(group={CONFLICT_CLASS_NAME: FEATURE_SCOPE_ACCESS}) + feature = Feature(CONFLICT_FEATURE, options=options) + data_access = DataAccessCollection(connections={"match_data_handle_845r": GLOBAL_SCOPE_ACCESS}) + plugins: FeatureGroupEnvironmentMapping = {conflict_fg: {MatchDataFw845r}} + if rival_fg is not None: + plugins[rival_fg] = {MatchDataFw845r} + result, escaped = _capture(partial(IdentifyFeatureGroupClass.evaluate, feature, plugins, None, data_access)) + identified = () if result is None else tuple(sorted(fg.get_class_name() for fg in result.identified)) + snapshot = _ConflictSnapshot(escaped=escaped, identified_names=identified) + del result + del plugins + del feature + return snapshot + finally: + del conflict_fg + del rival_fg + gc.collect() + + +class TestMatchDataConflictAbortsTheMatch: + """Two conflicting readers for one feature is a misconfiguration, not a non-match.""" + + def test_conflict_reaches_the_caller(self) -> None: + """The conflict ValueError must cross the match seam instead of becoming a matcher_error near-miss.""" + snapshot = _evaluate_conflict(with_rival=False) + + assert snapshot.escaped is not None, "the two-readers conflict must not be contained as a non-match" + assert snapshot.escaped.startswith(f"{RAISE_TYPE_NAME}: "), ( + f"the conflict's own ValueError must reach the caller, got: {snapshot.escaped}" + ) + assert CONFLICT_TEXT in snapshot.escaped + assert snapshot.identified_names == () + + def test_conflict_is_not_dropped_when_a_rival_claims_the_name(self) -> None: + """A rival matching the same name must not swallow the misconfiguration and win in its place.""" + snapshot = _evaluate_conflict(with_rival=True) + + assert snapshot.identified_names != (RIVAL_CLASS_NAME,), ( + "the rival must not silently win while the two-readers conflict is swallowed" + ) + assert snapshot.escaped is not None, "a rival candidate must not hide the two-readers conflict" + assert snapshot.escaped.startswith(f"{RAISE_TYPE_NAME}: ") + assert CONFLICT_TEXT in snapshot.escaped diff --git a/tests/test_core/test_prepare/test_raising_matcher_containment.py b/tests/test_core/test_prepare/test_raising_matcher_containment.py new file mode 100644 index 000000000..97ec63667 --- /dev/null +++ b/tests/test_core/test_prepare/test_raising_matcher_containment.py @@ -0,0 +1,255 @@ +"""Red-phase pins for issue #845 (part 2): a raising ``match_feature_group_criteria`` must be contained. + +A raising matcher must be skipped so every other feature still resolves, and on the broken class's own +feature name it is recorded as a ``matcher_error`` near-miss whose reason is plain text, so no retained +traceback pins the class. The build-phase fail-closed contract (#790) is out of scope; it is pinned in +tests/test_core/test_api/test_sbdg_resolve_feature_broken_rule.py. Doubles are dropped per test. +""" + +import gc +from collections.abc import Callable +from dataclasses import dataclass +from functools import partial +from typing import Optional, TypeVar + +from mloda.core.abstract_plugins.components.data_access_collection import DataAccessCollection +from mloda.core.abstract_plugins.components.feature import Feature +from mloda.core.abstract_plugins.components.feature_name import FeatureName +from mloda.core.abstract_plugins.components.options import Options +from mloda.core.abstract_plugins.compute_framework import ComputeFramework +from mloda.core.abstract_plugins.feature_group import FeatureGroup +from mloda.core.prepare.accessible_plugins import FeatureGroupEnvironmentMapping +from mloda.core.prepare.identify_feature_group import ( + IdentifyFeatureGroupClass, + render_resolution_failure, +) +from mloda.steward import resolve_feature + + +RAISE_MESSAGE = "boom_845_matcher_exploded" +RAISE_TYPE_NAME = "RuntimeError" +BROKEN_CLASS_NAME = "RaisingMatcherFG845" +BROKEN_OWN_FEATURE = "raising_matcher_own_feat_845" +NEIGHBOR_CLASS_NAME = "ContainedNeighborFG845" +NEIGHBOR_FEATURE = "contained_neighbor_feat_845" +# Deliberately dissimilar, so no "Did you mean" suggestion can name the broken class. +UNRELATED_FEATURE = "zqx_no_group_owns_this_845" +MATCHER_ERROR_STAGE = "matcher_error" + +T = TypeVar("T") + + +class ContainedFw845(ComputeFramework): + """Dummy compute framework for the raising-matcher containment tests.""" + + +class ContainedNeighborFG845(FeatureGroup): + """Inert, resolvable neighbour: it must keep resolving while a broken candidate exists.""" + + @classmethod + def feature_names_supported(cls) -> set[str]: + return {NEIGHBOR_FEATURE} + + @classmethod + def compute_framework_rule(cls) -> set[type[ComputeFramework]] | None: + return {ContainedFw845} + + def input_features(self, options: Options, feature_name: FeatureName) -> Optional[set[Feature]]: + return None + + +def _capture(call: Callable[[], T]) -> tuple[Optional[T], Optional[str]]: + """Run call, returning (value, None) or (None, 'Type: message'). No traceback is retained.""" + try: + return call(), None + except Exception as exc: # noqa: BLE001 (red-phase probe: an escape is the fact under test) + return None, f"{type(exc).__name__}: {exc}" + + +def _make_raising_matcher_fg() -> type[FeatureGroup]: + """Build a candidate whose matcher raises, while still declaring its own feature name.""" + # Class objects are cyclic; collect leftovers from earlier tests before defining a twin. + gc.collect() + + class RaisingMatcherFG845(FeatureGroup): + """Declares one feature name, but its criteria hook raises for every request.""" + + @classmethod + def feature_names_supported(cls) -> set[str]: + return {BROKEN_OWN_FEATURE} + + @classmethod + def compute_framework_rule(cls) -> set[type[ComputeFramework]] | None: + return {ContainedFw845} + + @classmethod + def match_feature_group_criteria( + cls, + feature_name: FeatureName | str, + options: Options, + data_access_collection: Optional[DataAccessCollection] = None, + ) -> bool: + raise RuntimeError(RAISE_MESSAGE) + + def input_features(self, options: Options, feature_name: FeatureName) -> Optional[set[Feature]]: + return None + + return RaisingMatcherFG845 + + +@dataclass(frozen=True) +class _OwnFeatureSnapshot: + """Plain-data readout of one own-feature evaluation. Holds no class and no exception object.""" + + escaped: Optional[str] + failure_kind: Optional[str] + eliminated_names: tuple[str, ...] + stage: Optional[str] + reason: Optional[str] + reason_type: Optional[str] + message: Optional[str] + + +def _evaluate_own_feature() -> _OwnFeatureSnapshot: + """Evaluate the broken class's OWN feature name and read the result out as plain data.""" + broken_fg = _make_raising_matcher_fg() + try: + feature = Feature(BROKEN_OWN_FEATURE) + plugins: FeatureGroupEnvironmentMapping = {broken_fg: {ContainedFw845}} + result, escaped = _capture(partial(IdentifyFeatureGroupClass.evaluate, feature, plugins, None)) + if result is None: + return _OwnFeatureSnapshot(escaped, None, (), None, None, None, None) + + elimination = result.eliminations.get(broken_fg) + message, render_escaped = _capture(partial(render_resolution_failure, result, feature)) + snapshot = _OwnFeatureSnapshot( + escaped=render_escaped, + failure_kind=result.failure_kind, + eliminated_names=tuple(sorted(fg.get_class_name() for fg in result.eliminations)), + stage=None if elimination is None else str(elimination.stage), + reason=None if elimination is None else str(elimination.reason), + reason_type=None if elimination is None else type(elimination.reason).__name__, + message=message, + ) + del elimination + del result + del plugins + return snapshot + finally: + del broken_fg + gc.collect() + + +class TestRaisingMatcherContainment: + """A raising matcher is a contained non-match, recorded as a matcher_error near-miss on its own feature.""" + + def test_unrelated_feature_is_not_poisoned(self) -> None: + """resolve_feature of an unrelated name reports the ordinary no-match, never the bare raise.""" + broken_fg = _make_raising_matcher_fg() + try: + result = resolve_feature(UNRELATED_FEATURE) + winner_name = result.feature_group.get_class_name() if result.feature_group is not None else None + error = result.error + candidate_names = [candidate.get_class_name() for candidate in result.candidates] + del result + finally: + del broken_fg + gc.collect() + + assert winner_name is None + assert candidate_names == [] + assert error is not None + # The ordinary no-match, not the bare raise. The broken candidate may still appear as a near-miss. + assert error != RAISE_MESSAGE + assert error.startswith(f"No feature groups found for feature name: '{UNRELATED_FEATURE}'.") + + def test_resolvable_neighbour_still_resolves(self) -> None: + """A resolvable unrelated feature still wins its own group while the broken class exists.""" + broken_fg = _make_raising_matcher_fg() + try: + result = resolve_feature(NEIGHBOR_FEATURE) + winner_name = result.feature_group.get_class_name() if result.feature_group is not None else None + error = result.error + del result + finally: + del broken_fg + gc.collect() + + assert error is None + assert winner_name == NEIGHBOR_CLASS_NAME + + def test_evaluate_skips_the_raising_candidate(self) -> None: + """The seam does not propagate the raise: the broken candidate is skipped, the neighbour wins.""" + broken_fg = _make_raising_matcher_fg() + try: + feature = Feature(NEIGHBOR_FEATURE) + plugins: FeatureGroupEnvironmentMapping = { + broken_fg: {ContainedFw845}, + ContainedNeighborFG845: {ContainedFw845}, + } + result, escaped = _capture(partial(IdentifyFeatureGroupClass.evaluate, feature, plugins, None)) + identified_names = [] if result is None else sorted(fg.get_class_name() for fg in result.identified) + matched_names = [] if result is None else sorted(fg.get_class_name() for fg in result.criteria_matched) + del result + del plugins + finally: + del broken_fg + gc.collect() + + assert escaped is None + assert identified_names == [NEIGHBOR_CLASS_NAME] + assert matched_names == [NEIGHBOR_CLASS_NAME] + assert BROKEN_CLASS_NAME not in identified_names + assert BROKEN_CLASS_NAME not in matched_names + + def test_own_feature_records_a_matcher_error_elimination(self) -> None: + """Requesting the broken class's own name records it as a matcher_error near-miss.""" + snapshot = _evaluate_own_feature() + + assert snapshot.escaped is None + assert snapshot.failure_kind == "none" + assert snapshot.eliminated_names == (BROKEN_CLASS_NAME,) + assert snapshot.stage == MATCHER_ERROR_STAGE + assert snapshot.reason is not None + assert RAISE_TYPE_NAME in snapshot.reason + assert RAISE_MESSAGE in snapshot.reason + + def test_near_miss_block_names_the_broken_class(self) -> None: + """render_resolution_failure names the broken class, its exception type and its message.""" + snapshot = _evaluate_own_feature() + + assert snapshot.escaped is None + assert snapshot.message is not None + message = snapshot.message + assert f"Feature group(s) eliminated while matching '{BROKEN_OWN_FEATURE}':" in message + bullets = [line for line in message.split("\n") if line.startswith(f" - {BROKEN_CLASS_NAME} (")] + assert len(bullets) == 1 + assert RAISE_TYPE_NAME in bullets[0] + assert RAISE_MESSAGE in bullets[0] + + def test_recorded_reason_is_a_plain_string(self) -> None: + """The contained raise is recorded as text: no exception object (and no traceback) is retained.""" + snapshot = _evaluate_own_feature() + + assert snapshot.escaped is None + assert snapshot.stage == MATCHER_ERROR_STAGE + assert snapshot.reason_type == "str" + + def test_resolve_feature_reports_the_matcher_error_for_the_own_feature(self) -> None: + """The debug path agrees with the seam: same near-miss, not a bare provider crash message.""" + broken_fg = _make_raising_matcher_fg() + try: + result = resolve_feature(BROKEN_OWN_FEATURE) + winner_name = result.feature_group.get_class_name() if result.feature_group is not None else None + error = result.error + del result + finally: + del broken_fg + gc.collect() + + assert winner_name is None + assert error is not None + assert error.startswith(f"No feature groups found for feature name: '{BROKEN_OWN_FEATURE}'.") + assert BROKEN_CLASS_NAME in error + assert RAISE_TYPE_NAME in error + assert RAISE_MESSAGE in error diff --git a/tests/test_gc_freeze_contract.py b/tests/test_gc_freeze_contract.py index af23c4463..69ad47e35 100644 --- a/tests/test_gc_freeze_contract.py +++ b/tests/test_gc_freeze_contract.py @@ -1,8 +1,8 @@ """Pin the gc.freeze contract that keeps the suite's per-test gc.collect() calls off the hot path. -26 test modules call gc.collect() (13 from an autouse fixture, the rest inline in a test body or a finally -block), because a throwaway FeatureGroup subclass defined in a test body lingers in -FeatureGroup.__subclasses__() until a GC pass reclaims its reference cycle. Each collect rescans the whole +The suite calls gc.collect() constantly (from the shared autouse isolation fixture in tests/conftest.py, and +inline in test bodies and finally blocks), because a throwaway FeatureGroup subclass defined in a test body +lingers in FeatureGroup.__subclasses__() until a GC pass reclaims its cycle. Each collect rescans the whole imported heap (pandas, polars, duckdb, pyarrow, sklearn, every plugin and test module), so one collect costs hundreds of milliseconds and gc dominates the suite's wall time. Freezing the imported graph into the permanent generation once collection has finished makes those collects free without weakening the isolation @@ -17,7 +17,6 @@ import subprocess # nosec B404 import sys from pathlib import Path -from typing import Any import pytest @@ -94,16 +93,7 @@ _HOST_HOOK_IDS = ["sessionfinish_only", "collection_finish_then_sessionfinish"] -@pytest.fixture(autouse=True) -def _no_feature_group_registry_pollution() -> Any: - """Guarantee this module never leaks its throwaway FeatureGroup subclass (see the filter test's twin fixture).""" - yield - gc.collect() - gc.collect() - leaked = [c for c in get_all_subclasses(FeatureGroup) if c.__module__ == __name__] - assert not leaked, f"Leaked FeatureGroup subclasses from {__name__}: {[c.__name__ for c in leaked]}" - - +# No local isolation fixture: the shared autouse one in tests/conftest.py already covers this module (#845). def _define_throwaway_feature_group() -> type[FeatureGroup]: """Define a FeatureGroup subclass inside a function body, exactly as the suite's throwaway probes do.""" diff --git a/tests/test_registry_isolation.py b/tests/test_registry_isolation.py new file mode 100644 index 000000000..a689eb5b6 --- /dev/null +++ b/tests/test_registry_isolation.py @@ -0,0 +1,142 @@ +"""Pin the shared FeatureGroup registry-isolation mechanism (#845, part 1). + +The mitigation must stay ONE mechanism: ``tests.registry_isolation.reclaim_leaked_feature_groups`` plus +one autouse fixture in ``tests/conftest.py``, so every test module is isolated and no module carries a copy. +""" + +from __future__ import annotations + +import gc +from pathlib import Path + +import pytest + +from mloda.core.abstract_plugins.components.utils import get_all_subclasses +from mloda.core.abstract_plugins.feature_group import FeatureGroup + +from tests import registry_isolation_probe +from tests.registry_isolation import reclaim_leaked_feature_groups + + +TESTS_ROOT = Path(__file__).resolve().parent + +PROBE_MODULE = registry_isolation_probe.__name__ + +FIXTURE_NAME = "_no_feature_group_registry_pollution" +# Assembled rather than written out, so this module is never itself a hit of the pattern it scans for. +FIXTURE_DEF = f"def {FIXTURE_NAME}" + + +def _registered_names() -> set[str]: + """Names (never class objects, which would pin them) of this module's registered FeatureGroup subclasses.""" + return {c.__name__ for c in get_all_subclasses(FeatureGroup) if c.__module__ == __name__} + + +def _registered_names_of(module_name: str) -> set[str]: + """Names (never class objects, which would pin them) of the registered subclasses from one module.""" + return {c.__name__ for c in get_all_subclasses(FeatureGroup) if c.__module__ == module_name} + + +def _define_throwaway_subclass() -> str: + """Define a FeatureGroup subclass and return only its name; returning the class would pin it.""" + + class ThrowawayRegistryProbe845FeatureGroup(FeatureGroup): + pass + + return ThrowawayRegistryProbe845FeatureGroup.__name__ + + +def _define_leaked_subclass() -> type[FeatureGroup]: + """Define a FeatureGroup subclass and return it, so the caller holds a strong reference: a genuine leak.""" + + class LeakedRegistryProbe845FeatureGroup(FeatureGroup): + pass + + return LeakedRegistryProbe845FeatureGroup + + +class TestReclaimLeakedFeatureGroups: + """reclaim_leaked_feature_groups(before, module_name) is the one reclaim-and-report mechanism.""" + + def test_reclaims_a_throwaway_subclass(self) -> None: + """A transient subclass is registered, then reclaimed. One test, not two: xdist could split a pair.""" + before = get_all_subclasses(FeatureGroup) + name = _define_throwaway_subclass() + assert name in _registered_names(), "the probe never registered; the reclaim assertion would prove nothing" + assert reclaim_leaked_feature_groups(before, __name__) == [] + assert name not in _registered_names(), f"{name} survived the reclaim" + + def test_reports_a_genuine_leak(self) -> None: + """A strongly referenced subclass is reported, so the conftest fixture fails loudly instead of hiding it.""" + before = get_all_subclasses(FeatureGroup) + leaked_cls = _define_leaked_subclass() + reported = [c.__name__ for c in reclaim_leaked_feature_groups(before, __name__)] + expected = leaked_cls.__name__ + del leaked_cls # drop the strong reference before asserting, so a failure leaves nothing behind + gc.collect() + gc.collect() + assert reported == [expected], f"a strongly referenced subclass must be reported, got {reported}" + assert expected not in _registered_names(), "the deliberate leak must not outlive this test" + + def test_reclaims_a_subclass_owned_by_another_module(self) -> None: + """A helper-made subclass is collected too; only the RETURN value stays filtered to module_name. + + Generational GC is paused for the window, so the reclaim under test is the only collection that runs. + """ + gc.disable() + try: + before = get_all_subclasses(FeatureGroup) + name = registry_isolation_probe.define_helper_subclass() + registered_before = name in _registered_names_of(PROBE_MODULE) + reported = [c.__name__ for c in reclaim_leaked_feature_groups(before, __name__)] + registered_after = name in _registered_names_of(PROBE_MODULE) + finally: + gc.enable() + gc.collect() # never leave the probe behind for the next test on this worker + + assert registered_before, "the probe never registered; the reclaim assertion would prove nothing" + assert reported == [], f"a class this module does not own must not be reported, got {reported}" + assert not registered_after, f"{name} survived the reclaim: an unowned class must still be collected" + + def test_no_new_subclasses_reports_nothing(self) -> None: + """The cheap path: nothing appeared since the snapshot, so nothing is reported and no collection is needed.""" + before = get_all_subclasses(FeatureGroup) + assert reclaim_leaked_feature_groups(before, __name__) == [] + + +class TestIsolationFixtureIsGlobal: + """The isolation fixture is autouse in the root conftest, so a new test module inherits it for free.""" + + def test_fixture_reaches_every_test(self, request: pytest.FixtureRequest) -> None: + """This module declares no such fixture, so seeing it here proves tests/conftest.py supplies it.""" + assert FIXTURE_NAME in request.fixturenames, ( + f"{FIXTURE_NAME} must be an autouse fixture in tests/conftest.py so every test module is isolated" + ) + + +def _fixture_definitions() -> dict[Path, int]: + """Every file under tests/ that defines the fixture, mapped to its definition count. + + Only conftest.py and test_*.py: a fixture is collected from nowhere else. + """ + found: dict[Path, int] = {} + for py_file in sorted({*TESTS_ROOT.rglob("conftest.py"), *TESTS_ROOT.rglob("test_*.py")}): + if "__pycache__" in py_file.parts: + continue + count = py_file.read_text(encoding="utf-8").count(FIXTURE_DEF) + if count: + found[py_file] = count + return found + + +class TestFixtureIsNotCopied: + """The per-module copies stay deleted; the fixture lives in exactly one place.""" + + def test_defined_exactly_once_in_root_conftest(self) -> None: + """Exactly one definition of the fixture exists in the tests tree, and it is in tests/conftest.py.""" + found = _fixture_definitions() + locations = {str(p.relative_to(TESTS_ROOT)): n for p, n in found.items()} + assert sum(found.values()) == 1, f"{FIXTURE_NAME} must be defined exactly once, found {locations}" + assert list(found) == [TESTS_ROOT / "conftest.py"], ( + f"the single definition must live in tests/conftest.py, found {locations}" + )