Skip to content

Public-usage review: LogQ fix, CLI parity, repo hygiene#19

Closed
tanaysd wants to merge 8 commits into
mainfrom
cursor/public-usage-review-6712
Closed

Public-usage review: LogQ fix, CLI parity, repo hygiene#19
tanaysd wants to merge 8 commits into
mainfrom
cursor/public-usage-review-6712

Conversation

@tanaysd

@tanaysd tanaysd commented May 24, 2026

Copy link
Copy Markdown
Owner

Summary

End-to-end review of the repo for public consumption now that
tanaysd/mmoe-recsys is public. Findings are catalogued in the new
PUBLIC_REVIEW.md. The fixes that can be made cheaply without
reshaping the public API are applied here; everything else is documented
so a public reader knows the posture is deliberate.

Local sanity (re-run after every commit): uv run ruff check .,
uv run ruff format --check ., uv run mypy packages/, uv run pytest -q
all pass — 127 tests pass (was 118; +9 new tests for LogQ, seed,
and bulk-insert behaviors).

Findings catalogued in PUBLIC_REVIEW.md

# Severity Area Fixed in this PR?
1 HIGH ML correctness — LogQ correction silently a no-op
2 MEDIUM Docs/CLI drift — --resume-from undocumented in CLI
3 MEDIUM Quick Start perf — ~60k SQLite fsyncs in populate
4 MEDIUM Reproducibility — no global seed in training
5 MEDIUM Docs/safety — README over-sells deployability ✅ (warning added)
6 LOW Repo hygiene — missing CONTRIBUTING/SECURITY/etc.
7 LOW Docs polish — "5-step" vs 6 numbered steps
8 LOW Docs — no protobuf regen command
9 INFO gRPC TLS/authn/etc. — by design, documented Doc-only

Commits (one logical change each)

  1. docs: add public-usage review covering 9 findings — adds
    PUBLIC_REVIEW.md with the full report.
  2. fix(retrieval): LogQ correction was silently a no-op
    np.log(max(v, 1)) clamped every popularity value to log(1) = 0;
    replaced with np.log(max(v, 1e-12)). Unknown-item fallback unified
    with the in-vocabulary path. Adds tests/retrieval/test_dataset_logq.py.
  3. feat(training): add --resume-from and --seed CLI flags
    adds the CLI flags the README already advertised, adds
    recsys_common.seed.set_seed() (seeds Python/NumPy/Torch), wires
    both train_retrieval.py and train_ranking.py to seed on startup.
    Adds tests/common/test_seed.py.
  4. perf(serving): batch SQLite writes in populate_feature_store
    adds put_*_bulk methods using executemany inside a single
    transaction; scripts/populate_feature_store.py switches to them.
    Adds round-trip and empty-input tests.
  5. docs(community): add CONTRIBUTING, SECURITY, issue/PR templates, dependabot
    — six standard community files.
  6. chore: ruff auto-fixes (isort + format) — picked up by lint
    after the seed import was added to training scripts.
  7. docs(readme): production-readiness section + 6-step pipeline + dev posture
    — adds a "Production readiness" gap table, softens "production-grade"
    headline copy, fixes the 5 vs 6 step inconsistency, documents
    --resume-from, documents --seed, documents protobuf regeneration.
  8. docs(changelog): record 0.6.0 entries.

Notes for the reviewer

  • The LogQ bug is the only behavior change in this PR that affects the
    actual training math. Existing checkpoints will still load fine — the
    fix only changes how the next training run subtracts popularity from
    in-batch similarity logits. The integration tests
    (tests/integration/test_serving_pipeline.py) still pass end-to-end.
  • The per-row SqliteFeatureStore.put_* methods are intentionally kept;
    they are part of the FeatureStore Protocol surface and are used in
    several tests. The bulk variants are additive.
  • SECURITY_REVIEW.md is left untouched as a historical record;
    SECURITY.md links to it.

Checklist

  • uv run ruff check . passes
  • uv run ruff format --check . passes
  • uv run mypy packages/ passes
  • uv run pytest passes locally (127 tests)
  • Added or updated tests for the change (LogQ, seed, bulk)
  • Updated README.md and CHANGELOG.md
Open in Web Open in Cursor 

