Skip to content

Make the artifact a set of models, in the code and in the config - #12

Merged
Varfalamei merged 9 commits into
masterfrom
refactor/model-set-settings
Aug 26, 2026
Merged

Varfalamei merged 9 commits into
masterfrom
refactor/model-set-settings

Conversation

@Varfalamei

@Varfalamei Varfalamei commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Why

RecommenderALS was the whole feed. It owned a PopularModel for cold users, an optional RecommenderCoVis for live sessions, the blend weights, the four-way segment routing and the entire Strategy vocabulary. Its config declared popular, covis and blend — who serves cold users, whether there is a session layer, and how two lists are fused. None of that is a hyperparameter of ALS.

Two consequences: "add a model" meant editing ALS, and als_covis_youtravel read as "the ALS artifact with extras" rather than as what it is — a set of models behind one name.

What ships

The config keys by ROLE; the value's type picks the model.

ModelSetSettings(
    main=ALSSettings(...),                 # the main ranker
    fallback=PopularSettings(...),         # everyone the main ranker cannot represent
    realtime=CoVisSettings(...),           # optional, scores the live session
    blend=BlendSettings(MAIN_WEIGHT=1.0, REALTIME_WEIGHT=1.0, RRF_K=60),
)

RankerSettings = Union[ALSSettings, EASESettings, CoVisSettings, PopularSettings], and MODEL_FOR_SETTINGS is the single place mapping a settings type to its model class. So putting LightFM in the main role is main=LightFMSettings(...) — no schema change, no conditional anywhere, and adding a model is one registry entry plus the type in the union. ALSSettings is a leaf again: four ALS hyperparameters and the two common fields. Passing popular=/covis=/blend= to it is a validation error, with a test asserting it.

Field names were als/popular/covis in the first two commits of this branch — named after the models that happen to fill the roles today, which is the same category error as ALSSettings.covis one level down.

The code separates routing from scoring.

RecommenderModelSet   which member answers, and under what Strategy
members               how to score, and nothing else

RecommenderModelSet.recommend() is the only routing table in the package. A member knows its own algorithm: it answers can_serve(user) and returns candidates. It does not import a sibling, hold one as a field, read the set's config, or name a segment.

Members return (RecomItems, session_used) — a fact — and the set names the segment from it. My first version had members stamping their own strategy, and test_hot_user_ignores_history caught the lie: a hot user whose session seeds are all unknown to the models gets a plain ALS ranking, and labelling that model_realtime_hot_users silently corrupts every A/B readout split on strategy.

The role is realtime, the signal is session.

The role that consumes live events was called session while the strategies it emits are model_realtime_hot_users / model_realtime_warm_users — a published, append-only vocabulary mirrored by numeric id in api/docs/DEBUG_INFO_CODEC.md. Config and wire disagreed about the same thing. The rule is now explicit in CLAUDE.md §4: the signal is a session (history=, has_session, session_used — the API and Redis vocabulary), and the role, its weight and its strategy are realtime. BlendSettings.SESSION_WEIGHT would also have been the third distinct meaning of "session weight" in this package, next to CoVisSettings.COVIS_SESSION_WEIGHTS (a per-seed event multiplier) and the co-occurrence kernel's session_weight argument.

Strategy drops three members it could never emit: MODEL_HOT_AND_COLD_USERS, MODEL_ALS_COVIS_BLEND, MODEL_COVIS_SESSION (10 members to 7). The first lost its only emitter when RecommenderRandom was deleted; the other two briefly named algorithms instead of segments and were reverted in #11. Their ids stay reserved in DEBUG_INFO_CODEC.md forever — that table, not this enum, is the contract, and the consumer decodes historical records by id. Removing a ROW would break it; removing a MEMBER does not.

Manifest artifacts are declined, not deferred (48fb081). Three of the four problems a manifest.json would solve dissolve on their own, and the fourth is already answered by the trainer's log lines — against a runtime repack plus a loader change under the deploy ordering below. MODEL_SET_ROUTING.md records the routing-as-data design as deferred with its trigger conditions, so neither doc reads as a commitment nobody made.

Legacy artifacts: three hooks, shape only

