-
Notifications
You must be signed in to change notification settings - Fork 165
refactor(BA-3644): Models package import isolation and test fixture consolidation #7702
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
Conversation
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.
Pull request overview
This pull request refactors the models package to remove all re-exports from models/__init__.py and enforce direct subpackage imports across the codebase. This change improves code organization and prevents circular import issues by requiring explicit imports (e.g., from ai.backend.manager.models.session import SessionRow instead of from ai.backend.manager.models import SessionRow).
Key Changes:
- Removed all symbol re-exports from
models/__init__.py, leaving only documentation about the new import pattern - Updated hundreds of import statements across the codebase to use direct subpackage paths
- Added
UserRoleRow,ScalingGroupRow, andResourcePresetRowto test fixtures to ensure proper SQLAlchemy mapper initialization - Introduced
ensure_all_tables_registered()function inmodels/base.pyto dynamically import all model modules for metadata operations
Reviewed changes
Copilot reviewed 54 out of 54 changed files in this pull request and generated 24 comments.
Show a summary per file
| File | Description |
|---|---|
src/ai/backend/manager/models/__init__.py |
Removed all re-exports and replaced with documentation about direct imports |
src/ai/backend/manager/models/base.py |
Added ensure_all_tables_registered() function for dynamic model loading |
src/ai/backend/manager/cli/dbschema.py |
Call ensure_all_tables_registered() before schema operations |
src/ai/backend/manager/models/vfolder/row.py |
Updated imports to use direct subpackage paths |
src/ai/backend/manager/models/alembic/versions/*.py |
Updated migration imports to direct paths |
src/ai/backend/manager/api/*.py |
Updated 15+ API module imports to direct subpackage paths |
src/ai/backend/manager/scheduler/*.py |
Updated scheduler module imports to direct paths |
src/ai/backend/manager/sweeper/*.py |
Updated sweeper module imports to direct paths |
src/ai/backend/manager/registry.py |
Refactored large import block with direct subpackage imports |
src/ai/backend/manager/plugin/error_monitor.py |
Updated import to direct path |
src/ai/backend/manager/utils.py |
Updated model imports to direct paths |
src/ai/backend/manager/types.py |
Updated SessionRow import to direct path |
tests/unit/manager/repositories/**/*.py |
Added UserRoleRow, ScalingGroupRow, and ResourcePresetRow to table fixtures across 20+ test files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| UserRoleRow, # UserRow relationship dependency | ||
| UserRow, |
Copilot
AI
Jan 3, 2026
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 comment "UserRow relationship dependency" is incorrect and the table ordering is wrong. UserRoleRow has foreign keys to both UserRow and RoleRow, so it must come AFTER these tables. The correct order should place UserRoleRow after UserRow. Additionally, RoleRow is missing from this table list but UserRoleRow references it, which may cause foreign key constraint errors.
| UserRoleRow, # UserRow relationship dependency | ||
| KeyPairRow, |
Copilot
AI
Jan 3, 2026
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 comment "UserRow relationship dependency" is incorrect and the table ordering is wrong. UserRoleRow has foreign keys to both UserRow and RoleRow, so it must come AFTER these tables. The correct order should place UserRoleRow after UserRow. Additionally, RoleRow is missing from this table list but UserRoleRow references it, which may cause foreign key constraint errors.
| UserRow, | ||
| KeyPairRow, | ||
| ScalingGroupRow, | ||
| ResourcePresetRow, |
Copilot
AI
Jan 3, 2026
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.
Missing comment explaining the table dependency. For consistency with other test files, consider adding a comment such as "# ResourcePresetRow has FK to ScalingGroupRow".
| UserRoleRow, # UserRow relationship dependency | ||
| UserRow, |
Copilot
AI
Jan 3, 2026
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 comment "UserRow relationship dependency" is incorrect and the table ordering is wrong. UserRoleRow has foreign keys to both UserRow and RoleRow, so it must come AFTER these tables. The correct order should place UserRoleRow after UserRow. Additionally, RoleRow is missing from this table list but UserRoleRow references it, which may cause foreign key constraint errors.
| UserRoleRow, # UserRow relationship dependency | ||
| UserRow, |
Copilot
AI
Jan 3, 2026
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 comment "UserRow relationship dependency" is incorrect and the table ordering is wrong. UserRoleRow has foreign keys to both UserRow and RoleRow, so it must come AFTER these tables. The correct order should place UserRoleRow after UserRow. Additionally, RoleRow is missing from this table list but UserRoleRow references it, which may cause foreign key constraint errors.
| UserRoleRow, # UserRow relationship dependency | ||
| UserRow, |
Copilot
AI
Jan 3, 2026
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 comment "UserRow relationship dependency" is incorrect and the table ordering is wrong. UserRoleRow has foreign keys to both UserRow and RoleRow, so it must come AFTER these tables. The correct order should place UserRoleRow after UserRow. Additionally, RoleRow is missing from this table list but UserRoleRow references it, which may cause foreign key constraint errors.
| from ..models.domain import MAXIMUM_DOTFILE_SIZE, verify_dotfile_name | ||
| from ..models.group import ( | ||
| association_groups_users as agus, | ||
| ) | ||
| from ..models.group import ( | ||
| groups, | ||
| query_group_domain, | ||
| query_group_dotfiles, | ||
| verify_dotfile_name, | ||
| ) |
Copilot
AI
Jan 3, 2026
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.
Redundant imports from the same module. The imports from ..models.group on lines 23-25 and 26-30 should be consolidated into a single import statement for better readability and consistency.
| UserRoleRow, # UserRow relationship dependency | ||
| UserRow, |
Copilot
AI
Jan 3, 2026
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 comment "UserRow relationship dependency" is incorrect and the table ordering is wrong. UserRoleRow has foreign keys to both UserRow and RoleRow, so it must come AFTER these tables. The correct order should place UserRoleRow after UserRow. Additionally, RoleRow is missing from this table list but UserRoleRow references it, which may cause foreign key constraint errors.
| UserRoleRow, # UserRow relationship dependency | ||
| UserRow, |
Copilot
AI
Jan 3, 2026
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 comment "UserRow relationship dependency" is incorrect and the table ordering is wrong. UserRoleRow has foreign keys to both UserRow and RoleRow, so it must come AFTER these tables. The correct order should place UserRoleRow after UserRow. Additionally, RoleRow is missing from this table list but UserRoleRow references it, which may cause foreign key constraint errors.
| UserRoleRow, # UserRow relationship dependency | ||
| UserRow, |
Copilot
AI
Jan 3, 2026
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 comment "UserRow relationship dependency" is incorrect and the table ordering is wrong. UserRoleRow has foreign keys to both UserRow and RoleRow, so it must come AFTER these tables. The correct order should place UserRoleRow after UserRow. Additionally, RoleRow is missing from this table list but UserRoleRow references it, which may cause foreign key constraint errors.
…rt isolation Remove all symbol re-exports from models/__init__.py to enforce direct subpackage imports. This prevents circular import issues and improves code organization. Changes: - models/__init__.py: Remove all re-exports, keep only documentation - Update all imports across codebase to use direct subpackage imports (e.g., from ai.backend.manager.models.session import SessionRow) - Fix repository tests: Add ScalingGroupRow and ResourcePresetRow to with_tables for SQLAlchemy mapper initialization 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
…fixtures - Fix update_route_traffic_force to load all relations (model, image, session_owner) to prevent MissingGreenlet errors during lazy loading - Refactor test fixtures to return IDs instead of Row objects - Delete duplicate mock-based test files causing mapper initialization errors: - test_create_auto_scaling_rule_validated.py - test_create_endpoint_validated.py - test_get_endpoint_by_id_validated.py - test_get_endpoint_by_name_validated.py - test_list_endpoints_by_owner_validated.py - test_update_auto_scaling_rule_validated.py - test_update_endpoint_lifecycle_validated.py - Fix AsyncGenerator type annotation in test_registry.py fixture - Delete obsolete integration tests and scheduler test utilities - Reorganize model tests to proper locations 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
…manager:src deps - Move PasswordColumn from user/row.py to hasher/types.py - Breaks base.py → user/__init__.py → user/row.py → 33 Row chain - base.py now has 0 Row dependencies - Remove unused SessionGetter Protocol from manager/types.py - Eliminates TYPE_CHECKING import of SessionRow - manager/types.py now has 0 Row dependencies - Remove explicit manager:src dependencies from repository test BUILD files - Allows pants to infer only required dependencies - Repository tests now have 0 transitive Row dependencies 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
…kage structure - Updated import paths in `auth.py`, `cluster_template.py`, `domainconfig.py`, `events.py`, `groupconfig.py`, `logs.py`, `manager.py`, `scaling_group.py`, `service.py`, `session.py`, `session_template.py`, `stream.py`, `userconfig.py`, `utils.py`, `vfolder.py`, `dbschema.py`, `types.py`, `hasher/types.py`, `user/row.py`, `vfolder/row.py`, `plugin/error_monitor.py`, `repositories/model_serving/admin_repository.py`, `scheduler/agent_selector.py`, `scheduler/dispatcher.py`, `scheduler/drf.py`, `scheduler/fifo.py`, `scheduler/predicates.py`, `scheduler/types.py`, `sweeper/kernel.py`, `sweeper/session.py`, `tests/unit/manager/conftest.py`, and `tests/unit/manager/repositories/user/test_user_repository.py`. - Removed unnecessary relative imports and replaced them with absolute imports for clarity and maintainability. - Adjusted regex patterns in test cases for better matching of exception messages.
6df9d74 to
e48d2f1
Compare
Co-authored-by: octodog <[email protected]>
Remove all symbol re-exports from models/init.py to enforce direct
subpackage imports. This prevents circular import issues and improves
code organization.
Changes:
(e.g., from ai.backend.manager.models.session import SessionRow)
with_tables for SQLAlchemy mapper initialization
🤖 Generated with Claude Code
Co-Authored-By: Claude Opus 4.5 [email protected]
resolves #NNN (BA-MMM)
Checklist: (if applicable)
ai.backend.testdocsdirectory