Feature/logging #463
Feature/logging #463
Conversation
|
Warning Rate limit exceeded@FBumann has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 14 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 (2)
WalkthroughMigration from Python's stdlib logging to Loguru across the codebase: modules now import Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Results as results.py
participant Logger as loguru.logger
participant FlowSystem
Note over Results,Logger: Logging is temporarily disabled for 'flixopt' during restore
User->>Results: restore(dataset)
Results->>Logger: disable('flixopt')
Results->>FlowSystem: attempt restore(dataset)
alt restore succeeds
FlowSystem-->>Results: restored
Results->>Logger: enable('flixopt')
Results-->>User: return restored
else restore fails
FlowSystem-->>Results: raise error
Results->>Logger: enable('flixopt')
Results-->>User: raise _FlowSystemRestorationError
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 inconclusive)
✅ Passed checks (1 passed)
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: 4
🧹 Nitpick comments (7)
flixopt/network_app.py (1)
782-782: Replace stdout print with logger for consistencyUse the Loguru logger instead of print.
- print(f'Network visualization started on http://127.0.0.1:{port}/') + logger.info('Network visualization started on http://127.0.0.1:{}/', port)flixopt/linear_converters.py (1)
613-621: Avoid logging entire arrays; keep summary stats onlyCurrent warnings include full array contents, which can flood logs and hurt performance.
- logger.warning( - f"'{element_label}.{parameter_label}' is equal or below the common lower bound {lower_bound}." - f' {parameter_label}.min={np.min(value)}; {parameter_label}={value}' - ) + logger.warning( + "'{}.{}' <= lower bound {}. {}.min={} shape={}", + element_label, parameter_label, lower_bound, parameter_label, float(np.min(value)), np.shape(value), + ) @@ - logger.warning( - f"'{element_label}.{parameter_label}' exceeds or matches the common upper bound {upper_bound}." - f' {parameter_label}.max={np.max(value)}; {parameter_label}={value}' - ) + logger.warning( + "'{}.{}' >= upper bound {}. {}.max={} shape={}", + element_label, parameter_label, upper_bound, parameter_label, float(np.max(value)), np.shape(value), + )Also applies to: 617-621
flixopt/io.py (1)
498-516: Use warning level instead of critical for non-fatal states
model.status == 'warning'isn’t a critical failure; preferwarningfor signal without triggering critical alerts.- logger.critical(f'The model has a warning status {model.status=}. Trying to extract infeasibilities') + logger.warning(f'The model has a warning status {model.status=}. Trying to extract infeasibilities') @@ - logger.critical( + logger.warning( 'Infeasible constraints could not get retrieved. This functionality is only availlable with gurobi' )flixopt/aggregation.py (1)
106-108: Avoid expensive string building in logs; use lazy logging
describe_clusters()can be large/expensive. Use Loguru’s lazy evaluation.- logger.info(self.describe_clusters()) + logger.opt(lazy=True).info(lambda: self.describe_clusters())flixopt/calculation.py (1)
254-263: Use lazy logging for main results blockDefer YAML formatting until the message is actually emitted.
- if should_log: - logger.info( - f'{" Main Results ":#^80}\n' - + fx_io.format_yaml_string( - self.main_results, - compact_numeric_lists=True, - ) - ) + if should_log: + logger.opt(lazy=True).info( + lambda: f'{" Main Results ":#^80}\n' + + fx_io.format_yaml_string(self.main_results, compact_numeric_lists=True) + )tests/test_config.py (2)
235-240: Remove or repurpose this obsolete test.This test is now a no-op that just verifies
CONFIG.apply()doesn't raise an exception. Since thepropagateattribute doesn't exist in Loguru, this test no longer serves its original purpose.Consider either:
- Removing this test entirely, or
- Repurposing it to test a relevant Loguru-specific behavior
242-252: Strengthen rotation test to verify actual behavior.The current test only verifies that a handler exists but doesn't actually test that log rotation works. Consider enhancing this test to verify rotation behavior:
def test_file_handler_rotation(self, tmp_path): """Test that file handler actually rotates logs.""" log_file = tmp_path / 'rotating.log' CONFIG.Logging.file = str(log_file) CONFIG.Logging.max_file_size = 1024 # Small size to trigger rotation CONFIG.Logging.backup_count = 2 CONFIG.apply() # Write enough logs to trigger rotation for i in range(200): logger.info(f"Log message {i}" * 10) # Verify rotation occurred - should have .1, .2 backup files assert log_file.exists() assert (tmp_path / 'rotating.log.1').exists() or \ list(tmp_path.glob('rotating.log.*')) # Loguru may use different naming
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
docs/getting-started.md(1 hunks)examples/05_Two-stage-optimization/two_stage_optimization.py(1 hunks)flixopt/aggregation.py(1 hunks)flixopt/calculation.py(1 hunks)flixopt/color_processing.py(1 hunks)flixopt/components.py(1 hunks)flixopt/config.py(12 hunks)flixopt/core.py(1 hunks)flixopt/effects.py(1 hunks)flixopt/elements.py(1 hunks)flixopt/features.py(1 hunks)flixopt/flow_system.py(1 hunks)flixopt/interface.py(1 hunks)flixopt/io.py(1 hunks)flixopt/linear_converters.py(1 hunks)flixopt/modeling.py(1 hunks)flixopt/network_app.py(1 hunks)flixopt/plotting.py(1 hunks)flixopt/results.py(2 hunks)flixopt/solvers.py(1 hunks)flixopt/structure.py(1 hunks)pyproject.toml(1 hunks)tests/test_config.py(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
flixopt/solvers.py (1)
flixopt/config.py (1)
CONFIG(58-407)
tests/test_config.py (1)
flixopt/config.py (4)
CONFIG(58-407)Logging(93-140)apply(236-258)_setup_logging(449-506)
flixopt/results.py (2)
flixopt/flow_system.py (3)
FlowSystem(47-1356)from_dataset(426-481)_connect_network(795-827)flixopt/structure.py (1)
from_dataset(707-740)
🔇 Additional comments (12)
flixopt/network_app.py (1)
7-8: Loguru migration acknowledgedImport looks good and consistent with repo-wide switch.
flixopt/linear_converters.py (1)
10-10: Loguru migration acknowledgedImport is fine and matches the project move.
flixopt/io.py (1)
17-17: Loguru migration acknowledgedImport is fine.
flixopt/structure.py (1)
24-24: Loguru migration acknowledgedLooks good.
flixopt/components.py (1)
12-12: Loguru migration acknowledgedImport is consistent and used in this module.
flixopt/aggregation.py (1)
14-14: Loguru migration acknowledgedImport is fine.
flixopt/calculation.py (2)
22-22: Loguru migration acknowledgedImport is fine here.
22-22: Central Loguru configuration in CONFIG is properly implemented and applies sinks/levels as intended.The verification confirms that
flixopt/config.pyfully configures Loguru with all requested features:
- Console and file sinks are both configured with logger.add() calls
- Logging levels (DEBUG, INFO, WARNING, ERROR, CRITICAL) are configurable via CONFIG.Logging.level
- Custom formatting, file rotation (max_file_size, backup_count), and verbose tracebacks are all implemented
- CONFIG.apply() properly invokes _setup_logging() to activate the configuration
- Logging is initialized on module import via CONFIG.apply()
- The deprecated change_logging_level() function properly warns users to use CONFIG.Logging.level instead
No stdlib logging usage remains; no interception issues. The import in calculation.py is correct and follows the centralized configuration pattern.
Likely an incorrect or invalid review comment.
flixopt/solvers.py (1)
10-12: LGTM! Clean migration to Loguru.The import change from standard
loggingtologuruis correct. The logger API calls used throughout the module (logger.info(), etc.) are compatible between both libraries, so this is a drop-in replacement.tests/test_config.py (2)
7-7: LGTM! Correct import for Loguru migration.The import change aligns with the project-wide migration to Loguru.
387-400: Excellent test pattern!This test demonstrates the right approach by testing observable behavior (actual log messages in files) rather than internal handler structures. This is the pattern other tests should follow.
docs/getting-started.md (1)
25-41: LGTM! Clear and helpful logging documentation.The new logging section provides a clear introduction to configuring Loguru via CONFIG. The examples are concise, demonstrate both basic configuration and preset methods, and appropriately link to detailed documentation. This is well-placed in the getting started guide.
Use lazy logging for expensive operation
Strengthen rotation test Refactor tests to avoid private Loguru APIs
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
# Conflicts: # CHANGELOG.md # flixopt/core.py # flixopt/flow_system.py # flixopt/io.py # flixopt/linear_converters.py
Description
Improve logging functionality
Type of Change
Related Issues
Closes #(issue number)
Testing
Checklist
Summary by CodeRabbit
Documentation
Refactor
Tests
Examples
Chores