Two generations exist in the buckets and both must serve. The old artifact is a RecommenderALS __dict__ carrying every member itself plus a flat or nested ALSSettings; the new one is a RecommenderModelSet __dict__ with main/fallback/realtime.

  • ALSSettings.__setstate__ — rebuilds sub-configs from flat fields on unpickle;
  • ModelSetSettings.from_legacy_als_settings — either legacy config shape into a set config;
  • RecommenderModelSet.from_legacy_als_state — a legacy ARTIFACT into a model set, member by member, so there is exactly one routing path in the library rather than a second copy kept alive for old pickles.

This is load-bearing, and its failure mode is silent. At serving time the only composition field read is blendfallback and realtime configs are read by train()/calc_metrics(), which always get a freshly built config. So a missing shim would not raise on a legacy artifact: the RRF fusion would run at the default 1.0/1.0/60 instead of the trained weights and quietly reorder the feed. test_legacy_artifact_blends_with_its_own_weights_not_defaults pins that, and test_legacy_artifact_loads_and_recommends_identically pins that a legacy artifact answers all four segments with identical items, scores and strategy — a member assigned from the wrong key raises nothing and just serves worse.

serving/model.py therefore resolves the class from the pickle's shape first and only then from the artifact name: the name cannot tell the generations apart, since als_covis_youtravel is a public query parameter and cannot change (§5.9). New: test_serving_loader.py stubs Triton's backend utils so the real _load_model — which decides what production loads and had no test — runs off-cluster.

Delete all three hooks once both artifacts have been retrained twice.

Renaming ModelSetSettings fields and BlendSettings weights was free only right now: no artifact in either bucket carries a session key or a role-shaped config (verified against both real pickles). After the first retrain each rename would have cost another legacy hook.

Verification

  • 109 tests pass, black clean. The flake8 gate CI actually blocks on (E9,F63,F7,F82) is clean; the advisory --exit-zero pass reports 9 warnings, the same 9 already present on master — this branch adds none.
  • Gate against real artifacts, old code vs new: both dev artifacts pulled from S3, loaded through the real TritonPythonModel._load_model, 14 request cases each — 28/28 identical items, scores and strategies. The runner pins BLAS to one thread the way app/src/settings.py does in production; without that, ALS dot products sum in a thread-dependent order and scores drift by ~1e-7, which reads as a diff while items and labels are unaffected.
  • Deployed to dev end to end: runtime repacked, trainer image 2026.08.25-dev.87, DAG tag bumped, retrained. The resulting artifact loads as a RecommenderModelSet and answers every route.

Deploy ordering

A code change under §5.11: upload runtime_env.tgz first, then bump the trainer image tag and retrain. Reversing it raises ImportError on model reload for the whole bucket, including artifacts that were not retrained. Done for dev; not yet for prod.

The API-side counterpart lives in youtravel-recsys MR !9 and is not part of this PR.

ALSSettings declared `popular`, `covis` and `blend`, i.e. who serves cold users,
whether there is a session layer, and how two rankers are fused - none of which
is a hyperparameter of ALS. Reading `ALSSettings.covis` invited exactly the wrong
mental model, and the class could not be reused as a plain leaf config.

An artifact is a model SET, so the set is now its own type:

    ModelSetSettings(
        als=ALSSettings(...),        # main ranker, hot users
        popular=PopularSettings(...) # fallback, cold users
        covis=CoVisSettings(...),    # session layer, optional
        blend=BlendSettings(...),    # fusion weights, read only with covis
    )

ALSSettings is a leaf again: four ALS hyperparameters and the two common fields.
Passing `popular=`/`covis=`/`blend=` to it is now a validation error, and there
is a test asserting that.

Why this is safe, and where the one shim is:

Three shapes of `recsys_config` exist in S3 - flat, nested-on-ALSSettings, and
now ModelSetSettings. `ALSSettings.__setstate__` already rebuilt sub-configs from
flat fields; it stays and now writes into names the class no longer declares,
which pydantic still resolves off __dict__. The new part is
`RecommenderALS.model_set`, which normalises either legacy shape into a
ModelSetSettings.

