fix(watsonx): Package Import Error Fix#3737
Conversation
|
Fix for Issue #3735 |
📝 WalkthroughWalkthroughThe instrumentation now checks module availability before wrapping functions, attempting explicit imports and gracefully skipping instrumentation if modules are unavailable. ImportError exceptions trigger debug logging instead of failures, preserving functionality for present modules while avoiding crashes for missing ones. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/opentelemetry-instrumentation-watsonx/opentelemetry/instrumentation/watsonx/__init__.py`:
- Around line 714-735: Separate the module-probing from the wrapping call: first
attempt to import the module and only catch ModuleNotFoundError (not
ImportError) to decide to skip instrumentation (use __import__(wrap_module) or
importlib.util.find_spec and on ModuleNotFoundError call logger.debug(f"Module
{wrap_module} not found, skipping instrumentation") and continue); then, outside
that import check, call wrap_function_wrapper(wrap_module,
f"{wrap_object}.{wrap_method}", _wrap(...)) so that any other import-related
exceptions or errors raised during wrapping aren’t masked—optionally add a
distinct try/except around wrap_function_wrapper to log and re-raise unexpected
errors rather than treating them as "module not found."
- Around line 711-735: _instrument currently skips modules that fail to import
but _uninstrument blindly unwraps every entry in WATSON_MODULES causing
ImportError on missing modules; fix by tracking successful wraps (e.g., add a
set or list like self._wrapped_pairs or self._wrapped_modules and append the
tuple (wrap_module, f"{wrap_object}.{wrap_method}") when wrap_function_wrapper
succeeds) and change _uninstrument to iterate only over that tracked collection
and call unwrap on those entries (or guard import_module calls with try/except).
Update references to wrap_function_wrapper and unwrap to use the tracked names
so only actually-wrapped modules are unwrapped.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
packages/opentelemetry-instrumentation-watsonx/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
packages/opentelemetry-instrumentation-watsonx/opentelemetry/instrumentation/watsonx/__init__.py
| if not wrap_module: | ||
| continue | ||
|
|
||
| try: | ||
| # Try to import the module first to check if it exists | ||
| __import__(wrap_module) | ||
| wrap_function_wrapper( | ||
| wrap_module, | ||
| f"{wrap_object}.{wrap_method}", | ||
| _wrap( | ||
| tracer, | ||
| wrapped_method, | ||
| token_histogram, | ||
| response_counter, | ||
| duration_histogram, | ||
| exception_counter, | ||
| event_logger, | ||
| ), | ||
| ) | ||
| except ImportError: | ||
| # Module not installed, skip instrumentation for this module | ||
| logger.debug( | ||
| f"Module {wrap_module} not found, skipping instrumentation" | ||
| ) | ||
| continue |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd packages/opentelemetry-instrumentation-watsonx && wc -l opentelemetry/instrumentation/watsonx/__init__.pyRepository: traceloop/openllmetry
Length of output: 118
🏁 Script executed:
cd packages/opentelemetry-instrumentation-watsonx && sed -n '1,50p' opentelemetry/instrumentation/watsonx/__init__.pyRepository: traceloop/openllmetry
Length of output: 1714
🏁 Script executed:
cd packages/opentelemetry-instrumentation-watsonx && sed -n '700,750p' opentelemetry/instrumentation/watsonx/__init__.pyRepository: traceloop/openllmetry
Length of output: 1882
🏁 Script executed:
cd packages/opentelemetry-instrumentation-watsonx && grep -n "_uninstrument" opentelemetry/instrumentation/watsonx/__init__.py | head -20Repository: traceloop/openllmetry
Length of output: 107
🏁 Script executed:
cd packages/opentelemetry-instrumentation-watsonx && grep -r "def unwrap" .Repository: traceloop/openllmetry
Length of output: 47
🏁 Script executed:
# Search for unwrap in OpenTelemetry utils to understand its behavior
fd -t f "utils.py" | xargs grep -l "def unwrap" 2>/dev/null | head -5Repository: traceloop/openllmetry
Length of output: 47
🏁 Script executed:
python3 << 'EOF'
# Let's try to understand the unwrap function from opentelemetry
# by checking if it exists and examining its signature
try:
from opentelemetry.instrumentation.utils import unwrap
import inspect
print("unwrap function signature:")
print(inspect.signature(unwrap))
print("\nDocstring:")
print(inspect.getdoc(unwrap))
except Exception as e:
print(f"Error: {e}")
EOFRepository: traceloop/openllmetry
Length of output: 445
🌐 Web query:
OpenTelemetry unwrap function behavior when module not installed site:github.com/open-telemetry
💡 Result:
In OpenTelemetry JS, the unwrap you’re seeing is the “shimmer” helper used to remove monkey-patches from an object’s function property.
What happens if the target module/function isn’t present?
unwrap(nodule, name)does not throw ifnoduleis falsy ornodule[name]doesn’t exist; it just logs"no function to unwrap."(and a stack trace via the configured shimmer logger) and returns. [1]- If
nodule[name]exists but isn’t a shimmer-wrapped function (i.e., it has no__unwrap),unwrapagain does not throw; it logs"no original to unwrap to -- has <name> already been unwrapped?"and returns. [1]
What happens if the module package isn’t installed?
In the Node InstrumentationBase, the code that checks whether a module was preloaded uses require.resolve(name) inside a try/catch. If require.resolve fails (e.g., module not installed / not resolvable), it silently skips (no warning, no crash). [2]
Sources:
[1] experimental/packages/opentelemetry-instrumentation/src/shimmer.ts (shimmer unwrap implementation) (raw.githubusercontent.com)
[2] experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts (_warnOnPreloadedModules handling of missing modules) (raw.githubusercontent.com)
Citations:
- 1: https://raw.githubusercontent.com/open-telemetry/opentelemetry-js/main/experimental/packages/opentelemetry-instrumentation/src/shimmer.ts
- 2: https://raw.githubusercontent.com/open-telemetry/opentelemetry-js/main/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts
🏁 Script executed:
python3 << 'EOF'
# Let's inspect the actual unwrap implementation
from opentelemetry.instrumentation.utils import unwrap
import inspect
# Get the source code
try:
source = inspect.getsource(unwrap)
print(source)
except Exception as e:
print(f"Could not get source: {e}")
# Try to get the file location
print(f"File location: {inspect.getfile(unwrap)}")
EOFRepository: traceloop/openllmetry
Length of output: 1115
🏁 Script executed:
# Let's find the instrumentation utils file and examine unwrap
find /usr -name "utils.py" -path "*/instrumentation/*" 2>/dev/null | head -3Repository: traceloop/openllmetry
Length of output: 143
🏁 Script executed:
# Try to examine the installed opentelemetry-instrumentation package
python3 -c "import opentelemetry.instrumentation.utils as u; import os; print(os.path.dirname(u.__file__))"Repository: traceloop/openllmetry
Length of output: 134
🏁 Script executed:
# List files in the instrumentation utils directory
python3 << 'EOF'
import opentelemetry.instrumentation.utils as u
import os
utils_dir = os.path.dirname(u.__file__)
print(f"Utils directory: {utils_dir}")
if os.path.isdir(utils_dir):
for f in os.listdir(utils_dir):
print(f)
EOFRepository: traceloop/openllmetry
Length of output: 378
🏁 Script executed:
cd packages/opentelemetry-instrumentation-watsonx && sed -n '737,742p' opentelemetry/instrumentation/watsonx/__init__.pyRepository: traceloop/openllmetry
Length of output: 404
🏁 Script executed:
# Check if there's any try/catch around the _uninstrument unwrap calls
cd packages/opentelemetry-instrumentation-watsonx && sed -n '730,742p' opentelemetry/instrumentation/watsonx/__init__.pyRepository: traceloop/openllmetry
Length of output: 691
🏁 Script executed:
# Check the import_module function to confirm it raises on missing modules
python3 << 'EOF'
from importlib import import_module
# Try importing a non-existent module to see what happens
try:
import_module("nonexistent_module_xyz")
except Exception as e:
print(f"Exception type: {type(e).__name__}")
print(f"Exception message: {e}")
EOFRepository: traceloop/openllmetry
Length of output: 160
Track successful wraps so uninstrumenting stays safe.
During _instrument(), modules that fail to import are gracefully skipped with a debug log (lines 723–729). However, _uninstrument() unconditionally unwraps all items from the static WATSON_MODULES list. The unwrap() function will raise an ImportError when attempting to unwrap a module that doesn't exist, as it internally calls import_module() which raises ModuleNotFoundError. In partial-install environments where some Watson ML submodules are unavailable, this causes _uninstrument() to fail.
Proposed fix
class WatsonxInstrumentor(BaseInstrumentor):
@@
def __init__(self, exception_logger=None, use_legacy_attributes: bool = True):
super().__init__()
Config.exception_logger = exception_logger
Config.use_legacy_attributes = use_legacy_attributes
+ self._wrapped_targets = []
@@
wrap_function_wrapper(
@@
)
+ self._wrapped_targets.append(
+ (wrap_module, wrap_object, wrap_method)
+ )
@@
def _uninstrument(self, **kwargs):
- for wrapped_methods in WATSON_MODULES:
- for wrapped_method in wrapped_methods:
- wrap_module = wrapped_method.get("module")
- wrap_object = wrapped_method.get("object")
- unwrap(f"{wrap_module}.{wrap_object}", wrapped_method.get("method"))
+ for wrap_module, wrap_object, wrap_method in self._wrapped_targets:
+ unwrap(f"{wrap_module}.{wrap_object}", wrap_method)
+ self._wrapped_targets.clear()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/opentelemetry-instrumentation-watsonx/opentelemetry/instrumentation/watsonx/__init__.py`
around lines 711 - 735, _instrument currently skips modules that fail to import
but _uninstrument blindly unwraps every entry in WATSON_MODULES causing
ImportError on missing modules; fix by tracking successful wraps (e.g., add a
set or list like self._wrapped_pairs or self._wrapped_modules and append the
tuple (wrap_module, f"{wrap_object}.{wrap_method}") when wrap_function_wrapper
succeeds) and change _uninstrument to iterate only over that tracked collection
and call unwrap on those entries (or guard import_module calls with try/except).
Update references to wrap_function_wrapper and unwrap to use the tracked names
so only actually-wrapped modules are unwrapped.
| try: | ||
| # Try to import the module first to check if it exists | ||
| __import__(wrap_module) | ||
| wrap_function_wrapper( | ||
| wrap_module, | ||
| f"{wrap_object}.{wrap_method}", | ||
| _wrap( | ||
| tracer, | ||
| wrapped_method, | ||
| token_histogram, | ||
| response_counter, | ||
| duration_histogram, | ||
| exception_counter, | ||
| event_logger, | ||
| ), | ||
| ) | ||
| except ImportError: | ||
| # Module not installed, skip instrumentation for this module | ||
| logger.debug( | ||
| f"Module {wrap_module} not found, skipping instrumentation" | ||
| ) | ||
| continue |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
head -n 50 packages/opentelemetry-instrumentation-watsonx/opentelemetry/instrumentation/watsonx/__init__.pyRepository: traceloop/openllmetry
Length of output: 1714
🏁 Script executed:
wc -l packages/opentelemetry-instrumentation-watsonx/opentelemetry/instrumentation/watsonx/__init__.pyRepository: traceloop/openllmetry
Length of output: 165
🏁 Script executed:
sed -n '700,750p' packages/opentelemetry-instrumentation-watsonx/opentelemetry/instrumentation/watsonx/__init__.pyRepository: traceloop/openllmetry
Length of output: 1882
🏁 Script executed:
# Find the _instrument method to see full context
ast-grep --pattern 'def _instrument($_) { $$$ }'Repository: traceloop/openllmetry
Length of output: 47
🏁 Script executed:
# Search for _uninstrument method
rg -A 20 "def _uninstrument" packages/opentelemetry-instrumentation-watsonx/Repository: traceloop/openllmetry
Length of output: 986
🏁 Script executed:
# Check what wrap_function_wrapper is imported from
rg "from.*wrap_function_wrapper|import.*wrap_function_wrapper" packages/opentelemetry-instrumentation-watsonx/Repository: traceloop/openllmetry
Length of output: 201
🏁 Script executed:
# Check what exceptions wrap_function_wrapper raises
rg -i "wrap_function_wrapper" --type py | head -20Repository: traceloop/openllmetry
Length of output: 2805
🏁 Script executed:
# Look at opentelemetry.instrumentation.utils to understand unwrap behavior
python3 << 'EOF'
import sys
try:
from opentelemetry.instrumentation.utils import unwrap
import inspect
print("unwrap signature:", inspect.signature(unwrap))
print("\nunwrap source:")
print(inspect.getsource(unwrap))
except Exception as e:
print(f"Error: {e}")
print("unwrap may not be available in this environment")
EOFRepository: traceloop/openllmetry
Length of output: 1178
🏁 Script executed:
# Check if wrap_function_wrapper can raise ImportError by examining its behavior
python3 << 'EOF'
try:
from wrapt import wrap_function_wrapper
import inspect
print("wrap_function_wrapper signature:")
print(inspect.signature(wrap_function_wrapper))
except Exception as e:
print(f"Error: {e}")
EOFRepository: traceloop/openllmetry
Length of output: 121
🏁 Script executed:
# Look for any other instrumentation patterns in opentelemetry to understand best practices
rg "wrap_function_wrapper" packages/ -A 5 -B 2 | head -60Repository: traceloop/openllmetry
Length of output: 7395
🏁 Script executed:
# Check if there are tests that show what happens when wrap_function_wrapper fails
find packages/opentelemetry-instrumentation-watsonx -name "*test*" -type fRepository: traceloop/openllmetry
Length of output: 899
Separate module availability check from wrapping to avoid masking unrelated import failures.
At lines 714–735, the single try/except ImportError block combines module probing with wrap_function_wrapper(). This masks import failures unrelated to a missing module (e.g., errors in module dependencies or incorrect target attributes) and logs them misleadingly as "Module not found."
The proposed fix correctly separates module existence verification (which catches only ModuleNotFoundError) from the wrapping call, ensuring unrelated import errors are not silently skipped.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/opentelemetry-instrumentation-watsonx/opentelemetry/instrumentation/watsonx/__init__.py`
around lines 714 - 735, Separate the module-probing from the wrapping call:
first attempt to import the module and only catch ModuleNotFoundError (not
ImportError) to decide to skip instrumentation (use __import__(wrap_module) or
importlib.util.find_spec and on ModuleNotFoundError call logger.debug(f"Module
{wrap_module} not found, skipping instrumentation") and continue); then, outside
that import check, call wrap_function_wrapper(wrap_module,
f"{wrap_object}.{wrap_method}", _wrap(...)) so that any other import-related
exceptions or errors raised during wrapping aren’t masked—optionally add a
distinct try/except around wrap_function_wrapper to log and re-raise unexpected
errors rather than treating them as "module not found."
galkleinman
changed the title
| __import__(wrap_module) | ||
| wrap_function_wrapper( | ||
| wrap_module, | ||
| f"{wrap_object}.{wrap_method}", | ||
| _wrap( | ||
| tracer, | ||
| wrapped_method, | ||
| token_histogram, | ||
| response_counter, | ||
| duration_histogram, | ||
| exception_counter, | ||
| event_logger, | ||
| ), | ||
| ) | ||
| except ImportError: | ||
| # Module not installed, skip instrumentation for this module |
There was a problem hiding this comment.
We shouldn't import here. There's code in traceloop-sdk which is checking which packages are available and turns on their instrumentations accordingly/
2 participants
feat(instrumentation): ...orfix(instrumentation): ....Summary by CodeRabbit