refactor: adopt property_spec() for all PROPERTY_MAPPING specs - #87
Merged
TomKaltofen merged 3 commits intoJul 13, 2026
Merged
Conversation
Closes mloda-ai#84. All 259 specs across 66 files move from hand-written dicts to the property_spec() builder, so spec invariants are validated at import and every option carries a real explanation for declared_option_keys() and the docs. The builder defaults context=True while the parser reads a missing context as False (_is_context_parameter). 87 specs omitted context and were therefore group options: they get an explicit context=False so their resolution and hashing are unchanged. Verified by snapshotting every FeatureGroup's effective spec (allowed_values, default, effective context, effective strict, validators) before and after: the only delta across all 259 is the added explanation. The 54 specs that carried no explanation (their text lived inside the value dict) collapse to 13 distinct option keys, since a base and its concretes share an option and differ only in the value space they allow. Each key gets one explanation, used identically by the base and its concretes.
Add a repo-wide guard that connector options stay group and every other family stays context. The builder defaults context=True while mloda reads a missing context as group, so a spec authored without an explicit context=False would silently flip a connector option and change resolution and hashing. Nothing locked that before. Reuse the base wording for llm_method and retrieval_method: these were the only two keys where the base already had an explanation and the concrete did not, so the concrete had picked up freshly authored text that disagreed with its base.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #84.
All 259
PROPERTY_MAPPINGspecs across 66 files move from hand-written dicts to mloda 0.10.0'sproperty_spec()builder. Spec invariants are now validated at import, and every option carries a real explanation, which is whatdeclared_option_keys()and the docs surface to users.The trap this had to avoid
property_spec()defaultscontext=Trueand always writes the key. mloda's parser reads a missingcontextas False (FeatureChainParser._is_context_parameter). 87 specs (every connector spec) omittedcontextand were therefore group options. Migrating them naively would have silently flipped them to context, changing feature-group resolution and hashing.Those 87 sites get an explicit
context=False. Thecontext=Truecount is 172 before and 172 after.How behavior preservation was verified
Every FeatureGroup's effective spec was snapshotted before and after (
allowed_values,default, effectivecontext, effectivestrict_validation, validators), normalizing each absent key to the parser's own fallback.Result across 67 classes and 259 specs: zero semantic differences. The only delta is the added
explanationstrings.The 54 authored explanations
54 specs carried no
explanation(their text lived inside the value dict, e.g.{"sentence": "Sentence-boundary aware chunks"}). These collapse to 13 distinct option keys, because a base and its concretes share an option and differ only in the value space they allow. Each key gets one explanation, used identically by the base and its concretes. The per-value descriptions still live inallowed_values, so nothing was lost.Also
test_group_context_split_is_stablepins connector options as group and every other family as context. Nothing locked this before, and it is the single highest-risk failure mode of this refactor. Mutation-checked: it fails if an explicitcontext=Falseis dropped.demo.ipynbtaught the old hand-written dict form under "swap in your own embedder"; it now teachesproperty_spec.Verification
toxgreen: 706 passed, 7 skipped,ruff format,ruff check,mypy --strict(241 files), bandit.Reviewed independently by two deep reviews (Claude Opus and codex); both confirmed no behavior change. Findings on import ordering and on two leaf explanations that disagreed with their base are fixed in 04d3d64.
Note for reviewers
The registry guide
feature-group-patterns/11-options.mdis stale and was not used as the source of truth here. It still documents the retired flattened form as valid, and it claimsproperty_specacceptsvalidation_functionandtype_validator(both raiseTypeError; the real parameters areelement_validatorandmatch_guard). This PR was written against the verified 0.10.0 signature. Worth a separate upstream fix.