feat(gh-183): add logging functionality #217
Conversation
WalkthroughThe changes introduce enhanced logging capabilities into the project. A new dependency ( Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Logger as Logger Module
participant Loguru as Loguru Library
participant Callback as jax.debug.callback
App->>Logger: enable_logging("DEBUG")
Logger->>Loguru: Remove existing handlers
Logger->>Loguru: Add new logging handler with config
App->>Logger: Log message (e.g., info, error)
Logger-->>Logger: Check if logging is enabled (global LOG)
alt Logging Enabled
Logger->>Callback: Invoke logging callback with message details
else Logging Disabled
Note over Logger,App: Skip logging action
end
Poem
✨ Finishing Touches
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:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/flowMC/logger/_logger.py (2)
109-123: Document the purpose ofoptionsmodification in the exception method.In the
exceptionmethod,options[0]is set toTruewithout explaining what this option controls. Consider adding a comment explaining the significance of this modification.- options = (True,) + __self._options[1:] + # Set the first option to True to capture the current exception + options = (True,) + __self._options[1:]
177-198: Consider adding unit tests for the logging functionality.The PR adds significant new functionality but no tests are visible in this changeset. Consider adding unit tests that verify:
- The logger works correctly with different log levels
- The integration with JAX's JIT works as expected
- The enable_logging function properly configures the logger
This will ensure the logging functionality remains robust as the codebase evolves.
Would you like me to suggest a basic structure for testing this logging functionality?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pyproject.toml(1 hunks)src/flowMC/logger/__init__.py(1 hunks)src/flowMC/logger/_logger.py(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/flowMC/logger/__init__.py (1)
src/flowMC/logger/_logger.py (1)
enable_logging(177-198)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build (3.12)
- GitHub Check: build (3.11)
- GitHub Check: pre-commit (3.12)
- GitHub Check: pre-commit (3.11)
🔇 Additional comments (4)
pyproject.toml (1)
26-26: LGTM! Appropriate dependency addition for enhanced logging.The addition of loguru with version constraint >=0.7.0 aligns perfectly with the PR objective of introducing enhanced logging capabilities. This is consistent with the project's dependency management approach.
src/flowMC/logger/__init__.py (1)
1-2: LGTM! Clean public API exposure.The explicit imports provide a well-defined public interface for the logging functionality, following Python's best practices for package organization. This makes the API intuitive for users to access the logger and enable_logging function directly.
src/flowMC/logger/_logger.py (2)
1-199: LGTM! Well-implemented JAX-compatible logging module.The implementation successfully creates a JAX-compatible logger by wrapping loguru functionality with
jax.debug.callback. The code is well-structured with comprehensive documentation and examples.
13-14:❓ Verification inconclusive
Consider thread safety for the global LOG variable.
The global
LOGvariable is modified by theenable_loggingfunction. In multi-threaded environments, this could lead to race conditions. Consider using thread-local storage or another thread-safe approach if this logger might be used in multi-threaded contexts.
🏁 Script executed:
#!/bin/bash # Check if this codebase uses threading or multiprocessing echo "Checking for threading or multiprocessing usage in the codebase..." rg -l "import threading|from threading|import multiprocessing|from multiprocessing" --type pyLength of output: 238
Review Change: Verify Thread Safety for Global LOG Variable
The current implementation of the
LOGflag insrc/flowMC/logger/_logger.pyremains vulnerable to race conditions if it ever becomes subject to concurrent access (e.g., via theenable_loggingfunction). Although an initial search for threading or multiprocessing usage yielded no results, this check may not be conclusive. Please manually verify whether this module is—or might be used in the future—as part of a multi-threaded context. If concurrent access is anticipated, consider refactoring the variable to use a thread-safe mechanism (such as thread-local storage or proper locking).
|
While I am positive of integrating This means the logging should go into strategy level but not resource level. I will have a look of this PR and start migrating some of these logging functionality into the main program, which I believe it should still be very beneficial |
|
That seems reasonable, let me know if I can help you with that! |
|
But what about the idea of checking gradients and other values at runtime? Is there any other way for it? |
2 participants
Summary
Logging functionality for better verbose and messages.
flowMC.logger.loggeris a modified form ofloguru.loggerto work with JAX, especially underjax.jittransform 1.loguruhas different logging modes, making logs easy to read.Pointer to discuss
FLOWMC_LOG_LEVEL=<log level or name>?Related To
Summary by CodeRabbit
New Features
Chores
Footnotes
https://github.com/jax-ml/jax/discussions/25685 ↩