Skip to content

[iris] Introduce stores layer between transitions.py and db.py#5147

Merged
rjpower merged 3 commits intomainfrom
claude/interesting-lewin-3cb93c
Apr 24, 2026
Merged

[iris] Introduce stores layer between transitions.py and db.py#5147
rjpower merged 3 commits intomainfrom
claude/interesting-lewin-3cb93c

Conversation

@rjpower
Copy link
Copy Markdown
Collaborator

@rjpower rjpower commented Apr 23, 2026

Phase 1 of the transitions -> stores -> db refactor: add ControllerStore bundling typed per-entity stores, and fold EndpointRegistry into EndpointStore. Remaining stores (Job, Task, TaskAttempt, Worker, DispatchQueue, Reservation) are skeletons to be filled in subsequent phases. ControllerTransitions now takes store= and routes endpoint ops through it; transitions.py retains a temporary self._db escape hatch for unmigrated queries.

@rjpower rjpower added the agent-generated Created by automation/agent label Apr 23, 2026
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 23, 2026

Claude finished @rjpower's task in 1m 15s —— View job


Code review

Checked for bugs and CLAUDE.md / AGENTS.md compliance across all 18 modified files. Ran four parallel reviewers (2x AGENTS.md compliance, 2x bug/logic) and validated each candidate issue against the diff and repo state.

Findings (1 issue):

  • 📌 transitions.py#L979-L981 — comment references plans/i-want-to-introduce-prancy-swing.md, which does not exist in the repo. Inline comment with suggested fix left on the PR.

Intentionally not flagged (false positives / deliberate design):

  • self._db = store._db — PR body explicitly calls this out as a temporary escape hatch for the phased migration
  • Empty skeleton JobStore/TaskStore/… classes — PR body explicitly calls these out as placeholders for subsequent phases
  • Missing return-type annotations on ControllerStore.transaction() / read_snapshot() — mirrors pre-existing ControllerDB signatures; stylistic, not high-signal
  • delete_task_endpoints being typed to concrete EndpointStore while peer helpers remain duck-typed — consistency nit, not a correctness issue

No bugs, no broken call sites, no misrouted store/db access. The refactor wires cleanly: every self._db.endpoints.* callsite is migrated to self._store.endpoints.*, and the register_reopen_hook mechanism correctly replaces the old ControllerDB.endpoints property.

  • Review complete
    • Branch: claude/interesting-lewin-3cb93c

Comment on lines +979 to +981
# Escape hatch kept only while the phased migration moves SQL out of
# this file. Direct ``self._db`` SQL calls should decrease every phase
# and hit zero at the end (see plans/i-want-to-introduce-prancy-swing.md).
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The referenced plan file plans/i-want-to-introduce-prancy-swing.md does not exist in the repository (the canonical plan location is .agents/projects/, not plans/). Per AGENTS.md: "Delete stale comments immediately on discovery." Either add the plan doc or drop the reference.

Suggested change
# Escape hatch kept only while the phased migration moves SQL out of
# this file. Direct ``self._db`` SQL calls should decrease every phase
# and hit zero at the end (see plans/i-want-to-introduce-prancy-swing.md).
# Escape hatch kept only while the phased migration moves SQL out of
# this file. Direct ``self._db`` SQL calls should decrease every phase
# and hit zero at the end.

@rjpower rjpower force-pushed the claude/interesting-lewin-3cb93c branch from d085f0c to cb895dc Compare April 24, 2026 16:08
@rjpower rjpower requested a review from yonromai April 24, 2026 16:08
@rjpower rjpower force-pushed the claude/interesting-lewin-3cb93c branch from cb895dc to 2525406 Compare April 24, 2026 16:18
rjpower added 3 commits April 24, 2026 09:23
Phase 1 of the transitions -> stores -> db refactor: add ControllerStore
bundling typed per-entity stores and fold EndpointRegistry into EndpointStore.
Remaining stores (Job, Task, TaskAttempt, Worker, DispatchQueue, Reservation)
are skeletons to be filled in subsequent phases.
Phase 2 of the stores refactor. Moves the ~26 jobs/job_config/users/user_budgets
queries out of transitions.py into JobStore. Adds fetchall/fetchone on
TransactionCursor so read methods accept either a write cursor or a read snapshot
uniformly. Renames the endpoint registry test file to match the new store name.
@rjpower rjpower force-pushed the claude/interesting-lewin-3cb93c branch from 2525406 to bcd7edc Compare April 24, 2026 16:23
Copy link
Copy Markdown
Contributor

@yonromai yonromai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Code review

Approved. No issues found in the current head. Checked for bugs and AGENTS.md compliance.

Validation:

  • cd lib/iris && uv run --group dev python -m pytest -n1 --tb=short -m 'not slow and not docker and not e2e' tests/cluster/controller/test_endpoint_store.py tests/cluster/controller/test_transitions.py tests/cluster/controller/test_service.py tests/cluster/controller/test_dashboard.py tests/cluster/controller/test_api_keys.py tests/cluster/controller/test_auth.py tests/cluster/controller/test_task_resource_history.py tests/test_budget.py
  • Result: 347 passed, 6 warnings in 63.32s
  • uv run --package marin --group test python -m pytest -q tests/integration/iris/test_iris_kind.py::test_gpu_pod_attributes_with_in_memory_k8s
  • Result: 1 passed, 1 warning in 0.69s

Generated with Codex.

@rjpower rjpower enabled auto-merge (squash) April 24, 2026 17:01
@rjpower rjpower disabled auto-merge April 24, 2026 17:02
@rjpower rjpower merged commit ef99cff into main Apr 24, 2026
42 checks passed
@rjpower rjpower deleted the claude/interesting-lewin-3cb93c branch April 24, 2026 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent-generated Created by automation/agent

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants