Skip to content

Give the library a layer model, and an architecture contract to keep it - #10

Merged
Varfalamei merged 9 commits into
masterfrom
refactor/library-layers
Aug 17, 2026
Merged

Varfalamei merged 9 commits into
masterfrom
refactor/library-layers

Conversation

@Varfalamei

Copy link
Copy Markdown
Contributor

Why

The library had no declared structure, so every change added a module and the shape eroded. Three concrete symptoms, all verified:

  1. The research layer was loaded inside Triton. recommender_als.py imported rrf_fuse from policy/fusion.py, which executes policy/__init__.py, which imports PolicyModel and rectools' model_from_config. A broken import in offline-only code would have taken down inference for every model in the bucket, including als_youtravel.
  2. The co-visitation algorithm existed three times. covis_kernel.py had been extracted to unify the serving and research copies, but only the serving caller was ported, so the kernel was a third copy sitting at the package root with no layer.
  3. Four modules were named model.py, and smartrec_lib.model (settings) vs smartrec_lib.models (a model) differed by one character while meaning opposite things.

What

CLAUDE.md at the repo root states the architecture contract: four layers with imports flowing strictly downward, a table saying where a new file belongs, naming rules, and the 13 invariants that break production silently when violated (the pickle is a bare dict, so a renamed attribute does not raise — it reverts to the __init__ default and the model quietly serves degraded results).

model.py          L0  contract: RecomItems, Strategy, *Settings   (frozen by pickles)
kernels/          L1  pure algorithms: cooccurrence, fusion, constraints
recommenders/     L2  the servable models                         (paths frozen by pickles)
serving/          L2  Triton python backend
research/         L3  rectools-native CoVisModel and PolicyModel
evaluation/       L3  offline protocols
  • policy/fusion.py, policy/constraints.pykernels/; research no longer loads in serving.
  • covis_kernel.pykernels/cooccurrence.py, and research/covis.py is ported onto it, so the algorithm exists once and the two callers differ only in the parameters they pass.
  • models/ and policy/research/; the two remaining model.py are the two that are forced, one by the pickles in S3 and one by Triton's file-name convention.
  • Recommenders import RecommenderModel from recommenders.base instead of from their own package, so __init__.py line order stops being a correctness requirement.

Also fixes the red CI on master

test_fit_popular and test_fit_als pinned exact result lists over tied items. The fixture holds 100 rows over 99 items, so 98 tie at one unique user; test_fit_popular expected three arbitrary members of that tie, and not even the genuinely most popular item, which filter_viewed drops. They now assert what the fixture determines. This is why the suite passed locally and failed in CI with no behaviour change.

Adds unit tests for the kernel, which no test reached before — including top_k truncating a tie, the regime where the two callers can genuinely diverge.

Safety

Nothing frozen moved. Verified against a freshly trained als_covis artifact: the pickle references smartrec_lib.model and smartrec_lib.recommenders.recommender_covis and nothing else, so artifacts already in S3 load unchanged; a dill round-trip still serves covis_session. 99 tests pass under PYTHONHASHSEED 0, 1, 42 and 12345; flake8 hard gate is clean.

Deploy note: this is a code change, so it needs a runtime repack (make pixi-pack-build-docker + upload) before the retrain that ships it — recommend() comes from runtime_env.tgz, not from the pickle.

RecommenderOrchestrator is unreachable: the trainer's model map has no entry for
it (als, popular, ease, covis, als_covis), so no artifact is ever produced and
Triton never loads it. Its cascade was superseded by als_covis, which does the
same routing inside one artifact and has measured numbers behind it.

The `context` Triton input existed solely to feed the orchestrator - the backend
passed it to no other model, and the client only attaches the tensor `if context`,
which is never true today. Removing it also removes the failure mode that broke a
rebuilt api image earlier: a client without the parameter against a backend with
it raised TypeError on every SmartRec call.

Removed: recommender_orchestrator.py, its export, OrchestratorSettings, the
context parsing in serving/model.py and the input from config.pbtxt. Existing
artifacts keep working - the backend simply stops looking for an input nobody
sends. 68 library tests pass.
ALSSettings carried the settings of three models at once: ALS itself, the
Popular model it embeds for cold users, and CoVis plus the blend that fuses
them. Whether the models were combined was a boolean (SESSION_COVIS_ENABLED)
sitting next to seven orphan fields copy-pasted from the sub-models.

