fix(bench): break circular import in predictor/__init__.py#2802
Conversation
Greptile code reviewThis repo uses Greptile for automated review. Before merge, aim for Confidence Score: 5/5 with zero unresolved review threads — see CONTRIBUTING.md. Run a review — add a PR comment with: Give it ~5-10 minutes (sometimes longer) for results, then fix feedback and re-trigger until you reach Confidence Score: 5/5. Optional: automate with the greploop skill. |
|
@greptile review |
Greptile SummaryThis PR resolves a circular import in
Confidence Score: 5/5Safe to merge — the circular-import cycle is broken at the structural level, all callers are updated, and backward compatibility is preserved through re-export shims and the lazy loader. The dependency graph refactor is correct and complete: No files require special attention beyond the already-noted caching behaviour in Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph Before["Before (circular)"]
direction TB
A1["predictor/__init__.py"] -->|eager import| B1["investigation_handoff.py"]
B1 -->|from scoring import _taxonomy_for_root_cause| C1["scoring.py"]
C1 -->|from predictor.vocabulary import ...| D1["predictor/vocabulary.py"]
D1 -->|triggers parent package init| A1
style A1 fill:#f88,stroke:#c00
style D1 fill:#f88,stroke:#c00
end
subgraph After["After (acyclic)"]
direction TB
A2["predictor/__init__.py"] -->|eager imports| E2["llm_call / snapping / rerank / vocabulary"]
A2 -->|__getattr__ lazy| B2["investigation_handoff.py"]
B2 -->|from taxonomy import taxonomy_for_root_cause| T2["taxonomy.py"]
C2["scoring.py"] -->|from taxonomy import ...| T2
E2 -->|from closed_vocabulary import ...| CV["closed_vocabulary.py"]
T2 -->|from closed_vocabulary import ...| CV
style CV fill:#8f8,stroke:#060
style T2 fill:#8f8,stroke:#060
end
Reviews (3): Last reviewed commit: "fixed security issues" | Re-trigger Greptile |
| def __getattr__(name: str) -> Any: | ||
| """Lazy-load investigation handoff so ``scoring`` can import ``vocabulary`` | ||
| without a circular import (handoff imports ``_taxonomy_for_root_cause`` from | ||
| ``scoring``).""" | ||
| if name in _INVESTIGATION_HANDOFF_EXPORTS: | ||
| from tests.benchmarks.cloudopsbench.predictor.investigation_handoff import ( | ||
| align_predictions_to_investigation, | ||
| apply_investigation_handoff, | ||
| ) | ||
|
|
||
| return { | ||
| "align_predictions_to_investigation": align_predictions_to_investigation, | ||
| "apply_investigation_handoff": apply_investigation_handoff, | ||
| }[name] | ||
| raise AttributeError(f"module {__name__!r} has no attribute {name!r}") |
There was a problem hiding this comment.
__getattr__ implementation does not cache the resolved symbol on the module's __dict__. PEP 562 only intercepts attribute lookups that miss __dict__, so from predictor import apply_investigation_handoff works (result is stored in the caller's namespace and __getattr__ is never called again for that caller), but any caller that accesses the symbol via direct module attribute access — e.g. predictor.apply_investigation_handoff(...) in a loop — will call __getattr__ on every access, constructing the dict each time. Pinning the resolved value into globals() (which is module.__dict__) after the first resolution is the standard PEP 562 pattern and eliminates the overhead.
| def __getattr__(name: str) -> Any: | |
| """Lazy-load investigation handoff so ``scoring`` can import ``vocabulary`` | |
| without a circular import (handoff imports ``_taxonomy_for_root_cause`` from | |
| ``scoring``).""" | |
| if name in _INVESTIGATION_HANDOFF_EXPORTS: | |
| from tests.benchmarks.cloudopsbench.predictor.investigation_handoff import ( | |
| align_predictions_to_investigation, | |
| apply_investigation_handoff, | |
| ) | |
| return { | |
| "align_predictions_to_investigation": align_predictions_to_investigation, | |
| "apply_investigation_handoff": apply_investigation_handoff, | |
| }[name] | |
| raise AttributeError(f"module {__name__!r} has no attribute {name!r}") | |
| def __getattr__(name: str) -> Any: | |
| """Lazy-load investigation handoff so ``scoring`` can import ``vocabulary`` | |
| without a circular import (handoff imports ``_taxonomy_for_root_cause`` from | |
| ``scoring``).""" | |
| if name in _INVESTIGATION_HANDOFF_EXPORTS: | |
| from tests.benchmarks.cloudopsbench.predictor.investigation_handoff import ( | |
| align_predictions_to_investigation, | |
| apply_investigation_handoff, | |
| ) | |
| _attrs = { | |
| "align_predictions_to_investigation": align_predictions_to_investigation, | |
| "apply_investigation_handoff": apply_investigation_handoff, | |
| } | |
| # Cache each resolved symbol so subsequent direct module-attribute | |
| # accesses (e.g. ``predictor.apply_investigation_handoff(...)``) skip | |
| # __getattr__ entirely — standard PEP 562 pattern. | |
| globals().update(_attrs) | |
| return _attrs[name] | |
| raise AttributeError(f"module {__name__!r} has no attribute {name!r}") |
|
@greptile review |
|
💼 Interviewer: describe a time you shipped something impactful. @YauhenBichel: points at this PR Interviewer: you're hired. 🤝
👋 Join us on Discord - OpenSRE : hang out, contribute, or hunt for features and issues. Everyone's welcome. |
Fixes #2074
Describe the changes you have made in this PR -
tests/benchmarks/cloudopsbench/predictor/__init__.pywas eagerlyre-exporting two names from
predictor.investigation_handoff. After the recent vocabularyrefactor, the import chain became:
predictor/init.py
→ investigation_handoff
→ scoring._taxonomy_for_root_cause
→ vocabulary
→ (back through scoring) → circular
This PR replaces the eager re-export with a PEP 562
__getattr__lazyloader, so
apply_investigation_handoffandalign_predictions_to_investigationare resolved on first accessinstead of at module load time. The cycle never executes.
Code Understanding and AI Usage
Did you use AI assistance (ChatGPT, Claude, Copilot, etc.) to write any part of this code?
If you used AI assistance:
Explain your implementation approach:
Checklist before requesting a review
Note: Please check Allow edits from maintainers if you would like us to assist in the PR.