Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/docs/in_depth/feature-chain-parser.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
4 changes: 3 additions & 1 deletion docs/docs/in_depth/feature-group-matching.md
Original file line number Diff line number Diff line change
Expand Up @@ -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).

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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__)

Expand Down Expand Up @@ -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
Expand All @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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__)

Expand All @@ -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."""
Expand Down Expand Up @@ -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.",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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__)

Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand All @@ -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:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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.")
Expand Down Expand Up @@ -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))

Expand Down Expand Up @@ -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__}."
)
Expand Down Expand Up @@ -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))
Expand All @@ -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]]:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down
30 changes: 30 additions & 0 deletions mloda/core/abstract_plugins/components/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -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],
Expand Down
1 change: 1 addition & 0 deletions mloda/core/filter/global_filter.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
)
Expand Down
Loading
Loading