Composition is now expressed by nesting:

  ALSSettings.popular -> PopularSettings   (cold-user fallback)
  ALSSettings.covis   -> CoVisSettings|None (None = no session layer)
  ALSSettings.blend   -> BlendSettings      (ALS x CoVis RRF fusion)

"Combined or not" is `covis is None`, and CoVis's own knobs live only in
CoVisSettings. The getattr defaults that existed to tolerate the flat shape
are gone, since every sub-config is now guaranteed present.

Behaviour is unchanged: same inputs produce the same recommendations, scores
and strategy strings.

Back-compat with published artifacts: recsys_config is pickled inside every
model.pkl in S3, so ALSSettings.__setstate__ rebuilds the nested sub-configs
from the old flat fields on unpickle (pickle restores __dict__ verbatim and
skips validation). Stale flat keys are left in __dict__ so straggler readers
still work. tests/test_config_composition.py writes a model.pkl holding a
legacy-shaped config, reloads it with the current code and asserts the
recommender builds and returns identical items/scores/strategies on all four
routing paths; disabling the migration fails five of those tests.
… hooks

None of the three could ever run in production:

- LightFM and Random are absent from the trainer's model map (als, popular,
  ease, covis, als_covis), so no artifact is ever produced for them and Triton
  never loads one. Neither exists in the dev or prod S3 buckets.
- train_partial() was the only incremental-training entry point. The ALS
  implementation raised NotImplementedError and the base one raised too, so the
  path died before it saved anything; nothing calls it any more.

Removed: recommender_lightfm.py, recommender_random.py, their exports,
LighFMSettings and RandomSettings, their branches in the serving model-class
ladder, their logger names in the Triton logging bridge, and the LightFM test.
train_partial is gone from both base.py and recommender_als.py.

Strategy.MODEL_HOT_AND_COLD_USERS stays in the enum: no model emits it any
more, but the string is a published contract (api/docs/DEBUG_INFO_CODEC.md).

rectools-lightfm is dropped from smartrec-lib dependencies (uv.lock relocked -
removals only, no version churn), so the x86_64-only lightfm extension is out
of the import path and the suite runs on ARM without stubbing it. The CI step
that cleaned the lightfm wheel from the uv cache is dropped with it.

Verified: 79 passed with no lightfm stub, and the real 892MB prod als_youtravel
artifact still unpickles and recommends (hot / cold / session paths). Its only
smartrec reference is smartrec_lib.model.ALSSettings - no live artifact points
at a removed class.
…d differ

The algorithm exists twice: models/covis.py::CoVisModel (rectools-native, drives
evaluation/) and recommenders/recommender_covis.py::RecommenderCoVis (serving,
pickled into covis_youtravel and into the als_covis_youtravel session layer).
Whether they had drifted in behaviour was folklore; this makes it an executable
spec.

Verified equivalence: with the research-only caps neutralised the two produce
identical neighbour graphs and identical session scores - same keys, same edge
weights, same ranked items, same floats. min_cooc is handled identically. There
is no algorithmic drift in the core.

Verified divergences, one test each:
- fit_basket_size (research only, default 100): serving builds baskets from the
  user's entire history, so its C(N,2) pair count is unbounded.
- session_size (research only, default 20): serving applies no seed cap, and
  truncation changes the recency denominator, hence the scores.
- COVIS_SESSION_WEIGHTS ("sw", serving only): CoVisModel has no weight concept;
  its output matches the flag-OFF branch.
- tie-break: CoVisModel resolves equal co-occurrence counts by internal id
  ascending and is reproducible. RecommenderCoVis sorts by count only, so ties
  follow set-iteration order over strings - the same data yields different top_k
  neighbours under different PYTHONHASHSEEDs. Only the deterministic side is
  asserted here; asserting the non-determinism needs separate processes.

Test-only change: no production behaviour, artifact or contract is touched.
Maps what exists in recommenders/ (serving, five classes, five artifacts, the
Strategy vocabulary) against models/ + policy/ (research, rectools ModelBase,
zero artifacts, consumed only by evaluation/), with file/line evidence, and
resolves the CoVis duplication question: same algorithm, four parameter-or-defect
level divergences, pinned by tests/test_covis_equivalence.py.

