fix: remove global logging monkey-patch from memory.py#1268
fix: remove global logging monkey-patch from memory.py#1268praisonai-triage-agent[bot] wants to merge 2 commits intomainfrom
Conversation
Remove logging.Logger.trace monkey-patch that violated protocol-driven architecture by modifying global state at import time. The trace functionality remains available locally within the memory module. This addresses one architectural violation from issue #1262 while maintaining backward compatibility and keeping changes minimal. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
|
@coderabbitai review |
|
/review |
|
@gemini review this PR |
✅ Actions performedReview triggered.
|
Code Review by Qodo
1. Trace method now missing
|
📝 WalkthroughWalkthroughThis PR removes the global monkey-patching of Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related PRs
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 docstrings
🧪 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 |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| if self.isEnabledFor(TRACE_LEVEL): | ||
| self._log(TRACE_LEVEL, message, args, **kwargs) | ||
|
|
||
| logging.Logger.trace = trace | ||
| # Note: trace method available locally, but not monkey-patched globally | ||
|
|
There was a problem hiding this comment.
1. Trace method now missing 🐞 Bug ≡ Correctness
After removing the global logging.Logger.trace monkey-patch, memory.py still calls logger.trace(...), which will raise AttributeError at runtime on the embedding-storage path. The local trace(self, ...) function is defined but never bound to the module logger instance returned by get_logger().
Agent Prompt
### Issue description
The PR removes the global `logging.Logger.trace` monkey-patch, but `memory.py` (and an internal test) still expects `trace` to exist on the logger. This causes `AttributeError` when executing `logger.trace(...)`.
### Issue Context
`get_logger()` returns a standard `logging.Logger` or `logging.LoggerAdapter` without a `trace` method. `memory.py` defines `TRACE_LEVEL` and a `trace(self, ...)` function, but does not bind it to the module logger.
### Fix Focus Areas
- src/praisonai-agents/praisonaiagents/memory/memory.py[20-34]
- src/praisonai-agents/praisonaiagents/memory/memory.py[939-956]
- src/praisonai-agents/tests/test_embedding_logging.py[48-65]
### Suggested fix approach (choose one)
1) **Replace call sites**: change `logger.trace(msg)` to `logger.log(TRACE_LEVEL, msg)` (and update the test accordingly).
2) **Attach trace to this module’s logger only** (no global class patch): after creating `logger`, attach a `trace` method to the *logger instance* (and/or its underlying `logger.logger` when it’s a `LoggerAdapter`). Ensure both `logger.trace(...)` and `logger.logger.trace(...)` work if you need to keep the existing test behavior.
Either approach preserves the architectural goal (no global mutation) while preventing runtime crashes.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above first. Review areas:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes the global monkey-patching of the logging.Logger class with a trace method in memory.py. A critical issue was identified where this removal would lead to AttributeError exceptions in parts of the code that still call logger.trace(). It is recommended to bind the trace method locally to the logger instance to maintain functionality without affecting the global state.
| self._log(TRACE_LEVEL, message, args, **kwargs) | ||
|
|
||
| logging.Logger.trace = trace | ||
| # Note: trace method available locally, but not monkey-patched globally |
There was a problem hiding this comment.
Removing the global monkey-patch logging.Logger.trace = trace will cause an AttributeError when logger.trace() is called (e.g., on lines 945 and 955), as the local logger instance (which is a LoggerAdapter) does not have a trace method. To maintain the functionality locally without affecting the global state, you should bind the trace method to the local logger instance.
| # Note: trace method available locally, but not monkey-patched globally | |
| logger.trace = lambda message, *args, **kwargs: logger.log(TRACE_LEVEL, message, *args, **kwargs) |
…ng monkey-patch Agent-Logs-Url: https://github.com/MervinPraison/PraisonAI/sessions/56f044b6-1424-4985-b09b-afe047dadd8b Co-authored-by: MervinPraison <454862+MervinPraison@users.noreply.github.com>
Review completed. One real bug was found and fixed (commit Critical – After removing the Fix: replaced both with Other review areas — no further issues:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/praisonai-agents/praisonaiagents/memory/memory.py`:
- Line 33: The _ExtraDataAdapter returned by get_logger lacks a trace() method
so calls to logger.trace(...) (e.g., in memory.py) raise AttributeError; fix by
either adding trace support to the logging stack or changing the callers: add a
trace method to the base Logger in _logging.py (attach logging.Logger.trace =
trace or create a small wrapper that calls self.log(TRACE_LEVEL,...)) or extend
_ExtraDataAdapter to forward trace(...) to its .logger (delegate to
self.logger.trace or self.logger.log(TRACE_LEVEL,...)); alternatively replace
all logger.trace(...) calls with logger.log(TRACE_LEVEL, ...) — update whichever
approach you choose consistently so memory.py's logger.trace calls no longer
fail.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 88c80196-6336-4921-918c-17198015dfe8
📒 Files selected for processing (1)
src/praisonai-agents/praisonaiagents/memory/memory.py
| self._log(TRACE_LEVEL, message, args, **kwargs) | ||
|
|
||
| logging.Logger.trace = trace | ||
| # Note: trace method available locally, but not monkey-patched globally |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate logging implementation files =="
fd -i '_logging.py'
echo
echo "== Check whether trace support is defined/attached in logging implementation =="
fd -i '_logging.py' -x rg -n -C3 '\bdef\s+get_logger\b|\bdef\s+trace\b|TRACE_LEVEL|addLevelName|setLoggerClass|LoggerAdapter'
echo
echo "== Confirm trace call sites in memory module =="
rg -n -C2 '\blogger\.trace\s*\(' src/praisonai-agents/praisonaiagents/memory/memory.pyRepository: MervinPraison/PraisonAI
Length of output: 3575
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Read _logging.py to find trace attachment ==="
head -n 100 src/praisonai-agents/praisonaiagents/_logging.py
echo
echo "=== Search for trace attachment in _logging.py ==="
rg -n 'trace|TRACE_LEVEL|addLevelName' src/praisonai-agents/praisonaiagents/_logging.py
echo
echo "=== Check memory.py imports and logger initialization ==="
head -n 50 src/praisonai-agents/praisonaiagents/memory/memory.py
echo
echo "=== Verify logger object type and trace availability ==="
rg -n -B2 -A5 'logger\s*=' src/praisonai-agents/praisonaiagents/memory/memory.py | head -n 30Repository: MervinPraison/PraisonAI
Length of output: 5785
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Read lines 25-100 of memory.py to see trace attachment ==="
sed -n '25,100p' src/praisonai-agents/praisonaiagents/memory/memory.py
echo
echo "=== Search for setattr, monkey-patch, or trace binding in memory.py ==="
rg -n 'setattr|monkey|trace.*=|logger\s*\.' src/praisonai-agents/praisonaiagents/memory/memory.py | head -n 40Repository: MervinPraison/PraisonAI
Length of output: 5546
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for get_logger function in _logging.py ==="
rg -n -A 50 'def get_logger' src/praisonai-agents/praisonaiagents/_logging.py
echo
echo "=== Search for trace method definition/attachment in _logging.py ==="
rg -n -B3 -A5 'def trace|setattr.*trace|addLevelName.*TRACE|trace.*=' src/praisonai-agents/praisonaiagents/_logging.pyRepository: MervinPraison/PraisonAI
Length of output: 2702
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Continue reading get_logger function (lines 291-320) ==="
sed -n '291,320p' src/praisonai-agents/praisonaiagents/_logging.py
echo
echo "=== Search for any trace attachment after get_logger definition ==="
sed -n '241,330p' src/praisonai-agents/praisonaiagents/_logging.py | tail -n 50Repository: MervinPraison/PraisonAI
Length of output: 2560
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search entire memory.py for trace attachment/binding ==="
rg -n 'setattr|\.trace\s*=|trace.*=' src/praisonai-agents/praisonaiagents/memory/memory.py
echo
echo "=== Check if logger object is modified after initialization ==="
sed -n '21,250p' src/praisonai-agents/praisonaiagents/memory/memory.py | grep -n -A2 -B2 'logger\|trace'
echo
echo "=== Check _ExtraDataAdapter class for trace support ==="
rg -n -B5 -A20 'class _ExtraDataAdapter' src/praisonai-agents/praisonaiagents/_logging.pyRepository: MervinPraison/PraisonAI
Length of output: 127
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check lines around 945 and 955 where logger.trace is called ==="
sed -n '940,960p' src/praisonai-agents/praisonaiagents/memory/memory.py
echo
echo "=== Check full _ExtraDataAdapter class ==="
rg -n -B3 -A30 'class _ExtraDataAdapter' src/praisonai-agents/praisonaiagents/_logging.py
echo
echo "=== Check if trace is added to logging.Logger anywhere ==="
rg -n 'logging\.Logger' src/praisonai-agents/praisonaiagents/_logging.py
rg -n 'logging\.Logger' src/praisonai-agents/praisonaiagents/memory/memory.pyRepository: MervinPraison/PraisonAI
Length of output: 2861
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check LoggerAdapter implementation for method delegation ==="
python3 << 'EOF'
import logging
import inspect
# Check if LoggerAdapter has __getattr__ for method delegation
adapter = logging.LoggerAdapter(logging.getLogger('test'), {})
print("LoggerAdapter methods that might delegate:")
print([m for m in dir(adapter) if not m.startswith('_')])
print()
print("LoggerAdapter __getattr__ defined?", hasattr(logging.LoggerAdapter, '__getattr__'))
print("LoggerAdapter source file:", inspect.getfile(logging.LoggerAdapter))
EOF
echo
echo "=== Verify what happens when trace is called on LoggerAdapter ==="
sed -n '1,50p' src/praisonai-agents/praisonaiagents/memory/memory.py | grep -A3 'logger\s*='Repository: MervinPraison/PraisonAI
Length of output: 663
🏁 Script executed:
#!/bin/bash
set -euo zipfail
echo "=== Search for any global monkey-patch of logging.Logger.trace ==="
rg -n 'logging\.Logger\s*\.\s*trace|setattr.*logging.*trace|logging\.Logger\.trace' src/praisonai-agents/
echo
echo "=== Check entire _logging.py for trace setup ==="
wc -l src/praisonai-agents/praisonaiagents/_logging.py
rg -n 'trace' src/praisonai-agents/praisonaiagents/_logging.py
echo
echo "=== Check if there's a conftest or init that sets up trace globally ==="
find src/praisonai-agents -name 'conftest.py' -o -name '__init__.py' | head -5
rg -n 'logging\.Logger\.trace|setattr.*Logger.*trace' src/praisonai-agents/praisonaiagents/__init__.py 2>/dev/null || echo "No trace found in __init__.py"Repository: MervinPraison/PraisonAI
Length of output: 119
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for any global monkey-patch of logging.Logger.trace ==="
rg -n 'logging\.Logger\s*\.\s*trace|setattr.*logging.*trace|logging\.Logger\.trace' src/praisonai-agents/
echo
echo "=== Check entire _logging.py for trace setup ==="
wc -l src/praisonai-agents/praisonaiagents/_logging.py
rg -n 'trace' src/praisonai-agents/praisonaiagents/_logging.py
echo
echo "=== Check praisonaiagents __init__.py ==="
cat src/praisonai-agents/praisonaiagents/__init__.py | head -50Repository: MervinPraison/PraisonAI
Length of output: 133
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== List all files in praisonaiagents to find other logging setup ==="
find src/praisonai-agents/praisonaiagents -name '*.py' -type f | grep -E '(log|trace|event)' | head -20
echo
echo "=== Check if trace() is ever attached to logger via monkey-patch in memory.py ==="
grep -n 'setattr\|logger\.' src/praisonai-agents/praisonaiagents/memory/memory.py | grep -i trace
echo
echo "=== Run a direct test: can LoggerAdapter call trace? ==="
python3 << 'EOF'
import logging
# Create LoggerAdapter
base_logger = logging.getLogger('test')
adapter = logging.LoggerAdapter(base_logger, {})
# Try calling trace (should fail since trace doesn't exist)
try:
adapter.trace("test message")
print("SUCCESS: adapter.trace() worked")
except AttributeError as e:
print(f"ERROR: {e}")
# Try calling trace on base logger
try:
base_logger.trace("test message")
print("SUCCESS: base_logger.trace() worked")
except AttributeError as e:
print(f"ERROR on base_logger: {e}")
EOFRepository: MervinPraison/PraisonAI
Length of output: 1223
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check if logging_plugin.py attaches trace ==="
cat src/praisonai-agents/praisonaiagents/plugins/builtin/logging_plugin.py
echo
echo "=== Check streaming/logging.py ==="
cat src/praisonai-agents/praisonaiagents/streaming/logging.py | head -100
echo
echo "=== Check if there's an initialization that patches Logger.trace ==="
rg -n 'Logger\.trace' src/praisonai-agents/ || echo "No Logger.trace found"Repository: MervinPraison/PraisonAI
Length of output: 7580
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Confirm: memory.py logger is LoggerAdapter (because extra_data is passed) ==="
python3 << 'EOF'
import sys
sys.path.insert(0, 'src/praisonai-agents')
from praisonaiagents._logging import get_logger
# Test 1: Without extra_data (returns Logger)
logger1 = get_logger("test.module1")
print(f"Without extra_data: {type(logger1).__name__}")
# Test 2: With extra_data (returns LoggerAdapter)
logger2 = get_logger("test.module2", extra_data={"subsystem": "memory"})
print(f"With extra_data: {type(logger2).__name__}")
# Test 3: Try calling trace on LoggerAdapter
try:
logger2.trace("test")
print("trace() succeeded")
except AttributeError as e:
print(f"trace() failed: {e}")
EOF
echo
echo "=== Verify the exact type returned at memory.py line 23 ==="
sed -n '21,25p' src/praisonai-agents/praisonaiagents/memory/memory.pyRepository: MervinPraison/PraisonAI
Length of output: 544
Runtime AttributeError on logger.trace() calls — logger is _ExtraDataAdapter which lacks trace method.
The logger at line 23 is a _ExtraDataAdapter (because extra_data is passed to get_logger()), and LoggerAdapter does not have a trace() method. Lines 945 and 955 call logger.trace(...), which will crash with AttributeError: '_ExtraDataAdapter' object has no attribute 'trace' when executed.
The trace() function defined at lines 29–32 is never attached to the logger and remains orphaned. The comment at line 33 is misleading—trace is neither globally monkey-patched nor locally available on the logger instance.
Fix: Either attach trace to the underlying logger in _logging.py via logging.Logger.trace = trace, replace calls with logger.log(TRACE_LEVEL, ...), or extend _ExtraDataAdapter to delegate trace to the wrapped logger.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai-agents/praisonaiagents/memory/memory.py` at line 33, The
_ExtraDataAdapter returned by get_logger lacks a trace() method so calls to
logger.trace(...) (e.g., in memory.py) raise AttributeError; fix by either
adding trace support to the logging stack or changing the callers: add a trace
method to the base Logger in _logging.py (attach logging.Logger.trace = trace or
create a small wrapper that calls self.log(TRACE_LEVEL,...)) or extend
_ExtraDataAdapter to forward trace(...) to its .logger (delegate to
self.logger.trace or self.logger.log(TRACE_LEVEL,...)); alternatively replace
all logger.trace(...) calls with logger.log(TRACE_LEVEL, ...) — update whichever
approach you choose consistently so memory.py's logger.trace calls no longer
fail.
logging.Logger.trace = traceglobal monkey-patchlogger.trace()calls left by the removal — replaced withlogger.log(TRACE_LEVEL, ...)(standard Python logging API for custom levels)trace()helper function that was no longer bound to any loggerimport praisonaiagentssucceeds