That property is not cosmetic. At serving time the ONLY composition field read is
`blend` (_recommend_hot_user_with_covis_blend); `popular` and `covis` are read by
train() and calc_metrics(), which always get a freshly built config. So a missing
shim would not raise on a legacy artifact - the RRF fusion would silently run at
default 1.0/1.0/60 instead of the trained weights and quietly reorder the feed.
test_legacy_artifact_blends_with_its_own_weights_not_defaults pins that, and the
existing end-to-end legacy-pickle test still asserts identical output.

Drop the legacy branch after both ALS artifacts have been retrained twice.

Docs: §5.1 of CLAUDE.md gains ModelSetSettings in the frozen-FQN list, §5.4 now
describes all three shapes and states the rule (read composition through
`model_set`, never `recsys_config`), §6 records the fix and the shim's expiry.

104 passed (was 101), black and flake8 clean.
§5.13: an unknown --models token or --stand now exits non-zero. It used to log
and continue, which Airflow reports as success - a DAG typo left an artifact
un-refreshed while Triton served the stale pickle, with no alert.

§5.14 (new): the trainer keys a person by their account id whenever it has ever
seen them signed in. The API receives only one id per request, so a previously
signed-in visitor browsing anonymously arrives as a guest_id whose embedding
lives under the account - the API has to canonicalise before asking Triton, or
those users quietly fall from the hot path to the warm one. Both halves of the
rule are named so neither can be changed alone.
The previous commit split the CONFIG and left the real problem in place:
RecommenderALS was still the whole feed. It owned a PopularModel for cold users,
an optional RecommenderCoVis for sessions, the blend weights, the four-way
segment routing and the entire Strategy vocabulary. "Add a model" meant editing
ALS, and als_covis_youtravel read as "the ALS artifact with extras" rather than
as what it is: a set of models behind one name.

Now:

    RecommenderModelSet   which member answers, and under what Strategy
    members               how to score, and nothing else

RecommenderModelSet holds main (ALS) / fallback (Popular) / session (CoVis,
optional), and `recommend()` is the single routing table in the package. A member
knows only its own algorithm: it answers can_serve(user) and returns candidates.
It does not import a sibling, hold one as a field, read the set's config, or name
a segment. RecommenderALS lost 60 lines of fusion, the cold branch, the covis
branching and every Strategy decision for segments it does not own.

Adding a warm ranker or a content model for cold tours is now a change to
recommender_model_set.py and ModelSetSettings, and to no existing model.

Two things that could have gone wrong silently, and did not:

1. Strategy honesty. My first version stamped MODEL_REALTIME_HOT_USERS on any
   known user with a session. But a session whose seeds are all unknown to the
   models yields a plain ALS ranking, and the old code correctly reported
   MODEL_HOT_USERS for it. test_hot_user_ignores_history caught the lie. Members
   now return (RecomItems, session_used) - a fact - and the set names the segment
   from it. A mislabelled strategy would have quietly corrupted every A/B readout
   split on it.

2. The two artifacts in S3 are still RecommenderALS dicts. Rather than keep a
   second routing path alive for them, RecommenderModelSet.from_legacy_als_state
   rebuilds the members from that flat state and routes them through the same
   table. serving/model.py therefore dispatches on the PICKLE'S SHAPE before the
   artifact name - the name cannot tell the generations apart, since
   als_covis_youtravel is a public query parameter and cannot change (§5.9).
   test_legacy_artifact_loads_and_recommends_identically asserts a legacy
   artifact answers all four segments with identical items, scores and strategy;
   a member assigned from the wrong key raises nothing and just serves worse.

New: test_serving_loader.py stubs Triton's backend utils so the real _load_model
can be tested off-cluster - it decides what production loads and had no test.

109 passed (was 105), black and flake8 clean, L2->L3 check clean.
Code change per §5.11: runtime repack must precede the retrain. NOT yet verified
against a real artifact pulled from S3 - that is the one gate left before deploy.
@Varfalamei Varfalamei changed the title Move composition off ALSSettings into ModelSetSettings Make the artifact a set of models, in the code and in the config Aug 21, 2026
ModelSetSettings fields were als/popular/covis - named after the models that
happen to occupy the roles today. Putting LightFM in as the main ranker would
have meant either a second field for the same role or a field called `als`
holding a LightFMSettings. Same category error as ALSSettings.covis, one level
down.