Two findings that shape the options. First, neither composition implementation is
a superset of the other: PolicyModel routes every cold and warm user to the
popularity fallback, so "unknown user + session -> CoVis" (the covis_session
strategy) is structurally unrepresentable in it, and als_covis_blend is
reproducible only via its train-count session proxy. "Just move serving to
PolicyModel" is therefore blocked, not merely expensive. Second, the blend
weights in BlendSettings were tuned on the research CoVis (2026-08-03 policy
grid) while production serves the other one; the decisive 2026-08-13 re-measure
used the real artifacts. The transfer error is probably small but is unmeasured.

Three options with target shape, layout sketch, per-constraint analysis,
migration order and mid-way rollback: (A) freeze the research hierarchy as an
eval-only tool, (B) one parameterised co-visitation kernel behind two thin
shells, bit-identical in its first step, (C) promote PolicyModel to a serving
citizen behind a new artifact and an A/B.

Recommends B, first step already landed as the characterisation test. Records
eight traps, including that the pickle reconstructs nested classes but not the
outer recommender, so RecommenderALS is renameable and RecommenderCoVis is not -
and that the largest available win (the unnormalised 70/30 session blend, worst
of six measured options) needs no unification at all.
Every module now belongs to one of four layers with imports flowing strictly
downward: L0 contract (model.py), L1 pure kernels, L2 serving, L3 research.
CLAUDE.md states the rules, where a new file belongs, and the 13 invariants
that break production silently when violated.

The structure this replaces had no declared homes, so each change added a
module and the shape eroded:

- recommender_als.py imported rrf_fuse from policy/fusion.py, which executes
  policy/__init__.py, which imports PolicyModel and rectools' model_from_config.
  The entire research policy package was inside the Triton import closure, so a
  broken import in offline code would take down inference for every model.
  fusion.py and constraints.py move down to kernels/; research no longer loads
  in serving.
- The co-visitation algorithm was written twice. covis_kernel.py had been
  extracted to fix that but only the serving caller was ported, leaving a third
  copy at the package root with no layer. It becomes kernels/cooccurrence.py and
  research/covis.py is ported onto it, so the algorithm exists once and the two
  callers differ only in the parameters they pass.
- Four modules were named model.py, and `smartrec_lib.model` (settings) versus
  `smartrec_lib.models` (a model) differed by one character. models/ and policy/
  become research/; the two remaining model.py are the two that are forced - one
  by the pickles in S3, one by Triton's file-name convention.
- Every recommender imported RecommenderModel from its own package, making the
  line order inside __init__.py a correctness requirement. They now import from
  recommenders.base.

Nothing that is frozen moved. Verified against a freshly trained als_covis
artifact: the pickle references smartrec_lib.model and
recommenders.recommender_covis and nothing else, so artifacts already in S3 load
unchanged, and a dill round-trip still serves covis_session.

This is a code change: it needs a runtime repack before the retrain that ships
it, per CLAUDE.md section 5.11.
test_fit_popular and test_fit_als pinned exact result lists over items with
identical scores, which is what turned a dependency bump into the red build on
master: the suite passes locally and fails in CI without any behaviour change.

tests/interactions.csv holds 100 rows over 99 items, so 98 of them tie at one
unique user and exactly one (3105) has two. test_fit_popular expected
["3468", "434", "1217"] - three arbitrary members of that 98-way tie, and not
even the genuinely most popular item, which filter_viewed drops because user 1
viewed it. Both tests now assert what the fixture actually determines: nothing
viewed comes back, every returned item belongs to the tie group, scores are as
computed, and in the ALS case the one item with two users leads while the four
tied behind it are compared as a set.

Adds unit tests for the co-visitation kernel, which no test reached before.
They cover the regime where its two callers can genuinely diverge - top_k
truncating a tie - plus the basket and session caps, per-seed event weights,
and the exclude/allowed filters.

Verified under PYTHONHASHSEED 0, 1, 42 and 12345: 99 passed each time.
Updates the references that the layer refactor invalidated, including the
layering check in CLAUDE.md itself, which was still looking for module prefixes
that no longer exist and so could never have failed.

DESIGN_UNIFICATION.md keeps its original paths and line numbers as the record of
the analysis; its header now says which of its options was taken and which was
not.
@Varfalamei
Varfalamei merged commit e48bc2e into master Aug 17, 2026
3 checks passed
@Varfalamei
Varfalamei deleted the refactor/library-layers branch August 17, 2026 08:44
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