cursoragent and others added 8 commits May 24, 2026 22:09
Co-authored-by: Tanay Sood <tanaysd@users.noreply.github.com>
popularity.json stores probabilities in [0, 1], but the dataset loader
did np.log(max(v, 1)) which clamps every value to log(1) = 0. The
in-batch InfoNCE correction therefore subtracts 0 from every logit and
the documented popularity de-biasing has no effect.

The original max(...) was likely written assuming counts; the producer
writes probabilities. Switch the floor to 1e-12 so probabilities flow
through log unchanged and unknown items get the same floor treatment
(was np.log(1e-10), making missing items even more popular than the
floor — the opposite of the intent).

Add tests/retrieval/test_dataset_logq.py to prevent regression.

Co-authored-by: Tanay Sood <tanaysd@users.noreply.github.com>
- Both train_retrieval.py and train_ranking.py now expose --resume-from PATH
  (was advertised in README but had no CLI binding) and --seed N.
- Add recsys_common.seed.set_seed() seeding Python random / NumPy / torch
  (CPU + CUDA); both training entry points call it on startup.
- Adds TrainConfig.seed (default 42) so YAML configs and CLI can override.
- Refactor both CLI override blocks into a single shared shape so adding
  future fields doesn't drift between the two scripts.

Two consecutive runs of the same script against the same artifacts now
produce identical metrics and ONNX exports — required for benchmarking,
bug reproduction, and ablation studies.

Co-authored-by: Tanay Sood <tanaysd@users.noreply.github.com>
Each per-row put_* call commits its own transaction and forces an fsync,
so the Quick Start defaults (10k users + 50k items) performed ~60k
individual commits. On a typical laptop this is the slowest step of
the Quick Start by an order of magnitude.

- Add put_user_features_bulk, put_item_features_bulk,
  put_user_sessions_bulk on SqliteFeatureStore (executemany + single
  commit).
- Switch scripts/populate_feature_store.py to use the bulk APIs.
- Per-row APIs are retained for tests and for callers extending the
  FeatureStore Protocol.
- Add round-trip + empty-input tests for the bulk methods.

Co-authored-by: Tanay Sood <tanaysd@users.noreply.github.com>
…ndabot

Now that the repo is public, add the conventional community files
GitHub contributors look for:

- CONTRIBUTING.md — dev setup, required checks, conventions, scope
- SECURITY.md — explicit dev-only posture + private vuln-report channel
- .github/ISSUE_TEMPLATE/{bug_report,feature_request}.md
- .github/PULL_REQUEST_TEMPLATE.md
- .github/dependabot.yml — weekly batched pip + github-actions updates

These don't change any runtime behavior.

Co-authored-by: Tanay Sood <tanaysd@users.noreply.github.com>
- isort: keep third-party (torch.*) imports together; recsys_common.seed
  moves into the third-party block since recsys_common is now a sibling
  workspace package (not first-party to recsys_ranking/recsys_retrieval).
- ruff format collapses a multi-line SyntheticConfig() in the new LogQ
  test fixture.

Co-authored-by: Tanay Sood <tanaysd@users.noreply.github.com>
…sture

- Add a 'Production readiness' table enumerating what the reference
  implementation does NOT provide (TLS, authn/z, rate limiting, per-user
  authorization, unknown-user fallback, multi-writer feature store,
  observability), with the modules to plug each into. Links to SECURITY.md
  and SECURITY_REVIEW.md.
- Soften the 'production-grade' headline copy to 'production-shaped
  reference implementation' and add a visible callout near the top.
- Fix the '5-step pipeline' / 6 numbered steps inconsistency.
- Document --resume-from on both training scripts (was advertised but
  the CLI didn't expose it until the previous commit).
- Add 'Reproducibility' design-decision entry.
- Add 'Contributing' section linking CONTRIBUTING.md and document the
  protobuf regeneration command for proto/recsys.proto edits.

Co-authored-by: Tanay Sood <tanaysd@users.noreply.github.com>
…iene

Co-authored-by: Tanay Sood <tanaysd@users.noreply.github.com>
@tanaysd tanaysd closed this May 26, 2026
@tanaysd
tanaysd deleted the cursor/public-usage-review-6712 branch May 26, 2026 07:42
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.

2 participants