Conversation
|
Warning Rate limit exceeded@ypriverol has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 49 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (23)
WalkthroughThis update introduces a comprehensive logging infrastructure to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ibaqpy
participant Logger
participant LoggingConfig
User->>ibaqpy: Import package / run function
ibaqpy->>LoggingConfig: initialize_logging()
LoggingConfig->>Logger: configure_logging()
Logger-->>LoggingConfig: Logging configured
LoggingConfig-->>ibaqpy: Logging ready
Note over ibaqpy,Logger: During processing functions
ibaqpy->>Logger: Log execution time, function calls, events
Logger-->>ibaqpy: Logs with context and timing
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
There was a problem hiding this comment.
Pull Request Overview
This PR focuses on enhancing the logging throughout the ibaqpy package to provide better visibility into key processing steps. The changes include a version bump, improved and more consistent logging messages in various modules (write_queue, peptides2protein, peptide_normalization), and the addition of dedicated logging configuration utilities.
- Version bump from 0.0.4 to 0.0.5 in metadata and project configuration.
- Enhanced logging in CSV/Parquet writer tasks and peptide normalization workflows.
- New dedicated logging modules (logging_config.py and logger.py) to centralize logging configurations.
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| recipe/meta.yaml | Version bumped to support the release update. |
| pyproject.toml | Version bumped to synchronize with package updates. |
| ibaqpy/ibaq/write_queue.py | Added detailed debug logging for task queuing, writing, and closing. |
| ibaqpy/ibaq/utils.py | Introduced warnings filtering to suppress specific deprecation warnings. |
| ibaqpy/ibaq/peptides2protein.py | Updated logger usage to use a centralized logger; improved error logging. |
| ibaqpy/ibaq/peptide_normalization.py | Added logging decorators and refined logging messages for processing steps. |
| ibaqpy/ibaq/logging_config.py | New module to initialize and configure logging across the package. |
| ibaqpy/ibaq/logger.py | New module providing logger setup, context adapters, and logging decorators. |
| ibaqpy/init.py | Version bump and initialization of the logging system. |
Comments suppressed due to low confidence (1)
ibaqpy/ibaq/peptide_normalization.py:759
- [nitpick] Consider refactoring repetitive logging statements used in sample processing steps into a helper function to reduce duplication and improve maintainability.
logger.info(f"{str(sample).upper()}: Data preprocessing...")
| args_str = ", ".join([str(arg) for arg in args]) | ||
| kwargs_str = ", ".join([f"{k}={v}" for k, v in kwargs.items()]) | ||
| all_args = ", ".join(filter(None, [args_str, kwargs_str])) | ||
|
|
||
| logger.log(level, f"Calling {func.__name__}({all_args})") |
There was a problem hiding this comment.
[nitpick] Review the string formatting in the logger decorator to ensure it does not incur unnecessary overhead when the logging level is disabled; consider conditional formatting if performance becomes a concern.
| args_str = ", ".join([str(arg) for arg in args]) | |
| kwargs_str = ", ".join([f"{k}={v}" for k, v in kwargs.items()]) | |
| all_args = ", ".join(filter(None, [args_str, kwargs_str])) | |
| logger.log(level, f"Calling {func.__name__}({all_args})") | |
| if logger.isEnabledFor(level): | |
| args_str = ", ".join([str(arg) for arg in args]) | |
| kwargs_str = ", ".join([f"{k}={v}" for k, v in kwargs.items()]) | |
| all_args = ", ".join(filter(None, [args_str, kwargs_str])) | |
| logger.log(level, f"Calling {func.__name__}({all_args})") |
![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 (4)
recipe/meta.yaml (1)
53-53: Missing newline at end of file.Static analysis detected missing newline character at the end of the file. While not affecting functionality, adding a newline at the end of files is a common best practice that improves compatibility with various tools and version control systems.
extra: recipe-maintainers: - - ypriverol + - ypriverol +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 53-53: no new line character at the end of file
(new-line-at-end-of-file)
ibaqpy/ibaq/logger.py (3)
53-100: Comprehensive logging configuration with safety measures.The configuration function handles both console and file logging well. Good practices implemented:
- Removing existing handlers to avoid duplicate logs
- Creating log directories with
exist_ok=Trueto avoid race conditions- Configuring specific package loggers
Consider adding error handling for potential permission issues when creating log files:
if log_file: try: os.makedirs(os.path.dirname(os.path.abspath(log_file)), exist_ok=True) file_handler = logging.FileHandler(log_file) file_handler.setFormatter(formatter) root_logger.addHandler(file_handler) + except (PermissionError, IOError) as e: + console_handler.setLevel(logging.WARNING) + logging.warning(f"Could not set up log file at {log_file}: {e}") + logging.warning("Continuing with console logging only")
102-132: Good execution time logging implementation.The
log_execution_timedecorator properly captures and logs function execution times, including handling of exceptions while preserving the original stack trace.Two suggestions to enhance this decorator:
- Consider using milliseconds for shorter functions:
- logger.log(level, f"Completed {func.__name__} in {execution_time}") + execution_ms = execution_time.total_seconds() * 1000 + logger.log(level, f"Completed {func.__name__} in {execution_time} ({execution_ms:.2f}ms)")
- The default INFO level might be too verbose for frequent operations. Consider using DEBUG as the default:
- def log_execution_time(logger: Union[logging.Logger, logging.LoggerAdapter], level: int = logging.INFO): + def log_execution_time(logger: Union[logging.Logger, logging.LoggerAdapter], level: int = logging.DEBUG):
1-155: Consider adding structured logging support for better log processing.This is a comprehensive logging system, but it could benefit from structured logging support.
Consider adding JSON logging support to make logs machine-readable and easier to parse in log aggregation tools:
import json class JsonFormatter(logging.Formatter): """ Formatter that outputs JSON strings after parsing the log record. """ def __init__(self, **kwargs): self.json_default = kwargs.pop("json_default", str) super().__init__(**kwargs) def format(self, record): log_record = { "timestamp": self.formatTime(record), "level": record.levelname, "name": record.name, "message": record.getMessage(), } if hasattr(record, "exc_info") and record.exc_info: log_record["exception"] = self.formatException(record.exc_info) # Add extra fields from the LoggerAdapter if hasattr(record, "extra_contextual_info"): log_record.update(record.extra_contextual_info) return json.dumps(log_record, default=self.json_default) # Then add an option to configure_logging: def configure_logging( level: str = "info", log_file: Optional[str] = None, log_format: str = DEFAULT_LOG_FORMAT, date_format: str = DEFAULT_DATE_FORMAT, propagate: bool = True, json_logging: bool = False, ) -> None: # ... existing code ... # Choose formatter based on json_logging flag if json_logging: formatter = JsonFormatter() else: formatter = logging.Formatter(log_format, date_format)This would make logs much easier to analyze in systems like ELK stack, Graylog, or cloud logging platforms.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.gitignore(1 hunks)ibaqpy/__init__.py(1 hunks)ibaqpy/ibaq/logger.py(1 hunks)ibaqpy/ibaq/logging_config.py(1 hunks)ibaqpy/ibaq/peptide_normalization.py(7 hunks)ibaqpy/ibaq/peptides2protein.py(9 hunks)ibaqpy/ibaq/utils.py(1 hunks)ibaqpy/ibaq/write_queue.py(4 hunks)pyproject.toml(1 hunks)recipe/meta.yaml(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
ibaqpy/__init__.py (1)
ibaqpy/ibaq/logging_config.py (1)
initialize_logging(15-50)
ibaqpy/ibaq/peptide_normalization.py (1)
ibaqpy/ibaq/logger.py (2)
get_logger(33-50)log_execution_time(102-131)
ibaqpy/ibaq/write_queue.py (1)
ibaqpy/ibaq/logger.py (1)
get_logger(33-50)
ibaqpy/ibaq/peptides2protein.py (1)
ibaqpy/ibaq/logger.py (3)
get_logger(33-50)log_execution_time(102-131)log_function_call(134-155)
ibaqpy/ibaq/logging_config.py (1)
ibaqpy/ibaq/logger.py (1)
configure_logging(53-99)
🪛 YAMLlint (1.35.1)
recipe/meta.yaml
[error] 53-53: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (34)
.gitignore (1)
21-21: Ignore VSCode IDE settings.Adding
/ .vscode/ensures Visual Studio Code workspace files are not committed, aligning with the existing.idearule and keeping the repo clean.ibaqpy/ibaq/utils.py (1)
4-13: Well-placed warning suppression for numpy matrix deprecation.The addition of this specific warning filter follows best practices by targeting only the exact warning from the specific module, preventing it from cluttering logs while maintaining awareness of other important warnings.
pyproject.toml (1)
6-6: Version increment aligned with package changes.The version bump from 0.0.4 to 0.0.5 appropriately reflects the addition of the new logging infrastructure to the package.
recipe/meta.yaml (1)
4-4: Version increment matches pyproject.toml.This version update is consistent with the changes in pyproject.toml and init.py.
ibaqpy/__init__.py (4)
1-6: Good addition of comprehensive module docstring.The added docstring clearly describes the package's purpose and functionality, following Python best practices for documentation.
8-12: Consistent warning suppression implementation.This warning filter matches the one added to utils.py, ensuring consistent handling of numpy matrix deprecation warnings throughout the package.
14-14: Version increment aligned with other configuration files.The version update to 0.0.5 is consistent with changes in pyproject.toml and meta.yaml.
16-21: Well-structured logging initialization.The addition of logging initialization code is well-implemented:
- Imports the initialization function from the new logging_config module
- Initializes logging with default settings
- Includes helpful comments explaining how users can override these settings
Based on the provided snippet from logging_config.py, this will respect environment variables for configuration, which is good practice.
ibaqpy/ibaq/peptide_normalization.py (8)
3-3: Good addition of time module for performance measurements.The time module is appropriately added to enable precise execution time tracking throughout the peptide normalization process.
31-34: Well-structured logging setup with appropriate module naming.The logger initialization follows best practices by using a hierarchical name that reflects the module's location in the package structure. This enables granular log filtering and maintains consistency with the broader logging infrastructure.
662-662: Excellent use of execution time logging decorator.The
@log_execution_timedecorator enhances observability by automatically tracking and logging the start, completion, and total execution time of this complex function without cluttering the business logic.
726-726: More informative loading message with file path context.The log message now includes the Parquet file path, providing essential context for troubleshooting and monitoring.
793-802: Well-implemented performance tracking for feature normalization.The added timing code provides valuable performance metrics without affecting the core functionality. The log message includes both the operation result (feature count) and the execution time, which is helpful for performance optimization.
811-818: Good performance tracking for fraction merging process.Similar to the feature normalization timing, this provides consistent performance metrics for the fraction merging operation. The elapsed time reporting helps identify potential bottlenecks in processing pipelines.
835-842: Consistent timing implementation for peptidoform summation.The timing implementation follows the same pattern used for other operations, maintaining consistency in performance tracking throughout the module. This systematic approach to performance monitoring is commendable.
847-847: Clear logging for the final processing step.The log message clearly indicates the saving operation is in progress, providing appropriate visibility into the workflow completion stage.
ibaqpy/ibaq/logging_config.py (3)
1-14: Well-documented module with clear purpose statement.The module docstring clearly explains the purpose and usage of the logging configuration module, which is essential for maintainability.
15-51: Well-designed logging initialization with environment variable support.The
initialize_loggingfunction provides a flexible configuration mechanism with sensible defaults while respecting environment variables for runtime customization. This approach follows best practices for application configuration.The log initialization message provides visibility into the active configuration, which is helpful for troubleshooting.
53-76: Robust log file path generation with directory creation.The
get_log_file_pathfunction generates standardized log file paths with appropriate date-based naming and ensures the target directory exists. This eliminates common file operation errors and centralizes log file management.ibaqpy/ibaq/peptides2protein.py (5)
13-14: Appropriate imports for logging functionality.The additions properly import the standard logging module and the custom logger utilities from the package.
Also applies to: 35-35
42-43: Consistent logger initialization pattern.The logger is correctly initialized with a hierarchical name matching the module structure, following the same pattern used in other modules.
45-45: Effective use of function decorators for comprehensive logging.The strategic application of
@log_function_calland@log_execution_timedecorators to key functions provides detailed visibility into function calls, arguments, and performance metrics without cluttering the function implementations.Also applies to: 57-57, 74-74, 87-87, 264-264
131-131: Improved error logging with parameterized format.The error logging now uses parameterized format instead of f-strings, which is more efficient and enables better log processing by log management systems.
341-342: Better log level separation for different information types.The single log message has been appropriately split into separate INFO and DEBUG level messages, providing the essential row count at INFO level while keeping the verbose data sample at DEBUG level. This allows for more precise log filtering.
ibaqpy/ibaq/write_queue.py (7)
2-2: Good addition of time tracking and logger setup.The time module is appropriately imported for performance tracking, and the logger is properly initialized with a module-specific name consistent with the package's logging structure.
Also applies to: 11-15
84-84: Useful debug logging for queue operations.Adding debug logging for the queue operations provides visibility into the asynchronous writing process, including the number of rows being queued and the target file. This is helpful for troubleshooting potential performance or data flow issues.
Also applies to: 200-201
88-92: Enhanced docstring and logging for writer close operation.The improved docstring and debug logging for the close operation provide better clarity on the lifecycle of the writing thread, making it easier to diagnose issues with queue processing.
106-125: Excellent error handling and performance tracking for CSV writing.The added timing code, row counting, and comprehensive try-except block significantly improve the robustness and observability of the CSV writing process. The error logging ensures that write failures are properly documented before exceptions are re-raised.
127-127: Consistent logging for writer closure.Adding a debug log for the writer closure provides a complete picture of the writer's lifecycle, complementing the logs for queue closure.
194-208: Improved docstrings and consistent logging for Parquet writer.The enhanced docstrings and debug logging for the Parquet writer match the style and thoroughness of the CSV writer, maintaining consistency across the module.
217-234: Comprehensive performance and error logging for Parquet writing.The implementation mirrors the CSV writer's approach with timing measurements, row counting, and proper error handling. The additional log for Parquet writer initialization provides important context for the asynchronous writing process.
ibaqpy/ibaq/logger.py (3)
1-19: Well-structured logging setup with proper level definitions.The imports and logging level configuration are well-organized. The
LOG_LEVELSdictionary provides a clean way to convert string representations to logging module constants, which is helpful for configuration via environment variables or config files.
21-31: Good implementation of context-aware logging.The
ContextAdapterclass provides a clean way to add contextual information to log messages. This will be very helpful for debugging by associating related log entries with common context.
33-51: Well-designed logger factory function with proper type hints.The
get_loggerfunction is nicely implemented with good type hints and docstrings. It returns either a standard logger or a context-aware adapter based on whether context is provided.
| /tests/example/ibaq_corrected_combined.h5ad | ||
| /tests/example/ibaq_corrected_combined.tsv | ||
| /tests/example/PXD017834-ibaq.tsv | ||
| /tests/example/PXD017834-peptides-norm.csv | ||
| /tests/example/PXD017834-peptides-norm.parquet | ||
| /tests/example/QCprofile.pdf |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify test data ignore patterns and include log files.
Explicit entries under tests/example can be condensed into extension-based wildcards to automatically cover new example files and reduce maintenance overhead. Since the PR introduces a logging infrastructure, it’s also important to ignore generated log files and directories:
-/tests/example/ibaq_corrected_combined.h5ad
-/tests/example/ibaq_corrected_combined.tsv
-/tests/example/PXD017834-ibaq.tsv
-/tests/example/PXD017834-peptides-norm.csv
-/tests/example/PXD017834-peptides-norm.parquet
-/tests/example/QCprofile.pdf
+/tests/example/*.h5ad
+/tests/example/*.tsv
+/tests/example/*.csv
+/tests/example/*.parquet
+/tests/example/*.pdf
+# Ignore logs generated by the new logging infrastructure
+*.log
+logs/📝 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.
| /tests/example/ibaq_corrected_combined.h5ad | |
| /tests/example/ibaq_corrected_combined.tsv | |
| /tests/example/PXD017834-ibaq.tsv | |
| /tests/example/PXD017834-peptides-norm.csv | |
| /tests/example/PXD017834-peptides-norm.parquet | |
| /tests/example/QCprofile.pdf | |
| /tests/example/*.h5ad | |
| /tests/example/*.tsv | |
| /tests/example/*.csv | |
| /tests/example/*.parquet | |
| /tests/example/*.pdf | |
| # Ignore logs generated by the new logging infrastructure | |
| *.log | |
| logs/ |
| def log_function_call(logger: Union[logging.Logger, logging.LoggerAdapter], level: int = logging.DEBUG): | ||
| """ | ||
| Decorator to log function calls with arguments. | ||
|
|
||
| Args: | ||
| logger: The logger to use | ||
| level: The log level to use | ||
|
|
||
| Returns: | ||
| A decorator that logs function calls with arguments | ||
| """ | ||
| def decorator(func): | ||
| def wrapper(*args, **kwargs): | ||
| args_str = ", ".join([str(arg) for arg in args]) | ||
| kwargs_str = ", ".join([f"{k}={v}" for k, v in kwargs.items()]) | ||
| all_args = ", ".join(filter(None, [args_str, kwargs_str])) | ||
|
|
||
| logger.log(level, f"Calling {func.__name__}({all_args})") | ||
| return func(*args, **kwargs) | ||
|
|
||
| return wrapper | ||
| return decorator No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Function call logging with potential data exposure concerns.
The log_function_call decorator logs all arguments, which is useful but might:
- Expose sensitive data
- Create excessively large log entries
- Cause issues with objects that don't have a clean string representation
Consider adding safeguards:
def log_function_call(logger: Union[logging.Logger, logging.LoggerAdapter], level: int = logging.DEBUG):
"""
Decorator to log function calls with arguments.
Args:
logger: The logger to use
level: The log level to use
Returns:
A decorator that logs function calls with arguments
"""
def decorator(func):
def wrapper(*args, **kwargs):
- args_str = ", ".join([str(arg) for arg in args])
- kwargs_str = ", ".join([f"{k}={v}" for k, v in kwargs.items()])
+ # Safely convert args to strings, truncating if too long
+ def safe_str(obj, max_len=100):
+ try:
+ s = str(obj)
+ if len(s) > max_len:
+ return s[:max_len] + "..."
+ return s
+ except Exception:
+ return "<unprintable>"
+
+ # Optional: Skip sensitive parameter names
+ sensitive_params = {"password", "token", "secret", "key"}
+
+ args_str = ", ".join([safe_str(arg) for arg in args])
+ kwargs_str = ", ".join([
+ f"{k}='***'" if k.lower() in sensitive_params else f"{k}={safe_str(v)}"
+ for k, v in kwargs.items()
+ ])
all_args = ", ".join(filter(None, [args_str, kwargs_str]))
logger.log(level, f"Calling {func.__name__}({all_args})")
return func(*args, **kwargs)
return wrapper
return decorator📝 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 log_function_call(logger: Union[logging.Logger, logging.LoggerAdapter], level: int = logging.DEBUG): | |
| """ | |
| Decorator to log function calls with arguments. | |
| Args: | |
| logger: The logger to use | |
| level: The log level to use | |
| Returns: | |
| A decorator that logs function calls with arguments | |
| """ | |
| def decorator(func): | |
| def wrapper(*args, **kwargs): | |
| args_str = ", ".join([str(arg) for arg in args]) | |
| kwargs_str = ", ".join([f"{k}={v}" for k, v in kwargs.items()]) | |
| all_args = ", ".join(filter(None, [args_str, kwargs_str])) | |
| logger.log(level, f"Calling {func.__name__}({all_args})") | |
| return func(*args, **kwargs) | |
| return wrapper | |
| return decorator | |
| def log_function_call(logger: Union[logging.Logger, logging.LoggerAdapter], level: int = logging.DEBUG): | |
| """ | |
| Decorator to log function calls with arguments. | |
| Args: | |
| logger: The logger to use | |
| level: The log level to use | |
| Returns: | |
| A decorator that logs function calls with arguments | |
| """ | |
| def decorator(func): | |
| def wrapper(*args, **kwargs): | |
| # Safely convert args to strings, truncating if too long | |
| def safe_str(obj, max_len=100): | |
| try: | |
| s = str(obj) | |
| if len(s) > max_len: | |
| return s[:max_len] + "..." | |
| return s | |
| except Exception: | |
| return "<unprintable>" | |
| # Optional: Skip or mask sensitive parameter names | |
| sensitive_params = {"password", "token", "secret", "key"} | |
| args_str = ", ".join([safe_str(arg) for arg in args]) | |
| kwargs_str = ", ".join([ | |
| f"{k}='***'" if k.lower() in sensitive_params else f"{k}={safe_str(v)}" | |
| for k, v in kwargs.items() | |
| ]) | |
| all_args = ", ".join(filter(None, [args_str, kwargs_str])) | |
| logger.log(level, f"Calling {func.__name__}({all_args})") | |
| return func(*args, **kwargs) | |
| return wrapper | |
| return decorator |
Summary by CodeRabbit
New Features
Refactor
Chores
Style