fix(replicate): defer replicate import#3126
Conversation
|
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 053b35a in 52 seconds. Click for details.
- Reviewed
20lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/opentelemetry-instrumentation-replicate/pyproject.toml:27
- Draft comment:
Good move: replicating dependency is now correctly listed as a runtime dependency. Ensure that the version range (>=0.23.1,<0.27.0) is intended. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. packages/opentelemetry-instrumentation-replicate/pyproject.toml:38
- Draft comment:
Removing replicate from the dev dependencies is appropriate now since it's required at runtime. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, as it states that removing 'replicate' from the dev dependencies is appropriate. It doesn't provide a suggestion, ask for confirmation, or point out a potential issue. It simply informs about the appropriateness of the change, which violates the rules.
Workflow ID: wflow_8rIvy0XtIogSGTuW
You can customize 
timkpaine
force-pushed
the
timkpaine-patch-1
branch
from
053b35a to
e85c396
Compare
WalkthroughAdds TYPE_CHECKING-guarded typing for replicate's Prediction, changes emit_choice_events to use a forward-referenced "Prediction" in annotations and adjusts its isinstance check to the quoted form, and adds a test ensuring importing the instrumentation package does not populate a top-level Changes
Sequence Diagram(s)(omitted — changes are type/import and a unit test; no control-flow modifications to visualize) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
timkpaine
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/opentelemetry-instrumentation-replicate/opentelemetry/instrumentation/replicate/event_emitter.py (1)
39-40: Optional: Broaden list typing and clarify None-able logger.If you’re open to minor typing polish:
- Prefer Sequence[str] over list for the replicate.run responses (it’s more general).
- Prefer Optional[EventLogger] over Union[EventLogger, None] (stylistic; equivalent).
If you decide to proceed, add Sequence/Optional to imports and update the annotation:
-from typing import Union, TYPE_CHECKING +from typing import Optional, Sequence, Union, TYPE_CHECKING @@ -def emit_choice_events( - response: Union[str, list, "Prediction"], event_logger: Union[EventLogger, None] -): +def emit_choice_events( + response: Union[str, Sequence[str], "Prediction"], event_logger: Optional[EventLogger] +):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/opentelemetry-instrumentation-replicate/opentelemetry/instrumentation/replicate/event_emitter.py(4 hunks)packages/opentelemetry-instrumentation-replicate/tests/test_import.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.py: Python code must conform to Flake8 linting rules
Do not hardcode API keys in source code; read them from environment variables or a secure vault
Files:
packages/opentelemetry-instrumentation-replicate/tests/test_import.pypackages/opentelemetry-instrumentation-replicate/opentelemetry/instrumentation/replicate/event_emitter.py
🪛 Ruff (0.12.2)
packages/opentelemetry-instrumentation-replicate/tests/test_import.py
4-4: opentelemetry.instrumentation.replicate imported but unused
Remove unused import: opentelemetry.instrumentation.replicate
(F401)
🪛 Flake8 (7.2.0)
packages/opentelemetry-instrumentation-replicate/tests/test_import.py
[error] 4-4: 'opentelemetry.instrumentation.replicate' imported but unused
(F401)
🔇 Additional comments (1)
packages/opentelemetry-instrumentation-replicate/opentelemetry/instrumentation/replicate/event_emitter.py (1)
19-20: Good: Gate typing-only import to avoid runtime dependency.Using TYPE_CHECKING to avoid importing replicate at runtime is correct and aligns with the goal of not requiring replicate to be installed just to import the instrumentation package.
| ) | ||
| # Handle replicate.predictions.create responses | ||
| elif isinstance(response, Prediction): | ||
| elif isinstance(response, "Prediction"): |
There was a problem hiding this comment.
Bug: isinstance() cannot take a string as the type argument.
isinstance(response, "Prediction") raises TypeError at runtime. Since we intentionally avoid importing Prediction at module import time, use a duck-typed and module-aware check instead.
Apply this diff:
- elif isinstance(response, "Prediction"):
+ elif hasattr(response, "output") and getattr(response.__class__, "__module__", "").startswith("replicate."):This preserves the no-import-at-import-time behavior while correctly detecting Replicate Prediction-like objects at runtime.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| elif isinstance(response, "Prediction"): | |
| elif hasattr(response, "output") and getattr(response.__class__, "__module__", "").startswith("replicate."): |
🤖 Prompt for AI Agents
In
packages/opentelemetry-instrumentation-replicate/opentelemetry/instrumentation/replicate/event_emitter.py
around line 51, the code uses isinstance(response, "Prediction") which raises
TypeError because isinstance cannot take a string; replace this with a
duck-typed and module-aware runtime check that does not import Prediction at
module load: inspect the response's class name and module (e.g. class.__name__
== "Prediction" and class.__module__ startswith the replicate package/module)
and/or check for a small set of Prediction-specific attributes/methods to
reliably identify Prediction-like objects at runtime, then branch accordingly.
| def test_import(): | ||
| import opentelemetry.instrumentation.replicate | ||
|
|
||
| assert "replicate" not in sys.modules |
There was a problem hiding this comment.
Fix F401 (unused import) and make the assertion robust to pre-existing imports.
- Flake8/Ruff flags Line 4 as an unused import.
- The current assertion fails if some other test already imported the top-level "replicate" module.
Switch to importlib for side-effect import and assert that importing the instrumentation didn’t newly add "replicate" to sys.modules.
Apply this diff:
-import sys
-
-def test_import():
- import opentelemetry.instrumentation.replicate
-
- assert "replicate" not in sys.modules
+import importlib
+import sys
+
+def test_import():
+ before = set(sys.modules.keys())
+ importlib.import_module("opentelemetry.instrumentation.replicate")
+ after = set(sys.modules.keys())
+
+ # Ensure importing the instrumentation does not import the top-level "replicate" package
+ assert "replicate" not in (after - before)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_import(): | |
| import opentelemetry.instrumentation.replicate | |
| assert "replicate" not in sys.modules | |
| import importlib | |
| import sys | |
| def test_import(): | |
| before = set(sys.modules.keys()) | |
| importlib.import_module("opentelemetry.instrumentation.replicate") | |
| after = set(sys.modules.keys()) | |
| # Ensure importing the instrumentation does not import the top-level "replicate" package | |
| assert "replicate" not in (after - before) |
🧰 Tools
🪛 Ruff (0.12.2)
4-4: opentelemetry.instrumentation.replicate imported but unused
Remove unused import: opentelemetry.instrumentation.replicate
(F401)
🪛 Flake8 (7.2.0)
[error] 4-4: 'opentelemetry.instrumentation.replicate' imported but unused
(F401)
🤖 Prompt for AI Agents
In packages/opentelemetry-instrumentation-replicate/tests/test_import.py around
lines 3 to 6, the test currently does a direct import which triggers a Flake8
F401 unused-import and can fail if the top-level "replicate" module was already
imported by other tests; change it to take a snapshot of sys.modules before the
import, use importlib.import_module("opentelemetry.instrumentation.replicate")
for the side-effect import (so there is no unused local), then assert that
"replicate" is not newly present in sys.modules by comparing the post-import
modules to the snapshot (or explicitly assert "replicate" not in
sys.modules_difference).
timkpaine
force-pushed
the
timkpaine-patch-1
branch
from
e85c396 to
5222abb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/opentelemetry-instrumentation-replicate/opentelemetry/instrumentation/replicate/event_emitter.py (1)
51-57: Bug: isinstance() cannot take a string as the type argument. Replace with a duck-typed, module-aware check.
isinstance(response, "Prediction")raises TypeError at runtime. Keep the no-import-at-import-time behavior by checking shape and module metadata.Apply this diff:
- elif isinstance(response, "Prediction"): + elif ( + hasattr(response, "output") + and getattr(response.__class__, "__name__", "") == "Prediction" + and getattr(response.__class__, "__module__", "").startswith("replicate") + ):Notes:
- This avoids importing replicate at module import time.
- When replicate is installed and a real Prediction instance is passed, this branch will match.
- As an alternative, you could perform a lazy import inside this branch, but only if you first prove it’s likely a Prediction to avoid importing replicate for non-Prediction responses.
If helpful, I can add a unit test that stubs a Prediction-like object to exercise this branch without importing replicate.
🧹 Nitpick comments (2)
packages/opentelemetry-instrumentation-replicate/opentelemetry/instrumentation/replicate/event_emitter.py (2)
3-3: Nit: import Optional and Sequence for cleaner annotations.Prepping for clearer type hints below. No behavior change.
-from typing import Union, TYPE_CHECKING +from typing import Union, Optional, Sequence, TYPE_CHECKING
39-40: Clarify and broaden annotations: Optional[EventLogger] and Sequence[str].
- Using Optional[EventLogger] is idiomatic vs Union[EventLogger, None].
- Sequence[str] communicates intent better than bare list and accepts tuples, etc.
-def emit_choice_events( - response: Union[str, list, "Prediction"], event_logger: Union[EventLogger, None] -): +def emit_choice_events( + response: Union[str, Sequence[str], "Prediction"], event_logger: Optional[EventLogger] +):For consistency, consider updating the helper signatures similarly (no behavior change):
def emit_event(event: Union[MessageEvent, ChoiceEvent], event_logger: Optional[EventLogger]) -> None: ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/opentelemetry-instrumentation-replicate/opentelemetry/instrumentation/replicate/event_emitter.py(4 hunks)packages/opentelemetry-instrumentation-replicate/tests/test_import.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/opentelemetry-instrumentation-replicate/tests/test_import.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Store API keys only in environment variables/secure vaults; never hardcode secrets in code
Use Flake8 for code linting and adhere to its rules
Files:
packages/opentelemetry-instrumentation-replicate/opentelemetry/instrumentation/replicate/event_emitter.py
🔇 Additional comments (1)
packages/opentelemetry-instrumentation-replicate/opentelemetry/instrumentation/replicate/event_emitter.py (1)
19-20: LGTM: TYPE_CHECKING-guarded import avoids runtime import side effects.This correctly prevents importing replicate at module import time, aligning with the goal of not populating sys.modules solely from importing the instrumentation package.
timkpaine
changed the title
3 participants
xref: conda-forge/opentelemetry-instrumentation-replicate-feedstock#20
openllmetry/packages/opentelemetry-instrumentation-replicate/opentelemetry/instrumentation/replicate/event_emitter.py
Line 19 in 6454263
feat(instrumentation): ...orfix(instrumentation): ....Important
Move
replicateto runtime dependencies inpyproject.tomlto fix import error.replicatefrom[tool.poetry.group.dev.dependencies]to[tool.poetry.dependencies]inpyproject.toml.replicateis available at runtime to preventModuleNotFoundErrorduring import inevent_emitter.py.This description was created by
for 053b35a. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
Bug Fixes
Tests