feat: support irspack's feature-aware iALS - #148
Open
marevol wants to merge 5 commits into
Open
Conversation
Add support for irspack 0.5.0's feature-aware iALS. An optional `features:` block declares item- and/or user-side attribute tables; recotem encodes them into numeric matrices, feeds them to IALSRecommender through Optuna search and the final refit, persists the encoder state in the signed artifact, and uses it at serve time to recommend for cold-start users and for seed items absent from training. Highlights: - Feature tables come from the existing datasource registry (csv / parquet / bigquery / sql / plugins); the block's presence enables feature-aware training with no separate flag. - `features.*.source` reuses the same path-scheme allow-list, chained-scheme rejection, embedded-credential rejection, sha256-for-network rule, and plugin no_expand_fields as the top-level `source`. - Cold start reaches irspack's feature API on `:recommend` and `:recommend-related` (features only; features plus ad-hoc history; and a seed item absent from training). - The encoder state is persisted as plain Python containers only, so the artifact FQCN allow-list is unchanged; a new `features` header descriptor gates the payload shape and fails closed on unknown / newer / malformed versions. - Axis alignment between the search phase and the final refit is enforced structurally (encode requires an explicit row order); partial feature injection is refused with an unconditional raise. - RECOTEM_MAX_FEATURE_DIM caps the encoded dimension per side before the cubic solve. Robustness and observability: - Refuse an all-dead feature block (including an all-dead-numerical block, which a plain n_features check missed) so an artifact never advertises features while serving bias-only. - Count a finite-but-huge cold-start numerical value instead of injecting float32 inf. - Cap cold-start feature request value length (422 on violation). - Map an unrepresentable numerical column to a training-domain error (exit 4) and reject a complex-valued column explicitly. - Reject at recipe load a features id_column that also names a feature column. Includes docs, an examples/feature-aware/ walkthrough, and unit / integration / fuzz tests.
marevol
force-pushed
the
feat/feature-aware-ials
branch
from
July 19, 2026 01:37
12fc68e to
b6f8215
Compare
Restore the `isinstance(raw_source, dict)` guard in `load_recipe` so an absent `source:` key stays absent and pydantic emits an aggregatable "field required" error, instead of being coerced to `None` — which routed the check to a `mode="after"` validator that pydantic v2 skips whenever another field already errored. A recipe missing `source:` plus any other schema error now surfaces both problems in one validation round again (regression from the source-resolution refactor on this branch). Tests: - regression test for the combined missing-`source:` + other-error case - `::`-chain rejection test for `features.<side>.source` (parity with the existing top-level source-path validator) - strengthen the feature-aware serve roundtrip with a Case B differential guard (`:recommend-related` known seed + `user_features`), mirroring the Case A/C young-vs-old assertions so a feature-blind regression cannot pass on a bare 200 Also document that feature matrices are shared read-only across parallel Optuna trials (safe in irspack 0.5.0; do not add a per-trial copy).
… block The whole-block-dead refusal in `_fetch_side` keyed on `_spec_is_live`, which used width for categorical/multi_label specs. A constant-but-present column (non-empty vocab, width > 0, but the same one-hot on every row) is collinear with the always-1 bias and carries no signal, yet passed the refusal -- signing an artifact advertising `features` for what is really plain iALS. This was inconsistent with both the per-column dead-column warning (varies-based) and the numerical branch (std-based), which already treat a constant column as dead. Replace the width-based check with a shared `spec_is_live(series, spec)` helper that keys on the same "does the encoded block vary across rows" signal, so the refusal now agrees with the warning and the numerical zero-variance refusal. A single dead column among live ones is still allowed (min_frequency pruning is legitimate); only an entirely-dead block is refused. Also correct three documentation inaccuracies found in review: - security.md: `numpy.dtype` is admitted via its explicit FQCN entry, not the `numpy.*` module-prefix list (only `numpy._core.multiarray.scalar` is). - recipe-reference.md: the 8192-char feature-value cap surfaces on the batch verbs as a per-element VALIDATION_ERROR inside the 200 response, not a whole-batch 422. - recipe-reference.md: document that the id-column dtype-pinning caveat also applies to a categorical value column (a float64-inferred column trains its vocabulary as "1990.0" and silently counts a request's "1990" as unknown). Tests: add constant categorical / multi_label refusal cases plus varying and mixed controls; update two pre-existing tests that encoded the old behavior.
…esults Follow-up fixes from a multi-agent review of the feature-aware iALS branch. - A non-finite numerical value no longer silently drops a usable column. pd.to_numeric maps an overflow token like "1e400" to +inf, and pandas mean/std do not skip +-inf, so one such cell made the column's std non-finite and routed the whole column to the zero-variance path -- silently dropping a column that still held usable finite values while the artifact kept advertising features, with a warning that misattributed the cause as "divide by zero". build_encoder_state now computes mean/std over the finite values only (consistent with encode(), which already routes a per-row non-finite value to unknown); a column with no finite value at all is still dropped, now with a distinct, accurate warning. - :recommend-related cold-start paths now return 404 NO_CANDIDATES consistently. The all-seeds-known path already raised NO_CANDIDATES on an empty ranker result, but the user_features and item_features branches returned 200 with an empty items list for the identical condition; both now raise the same error, single and batch alike. - Correct the _features.py design comment: the float() coercions on the numerical mean/std are load-bearing plainness guards alongside the vocabulary str() coercions (a np.float64 would otherwise leak into the persisted encoder state), so neither may be refactored away. - Document the new [source] / [features.<side>.source] tags in recotem validate output in the changelog. Full suite: 2154 passed, 1 skipped; ruff and ruff format clean.
Address second-round review findings: - Add BodySizeLimitMiddleware bounding each HTTP request body at RECOTEM_MAX_BODY_BYTES (default 128 MiB, clamped [1 MiB, 2 GiB]). Over-cap requests get 413 PAYLOAD_TOO_LARGE before the body is buffered/parsed, enforced on both a declared Content-Length and chunked bodies with no length header. - Cap cold-start feature-dict key lengths at 1-256 chars: user_features column-name keys, item_features outer seed-id keys (now typed _ItemStr), and nested per-seed keys. Previously only string values were length-capped and max_length bounded the key count only, leaving key length unbounded. Over-length keys 422 reporting only the length, never the key text. - Docs: README gains the feature-aware iALS / cold-start bullets; api-reference documents the silently-ignored unknown feature key, the 413 code, and all length/size bounds; recipe-reference extends the dtype-trap note to value columns with source-side type guidance; operations/security/CLAUDE.md gain the env-var and threat-model entries. - Tests: fuzz corpus now exercises the features: block (seed YAML, column-field mutations, arbitrary-value injection at the features key); a 0-row feature table is pinned to feature_table_error; new body-cap (Content-Length, chunked, boundary) and key-length tests.
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.
Adds support for irspack 0.5.0's feature-aware iALS. A new optional
features:recipe block declares item- and/or user-side attribute tables; recotem encodes them into numeric matrices, feeds them toIALSRecommenderthrough Optuna search and the final refit, persists the encoder state in the signed artifact, and uses it at serve time to recommend for users and seed items absent from training.What it looks like
The block's presence enables feature-aware training — no separate flag. Feature tables come from the existing datasource registry (
csv/parquet/bigquery/sql/ plugins), which turned out to be role-agnostic:FetchContextcarries no interaction semantics.Cold start reaches irspack's feature API on all four verbs:
:recommend+user_features:recommend-related+user_features:recommend-related+item_featuresWorking example under
examples/feature-aware/.Notable design decisions
Feature-aware iALS is not a new irspack class — it is
IALSRecommenderwith extra kwargs, sobest_classcannot distinguish it. The existing 0.4↔0.5 skew guard already covers it: theIALSModelConfig.__setstate__7→10 arity change it refuses is this feature.lambda_*_featureis tuned by Optuna (5e-2–1e6, log — the range upstream's own example exercises). irspack ships no default range and its constructor default of0.0is a hard error whenever features are present. This is the first exception to "hyperparameter ranges come from irspack";docs/recipe-reference.mdis amended accordingly. Note the range's upper bound is not a verified features-off bound — for a true on/off comparison, run two recipes.The artifact FQCN allow-list is unchanged. The encoder state is plain Python containers only —
dict/list/str/int/float, no numpy, no classes, no sklearn estimators, no pandas objects — specifically soartifact/signing.pynever widens (docs/operations.mddocuments sklearn as a known unguarded unpickle axis).git diff main -- src/recotem/artifact/signing.pyis empty. Thestr()coercions on vocabulary keys are load-bearing: they are the only thing keeping numpy scalars out of the state. Note the allow-list does not back them up —numpy.str_pickles vianumpy._core.multiarray.scalar+numpy.dtype, both allow-listed, so it would load if it ever leaked in (harmlessly, since it is astrsubclass, but the state would no longer be plain).pandas.Indexis genuinely refused. (Corrected from an earlier claim thatnumpy.str_was not allow-listed.)A new
featuresheader descriptor doubles as the payload-shape gate recotem lacked.FORMAT_VERSIONdescribes the container only andrecotem_versionis never gated at serve time. Unknown/newer/malformed versions fail closed — a changed state shape would encode request features into the wrong vector space and return silently incorrect recommendations. A pre-feature serve stays ungated and needs no gating: it has no feature code, never reads the state, and its known-user recommendations remain correct.Two hazards addressed structurally, not by care
Axis alignment. The search phase orders items by
list(set(...))— unsorted, and for string ids not stable across processes — while the final refit usespd.Categorical(sorted). irspack raises on a row-count mismatch but accepts a misordered matrix silently: it trains and the Evaluator scores it with no error.encode()therefore takesindex_orderas a required argument and always reindexes; there is no API returning a matrix without naming its row order, and no caching. A standing in-suite guard re-runs the alignment test in subprocesses under three fixedPYTHONHASHSEEDvalues.Partial feature injection.
lambda_*_featurewithout a matrix trains silently as plain iALS.search.pyhad two construction sites (theper_trial_timeout_secondspath and the default path); both now route through a single_constructhelper whose asserts turn irspack's silent asymmetry into a loud one.Compatibility
Verified with real signed artifacts through the real write/read/unpickle path:
__setstate__absorbs unknown keys; recommend unaffectedfeaturesheader → old codeNoneload_recipeRecipeError: features: Extra inputs are not permittedextra="forbid"): 422 on the single verbs; on the batch verbs, 200 with a per-elementVALIDATION_ERROR(batch elements are validated per-element by design)A non-feature artifact's header has the same key set as
main's — thefeatureskey is absent, notnull— so an older serve loads it fine. (It is not byte-identical:recipe_hashis a header field and every recipe's hash changes, per the note below.)Security
features.*.sourcegets the same path-scheme allow-list,::-chain rejection, embedded-credential rejection, and sha256-for-network rule assource, through the same shared helpers. Plugin-declaredno_expand_fieldsreaches feature subtrees, soSQLSource'sdsn_envkeeps its env-expansion protection.user_featurescarries PII by construction. Raw values are never logged (column names and counts only);log_redactionalso redacts the keys as a mechanical backstop.Known limits
min_frequencyis the only recipe-level lever; cost is cubic in the dimension and multiplies withtraining.parallelism. Sized indocs/operations.md.numericalvalue degrades silently and is not counted. Clamping the standardized magnitude was deliberately deferred — it is a modelling decision, not a bugfix. A value extreme enough to break the solver returns400 FEATURE_VALUE_UNUSABLE.get_score_from_item_features) is a non-goal — it spans a different score axis and needs its own verb.Every recipe's
recipe_hashchangesfeaturesis a new optional field and_compute_recipe_hashdumps withoutexclude_none, so existing recipes now serialize{"features": null}. Nothing compares or gates onrecipe_hash, but it is emitted for SIEM consumption, so operators pinning it will see a spurious change. Noted inCHANGELOG.md.Pre-existing bugs found, left out of scope
time_globalsilently drops a user:1 - 0.3 - 0.0 == 0.7exactly but1 - 0.7 == 0.30000000000000004, so the ratio lands at0.9999999999999998andint()truncates a user into thetestsplit, which recotem never reads. Verified features still align correctly under it.datasource/csv.py's_validate_required_columnsis dead in production — the soleFetchContextconstruction passes noextra, so it always returns early.Verification
pytest tests2120 passed, 1 skipped;pytest -m slow4 passed;tests/e2e/run.shpasses; ruff + ruff format clean.Review fixes (commit
adad2b6)A multi-reviewer audit — followed by a second audit of the fixes themselves — found several defects, all addressed in the follow-up commit on this branch:
Correctness (client-facing):
≥309-digit JSON integer) raisedOverflowErrorpastexcept (TypeError, ValueError)and returned an uncaught HTTP 500; it now routes to the counted-unknown path (200).encode/encode_onenow mirror the fitting parser (pd.to_numeric) across the whole value domain, so a value the fit dropped — underscored, non-ASCII,bytes/bytearray/Fraction— can no longer be encoded against statistics it never contributed to (it silently encoded as a large-sigma outlier before).400s now set explicit status labels, so client errors are no longer counted asstatus="error"(which pages on-call at the documented 10% threshold).Silent-failure guards (training):
featuresfor what is really plain iALS. (The dtype trap: one blank cell makes pandas inferfloat64, so1reads back as1.0and never matches the axis's"1".)raise— the originalasserts were stripped under-O. (So the "single_constructhelper whose asserts…" note above is now a raise, not an assert.)exclude_itemsis unified: the cold-start verbs post-filter like every pre-existing verb, instead of back-filling to a full page (which changed the returned item count whenuser_featureswas present).Observability / limits:
Docs/comments were corrected to match the code — including the three claims above (encoder-state contents, the
numpy.str_allow-list behavior, and the header key-set-vs-byte-identical wording), plus theindex_orderinvariant (requiring the argument makes omission unrepresentable, not misalignment), the near-constant numerical floor, and the newfeature_axis_errorexit-4 runbook entry.Out of scope (pre-existing, flagged for follow-up):
training/lock.py's-O-strippable assert; ruff not selecting theSruleset (so# noqa: S101is decorative); afeatures:recipe pulling optuna intorecotem serve(same asmain'sper_algorithm_trials); a broken anchor link indocs/plugin-authoring.md.Review fixes, second round (commit
0a8313a)A follow-up multi-agent audit (spec-completeness, correctness/regression, security, test-coverage, plus an adversarial pass) confirmed the substantive claims above and surfaced one MAJOR gap plus doc/test polish, addressed in this commit:
Security (MAJOR):
user_featureskeys,item_featuresouter seed-id keys typed_ItemStr, nested per-seed keys), and a newBodySizeLimitMiddlewarecaps the whole body atRECOTEM_MAX_BODY_BYTES(default 128 MiB — clears the ~72 MiB legitimate worst case; clamped [1 MiB, 2 GiB]) with413 PAYLOAD_TOO_LARGE, enforced on bothContent-Lengthand chunked bodies.Docs:
VALIDATION_ERRORinside a 200 on batch verbs. The dangling design-doc link was removed (the file was never committed).Tests:
features:-bearing seed YAML (mutations reach the new nested validation), plus arbitrary-value injection at thefeatures:key.feature_table_error(exit 4).Reviewed, deliberately unchanged: the
recipe_hashshift (disclosed above; nothing gates on it — freezing the hash schema is a separate decision), therecotem validatestdout wording (intentional), the bounded id samples in the zero-overlap error (deliberate diagnostic tradeoff), and the cold-seed cap counting warm feature-carrying seeds (errs strict, never loose).