Now each field is a ROLE and the value's type is the model filling it:
main / fallback / session, each accepting any RankerSettings. Swapping an
algorithm in a role is a change to app/src/settings.py and nothing else, and the
field names finally match the member names in the composite.

MODEL_FOR_SETTINGS in recommender_model_set is the single place mapping a
settings type to its model class, so `main=LightFMSettings(...)` builds a
RecommenderLightFM with no conditional anywhere. Adding a model is one entry
there plus the type in RankerSettings.

BlendSettings.ALS_WEIGHT/COVIS_WEIGHT became MAIN_WEIGHT/SESSION_WEIGHT - the
fusion call used to read {"main": blend.ALS_WEIGHT, "session": blend.COVIS_WEIGHT},
visibly translating between two vocabularies. The legacy flat-field map absorbs
the rename, so old pickles are unaffected.

Verified rather than assumed: pydantic v2 smart-union preserves exact types
across RankerSettings, so a PopularSettings is not silently coerced into
EASESettings (both are CommonRecommenderSettings with all-default fields).
ModelSetSettings has never been retrained into an artifact, so renaming its
fields costs nothing in production - doing it after a retrain would have cost a
fourth legacy hook.

109 passed, black and flake8 clean.
The routing-as-data table (models dict + Route list) was designed, discussed
and DEFERRED on 2026-08-24: the owner chose to ship the simple role shape
(main/fallback/session) now. The document stays as the worked-out design with
its trigger conditions (a second add-on, a second project) and the recorded
cost of adopting it later (one more legacy config hook once role-shaped
ModelSetSettings exists in pickles).
@Varfalamei

Copy link
Copy Markdown
Contributor Author

The S3 real-artifact gate: PASSED

