-
Notifications
You must be signed in to change notification settings - Fork 47
Migrate policy queries to SQLAlchemy #4970
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ccbf717b9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "policy_created_at": pv.policy.created_at, | ||
| "user_id": pv.policy.user_id, | ||
| "name": pv.policy.name, | ||
| "version": pv.version, | ||
| "tags": {tag.key: tag.value for tag in pv.tags} if pv.tags else {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eager-load tags before accessing them
Because @with_db closes the session when the query function returns, any PolicyVersion objects handed to callers are detached. This helper accesses pv.tags, which is lazily loaded and is not eagerly loaded in get_policy_versions/get_policy_version_by_id (they only selectinload(PolicyVersion.policy)), so calling policy_version_to_public_dict on those returned objects will raise a DetachedInstanceError or trigger per-row lazy loads before the session is gone. Consider eager-loading PolicyVersion.tags in the query functions or avoid touching pv.tags after the session context ends.
Useful? React with 👍 / 👎.
|
| session = get_db() | ||
|
|
||
| query = select(PolicyVersion).join(PolicyVersion.policy) | ||
| count_query = select(func.count(func.distinct(PolicyVersion.policy_id))).join(PolicyVersion.policy) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The count query is counting distinct policy_id values instead of counting the total number of PolicyVersion records. This will return the wrong total count when multiple versions of the same policy match the filters.
Should be:
count_query = select(func.count()).select_from(PolicyVersion).join(PolicyVersion.policy)| count_query = select(func.count(func.distinct(PolicyVersion.policy_id))).join(PolicyVersion.policy) | |
| count_query = select(func.count()).select_from(PolicyVersion).join(PolicyVersion.policy) |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
8cfe0d6 to
be5f9c0
Compare
24f1d7d to
19c4a42
Compare
|
Found 6 test failures on Blacksmith runners: Failures
|
19c4a42 to
afc9c59
Compare
afc9c59 to
9d4cd17
Compare
| s3_path: str | None = None | ||
| git_hash: str | None = None | ||
| policy_spec: dict[str, Any] | None = Field(default=None, sa_column=Column(JSONB)) | ||
| attributes: dict[str, Any] | None = Field(default=None, sa_column=Column(JSONB)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PolicyVersion model is missing the 'internal_id' field which is referenced in other parts of the code and appears to be required by the database schema. Add 'internal_id: int = Field(sa_column=Column(Integer, autoincrement=True))' or similar definition to the model to match the database schema requirements.
Spotted by Graphite Agent (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
- Add migration to change unique constraint from (user_id, name) to (name) - Update upsert_policy to reject duplicate names with 409 Conflict - Deduping of existing duplicates was already run in prod Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add PolicyVersionTag model and update Policy/PolicyVersion models - Create queries/policy_queries.py with SQLAlchemy-based queries - Includes helper functions for converting models to response dicts - Foundation for migrating stats_routes from raw SQL to SQLAlchemy Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Replace stats_repo.upsert_policy with policy_queries.upsert_policy - Replace stats_repo.create_policy_version with policy_queries.create_policy_version - Replace stats_repo.get_policy_version_with_name with policy_queries.get_policy_version_with_name - Replace stats_repo.get_public_policy_version_by_id with policy_queries.get_policy_version_by_id - Replace stats_repo.upsert_policy_version_tags with policy_queries.upsert_policy_version_tags - Replace stats_repo.get_policies with policy_queries.get_policies - Replace stats_repo.get_policy_versions with policy_queries.get_policy_versions - Replace stats_repo.get_versions_for_policy with policy_queries.get_versions_for_policy - Replace stats_repo.get_user_policy_versions with policy_queries.get_user_policy_versions - Move response models (PolicyRow, PublicPolicyVersionRow, PolicyVersionWithName) to stats_routes Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
9d4cd17 to
fa7f5de
Compare
| version_count: int | None = None | ||
|
|
||
|
|
||
| class EpisodeReplay(BaseModel): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add an import statement to re-export PolicyVersionWithName from its new location to maintain backward compatibility: 'from metta.app_backend.routes.stats_routes import PolicyVersionWithName'
Spotted by Graphite Agent (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.

Summary
models/policies.pywith Policy, PolicyVersion, and PolicyVersionTag SQLModel modelsqueries/policy_queries.pywith SQLAlchemy query functionsroutes/stats_routes.pyto use the new query modulePolicyRow,PolicyVersionRow,PolicyVersionWithName,PublicPolicyVersionRow,upsert_policy,get_latest_policy_version,create_policy_version, etc.)Part of stack to migrate app_backend from raw SQL to SQLAlchemy.