The one open gate from the PR description is closed. Both REAL artifacts were pulled from the dev bucket (models/*/20260813183106, the flat legacy RecommenderALS shape — same generation prod serves) and run through both generations of code on an identical 14-case request matrix (3 hot users from the artifact's own id map × profile/session, weighted and plain history formats, garbage-history honesty cases, candidate restriction, cold, cold+candidates):

Artifact Old code (master 6658124, RecommenderALS.load_model) New code (this branch, via TritonPythonModel._load_model) Result
als_covis_youtravel god-class adapted RecommenderModelSet 14/14 identical — items, scores (9 dp), strategies
als_youtravel god-class adapted RecommenderModelSet 14/14 identical

Strategy honesty cases confirmed on real data: hot user + unusable history → model_hot_users (not realtime) in both generations; unknown user + unusable history → empty result keeping model_realtime_warm_users, no silent fallback to popular.

The new run goes through the exact serving entry point (_load_model with shape sniffing → from_legacy_als_state), not a shortcut.

Also in the branch since the description was written: docs/MODEL_SET_ROUTING.md — a worked-out routing-as-data design, explicitly deferred 2026-08-24 in favour of shipping this simple shape.

Deploy ordering reminder (§5.11): runtime repack first, then retrain.

The doc listed manifest.json plus per-member state files as the planned next
step. It is not planned - it is declined, and a doc that says otherwise turns
into a commitment nobody made.

Three of the four problems a manifest would solve dissolve without it: the
shape-sniffing loader becomes dead code once no flat artifact is left in either
bucket, storing the dataset once saves nothing while there is one artifact, and
a schema version does not prevent an attribute silently reverting to its
__init__ default - only running a real artifact through the real loader does.
The fourth, provenance, is already answered by the trainer's log lines.

Cost side: changing the artifact format means a runtime repack plus a loader
change under the deploy ordering of CLAUDE.md 5.11 - production risk for an
ergonomic gain.
The role that consumes live session events was called `session`, while the
strategies it produces are `model_realtime_hot_users` and
`model_realtime_warm_users` - a published, append-only vocabulary mirrored by
numeric id in api/docs/DEBUG_INFO_CODEC.md. Config and wire disagreed about the
same thing, which is exactly the kind of drift this repo has been bitten by
before.

So the rule is now explicit in CLAUDE.md section 4: the SIGNAL is a session
(`history=`, `has_session`, `session_used` - the API and Redis vocabulary), and
the ROLE, its weight and its strategy are realtime.

  ModelSetSettings.session   -> .realtime
  RecommenderModelSet.session -> .realtime
  BlendSettings.SESSION_WEIGHT -> REALTIME_WEIGHT

The weight rename has a second payoff: SESSION_WEIGHT would have been the third
distinct meaning of "session weight" in this package, next to
CoVisSettings.COVIS_SESSION_WEIGHTS (a per-seed event multiplier) and the
co-occurrence kernel's session_weight argument. COVIS_SESSION_WEIGHTS is a
frozen pickle field and is deliberately untouched.

Free to do only right now: no artifact in either bucket carries a `session` key
(verified against both real pickles), so this is a rename in code, not a pickle
migration. After the first retrain it would have needed a fourth legacy hook.

Verified on the real dev artifacts pulled from S3, old code against new, 14
request cases per artifact: 28/28 identical items, scores and strategies. The
runner now pins BLAS to one thread the way app/src/settings.py does in
production - without that, ALS dot products sum in a thread-dependent order and
scores drift by ~1e-7, which reads as a diff while items and labels are
unaffected.

Tests: 109 passed. Also fixed README, which showed RecommenderALS being
constructed with a ModelSetSettings - a member takes its own leaf config.
The rename to `realtime` (340fd02) left two places behind, both found by the
owner reading the docs rather than by my grep - which only looked for
`session=` and `als=ALSSettings`, not for the role named in prose backticks.

- MODEL_SET_ROUTING.md still described the shipped shape as
  `main`/`fallback`/`session`.
- smartrec-lib/README.md described `RecommenderALS` as owning a nested Popular
  sub-model and an optional CoVis session layer. That has not been true since
  2026-08-22: those are members of `RecommenderModelSet`, and ALS knows nothing
  about siblings. Added the composite to the model list, where it was missing.

The README quickstart also could not have been run by anyone: it constructed
`RecommenderALS(recsys_config=ModelSetSettings(als=...))`, but a member takes its
own leaf config. Fixed, then actually executed - and the fixed version printed
['30', '10', '10', ...], one real recommendation padded with seven duplicates at
score 0.0, because three interactions minus filter_viewed leaves one scorable
item for top_n=10.

Checked whether that padding can reach production: on the real dev artifact
(9636 items) a deliberately narrow filter of 3 candidates with top_n=500 returns
exactly 3 distinct items, no padding, no zero scores. So it is a degenerate-
fixture artefact. The example now carries enough rows to be honest, and the
comment says why.
MODEL_HOT_AND_COLD_USERS, MODEL_ALS_COVIS_BLEND and MODEL_COVIS_SESSION are
gone from the enum. Nothing has emitted any of them for weeks: the first lost
its only emitter when RecommenderRandom was deleted, the other two briefly
labelled the als_covis session paths and were reverted in 59f69bb because a
strategy must name the segment, not the algorithm. A member that no model can
produce reads as a live option, which is the whole reason to remove it -
model_hot_and_cold_users describes a visitor who is simultaneously hot and cold.

Their ids stay reserved in api/docs/DEBUG_INFO_CODEC.md forever, and that
distinction is the point: the codec table, not this enum, is the contract. Our
API emits `_debug_info` with string keys and never parses them back, so the
table is the only authority on what an old id meant, and the consumer decodes
historical records by id. Removing a ROW would break that; removing a MEMBER
does not. The table already had rows with no member (`popular`/8, `random`/9),
so this is not a new shape.

Verified safe before removing, not after:
- no pickle in either bucket references Strategy - checked in the pickle
  bytecode of the real prod artifact, not just the dev one, because CLAUDE.md
  5.1 listed Strategy among the frozen FQNs and that turned out to be
  over-cautious;
- nothing does `Strategy(value)`, nothing iterates the enum, and the three
  strings appear in no .py file outside their own definition.

Tests 109. CLAUDE.md 5.8 rewritten: it asserted these members "stay anyway".
@Varfalamei
Varfalamei merged commit 5f396fd into master Aug 26, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant