Feature/loguru cve2 #483
Feature/loguru cve2 #483
Conversation
✅ Summary of Changes
Key Features Implemented:
1. Stdout by default (was stderr in loguru)
- Logs now go to sys.stdout by default
- Configurable via stream parameter: CONFIG.Logging.enable_console('INFO', stream=sys.stderr)
2. SUCCESS log level preserved
- Custom level 25 (between INFO and WARNING)
- Green color in console output
- logger.success() method available
3. Multi-line formatting with box borders
- Single-line: INFO Message
- Multi-line:
INFO ┌─ First line
│ Middle line
└─ Last line
4. Colored output with colorlog
- DEBUG: cyan
- INFO: white
- SUCCESS: green
- WARNING: yellow
- ERROR: red
- CRITICAL: red on white background
5. Simplified API
- CONFIG.Logging.enable_console('INFO') - stdout, colored
- CONFIG.Logging.enable_console('INFO', colored=False) - plain text
- CONFIG.Logging.enable_console('INFO', stream=sys.stderr) - stderr output
- CONFIG.Logging.enable_file('INFO', 'app.log') - file logging
- CONFIG.Logging.disable() - disable all logging
6. Backward compatibility
- change_logging_level() still works (deprecated)
- Helper methods preserved: CONFIG.debug(), CONFIG.exploring(), etc.
Summary of the improved format: Format: [dimmed time] [colored level] │ message Features: - ⏰ Time displayed in HH:MM:SS format (dimmed so not distracting) - 🎨 Level colored for quick scanning (DEBUG=cyan, INFO=white, SUCCESS=green, WARNING=yellow, ERROR=red, CRITICAL=bold red) - 📊 Clean separator │ between level and message - 📦 Multi-line support with box borders (┌─, │, └─) - 📍 Stdout by default (configurable)
…oredMultilineFormatter CONFIG.Logging.set_colors()
|
Caution Review failedThe pull request is closed. WalkthroughMigrates project logging from Loguru to Python's stdlib logging with optional colorlog; adds Multiline/ColoredMultiline formatters and a SUCCESS level; introduces CONFIG.Logging helpers (enable_console, enable_file, disable, set_colors) and presets; updates modules to use logging.getLogger('flixopt'); replaces loguru dependency with colorlog. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Application
participant Config as CONFIG (flixopt.config)
participant Logger as logging.getLogger('flixopt')
participant Console as StreamHandler
participant File as RotatingFileHandler
participant Color as colorlog (optional)
Note over Config,Logger: Configure logging via CONFIG helpers
App->>Config: CONFIG.Logging.enable_console('INFO')
Config->>Logger: getLogger('flixopt') / setLevel(INFO)
alt colorlog available
Config->>Color: create ColoredMultilineFormatter
Color->>Console: attach colored formatter
else
Config->>Console: attach MultilineFormatter
end
App->>Logger: logger.success("multi\nline\nresult")
Logger->>Console: format (Multiline/ColoredMultiline)
Console-->>App: write to stdout/stderr
App->>Config: CONFIG.Logging.enable_file('app.log')
Config->>File: attach RotatingFileHandler + MultilineFormatter
Logger->>File: write rotated logs
App->>Config: CONFIG.Logging.disable()
Config->>Logger: remove handlers / silence logger
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (10)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
flixopt/linear_converters.py (1)
1107-1146: Fix loguru-stylelogger.warningcalls incheck_bounds(they now break with stdlib logging)After switching
loggertologging.getLogger(), thelogger.warning()calls incheck_bounds()still use loguru's{}-placeholder + positional-args style. Stdlib logging expects%-style formatting; with no%tokens in the message but extra args passed, these calls will raise aTypeErrorat runtime the first time an out-of-bounds value is hit.Refactor them to use f-strings:
if not np.all(value_arr > lower_bound): logger.warning( - "'{}.{}' <= lower bound {}. {}.min={}, shape={}", - element_label, - parameter_label, - lower_bound, - parameter_label, - float(np.min(value_arr)), - np.shape(value_arr), + f"'{element_label}.{parameter_label}' <= lower bound {lower_bound}. " + f"{parameter_label}.min={float(np.min(value_arr))}, shape={np.shape(value_arr)}" ) if not np.all(value_arr < upper_bound): logger.warning( - "'{}.{}' >= upper bound {}. {}.max={}, shape={}", - element_label, - parameter_label, - upper_bound, - parameter_label, - float(np.max(value_arr)), - np.shape(value_arr), + f"'{element_label}.{parameter_label}' >= upper bound {upper_bound}. " + f"{parameter_label}.max={float(np.max(value_arr))}, shape={np.shape(value_arr)}" )Lines 1128–1129 and 1138–1139.
🧹 Nitpick comments (4)
flixopt/calculation.py (1)
258-263: Consider guarding the expensive main-results YAML formatting with a log-level check.
logger.info(f'{" Main Results ":#^80}\n' + fx_io.format_yaml_string(...))eagerly formats a potentially large YAML string wheneverlog_main_resultsisTrue, even if the effective logger level is above INFO. With loguru you previously relied onopt(lazy=True)to avoid this cost.To better mirror the old behavior and avoid unnecessary work when INFO is disabled, you could add a level check:
- 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 and logger.isEnabledFor(logging.INFO): + logger.info( + f'{" Main Results ":#^80}\n' + + fx_io.format_yaml_string(self.main_results, compact_numeric_lists=True) + )This keeps output identical when INFO is enabled but avoids computing the YAML when it would be dropped anyway.
flixopt/network_app.py (1)
768-788: Consider logging the startup URL instead of using
shownetwork()currently prints the URL (print(f'Network visualization started on http://127.0.0.1:{port}/')). For consistency with the new logging scheme and better controllability in non-interactive environments, consider emitting this vialogger.infoinstead.flixopt/aggregation.py (1)
84-109: Guard expensivedescribe_clusters()call with log-level check (optional)
cluster()now doeslogger.info(self.describe_clusters()), meaningdescribe_clusters()runs unconditionally even if INFO is disabled. With loguru this was lazy-evaluated. While this likely isn’t a major hotspot, you can restore the lazy behaviour cheaply:@@ def cluster(self) -> None: - self.clustering_duration_seconds = timeit.default_timer() - start_time # Zeit messen: - logger.info(self.describe_clusters()) + self.clustering_duration_seconds = timeit.default_timer() - start_time # Zeit messen: + if logger.isEnabledFor(logging.INFO): + logger.info(self.describe_clusters())flixopt/__init__.py (1)
65-68: Consider clarifying the comment and variable usage.Two minor observations:
The comment says "no handlers" but
NullHandler()is added on line 68. WhileNullHandlereffectively suppresses output, it's technically a handler. Consider:"(silent: WARNING level, NullHandler)".The
_loggervariable is only used for initialization and never referenced again. Sincelogging.getLogger()returns the same instance everywhere, you could simplify to:-# Initialize logger with default configuration (silent: WARNING level, no handlers) -_logger = logging.getLogger('flixopt') -_logger.setLevel(logging.WARNING) -_logger.addHandler(logging.NullHandler()) +# Initialize logger with default configuration (silent: WARNING level, NullHandler) +logger = logging.getLogger('flixopt') +logger.setLevel(logging.WARNING) +logger.addHandler(logging.NullHandler())That said, the current implementation is functionally correct and follows library logging best practices.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
CHANGELOG.md(1 hunks)docs/getting-started.md(1 hunks)examples/05_Two-stage-optimization/two_stage_optimization.py(1 hunks)flixopt/__init__.py(4 hunks)flixopt/aggregation.py(3 hunks)flixopt/calculation.py(3 hunks)flixopt/color_processing.py(1 hunks)flixopt/components.py(2 hunks)flixopt/config.py(7 hunks)flixopt/core.py(1 hunks)flixopt/effects.py(2 hunks)flixopt/elements.py(2 hunks)flixopt/flow_system.py(2 hunks)flixopt/interface.py(2 hunks)flixopt/io.py(2 hunks)flixopt/linear_converters.py(2 hunks)flixopt/modeling.py(1 hunks)flixopt/network_app.py(2 hunks)flixopt/plotting.py(2 hunks)flixopt/results.py(2 hunks)flixopt/solvers.py(1 hunks)flixopt/structure.py(2 hunks)pyproject.toml(1 hunks)tests/test_config.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
tests/test_config.py (1)
flixopt/config.py (1)
CONFIG(173-723)
flixopt/modeling.py (2)
flixopt/config.py (1)
CONFIG(173-723)flixopt/structure.py (1)
Submodel(1361-1540)
flixopt/solvers.py (1)
flixopt/config.py (1)
CONFIG(173-723)
flixopt/calculation.py (1)
flixopt/io.py (1)
format_yaml_string(255-302)
flixopt/__init__.py (1)
flixopt/config.py (3)
CONFIG(173-723)change_logging_level(726-747)get_logger(116-137)
🪛 GitHub Actions: Python Package CI/CD
examples/05_Two-stage-optimization/two_stage_optimization.py
[error] 19-19: Ruff Linting: E402 Module level import not at top of file.
🪛 GitHub Check: lint
flixopt/config.py
[failure] 83-84: Ruff (I001)
flixopt/config.py:83:13: I001 Import block is un-sorted or un-formatted
🔇 Additional comments (23)
pyproject.toml (1)
43-43: colorlog dependency range looks fine; please verify version choice against target Python versions.The
colorlog >= 6.8.0, < 7constraint is consistent with the new standard-logging + optional color backend. Please just double-check that this range is tested against all supported Python versions (3.10–3.13) and that there are no known CVEs in the selected series before release.flixopt/modeling.py (1)
1-1: Shared logger initialization inmodeling.pyis consistent with the new logging setup.Using
logging.getLogger('flixopt')here aligns this module with the project-wide logging refactor and is safe given no behavior change elsewhere in the file.Also applies to: 10-10
flixopt/flow_system.py (1)
7-7: FlowSystem now correctly uses the shared stdlib logger.Switching to
logging.getLogger('flixopt')while keeping the same log calls (warning,info,debug,critical,error) preserves behavior and fits the new CONFIG-based logging setup.Also applies to: 37-38
flixopt/elements.py (1)
7-7: Elements now use the shared stdlib logger without changing behavior.
BusandFlowplausibility warnings now go throughlogging.getLogger('flixopt'), which matches the new global logging strategy and keeps semantics intact.Also applies to: 46-47
flixopt/solvers.py (1)
7-7: Shared logger added tosolvers.pywithout behavior change.Defining
logger = logging.getLogger('flixopt')here is consistent with other modules and does not affect current solver option handling; it’s ready for future logging without altering behavior now.Also applies to: 13-13
flixopt/interface.py (1)
8-8: Interface module logging now correctly targets the sharedflixoptlogger.The warnings/debug messages in
InvestParameters.transform_dataare now routed through stdlib logging while keeping the same messaging, which is exactly what we want under the new CONFIG-based logging system.Also applies to: 25-26
flixopt/core.py (1)
6-6: Core now uses the shared stdlib logger for debug output indrop_constant_arrays.Replacing the prior logger with
logging.getLogger('flixopt')keeps the debug message behavior while aligning with the new centralized logging configuration.Also applies to: 17-18
flixopt/calculation.py (1)
20-20: ****The custom
SUCCESSlevel andlogger.successmethod are already properly implemented and registered inflixopt/config.py(lines 20–33). The setup includeslogging.addLevelName()to register the level, andlogging.Logger.successis directly attached to the Logger class. Sincecalculation.pyimports from.configearly (line 28), before any logger calls, the wiring is guaranteed to be in place. No additional setup or test coverage is required.Likely an incorrect or invalid review comment.
flixopt/plotting.py (1)
28-51: Stdlib logger wiring in plotting module looks correctUsing a module-level
logger = logging.getLogger('flixopt')and f-string messages is consistent with the new logging setup; no loguru-specific formatting remains here.flixopt/io.py (1)
5-25: Logging migration in io module is consistentModule-level
logger = logging.getLogger('flixopt')and the existing f-string-based warnings integrate cleanly with stdlib logging.flixopt/color_processing.py (1)
8-16: Color processing logger migration is safeThe new module-level logger integrates cleanly; all
logger.debug/logger.warningcalls are f-string or literal messages, so no formatting issues with stdlib logging.flixopt/linear_converters.py (1)
7-23: Logger initialization matches the new logging designImporting
loggingand usinglogging.getLogger('flixopt')keeps this module aligned with the rest of the package-wide migration.flixopt/components.py (1)
7-29: Components module logger hook is consistentThe stdlib
loggingimport and sharedlogger = logging.getLogger('flixopt')are wired correctly, and the single warning in_plausibility_checks()uses f-strings compatible with stdlib logging.flixopt/network_app.py (1)
3-29: Network app now uses shared stdlib logger correctlyThe new
loggingimport andlogger = logging.getLogger('flixopt')plus thelogger.error(...)use inflow_graph()align with the rest of the logging migration.flixopt/aggregation.py (1)
12-41: Aggregation module logger setup is aligned with the new logging systemIntroducing
import loggingandlogger = logging.getLogger('flixopt')integrates this module with the shared logger used elsewhere.flixopt/effects.py (1)
10-29: Effects module logging migration is straightforward and correctSwitching to
logging.getLogger('flixopt')and using f-string messages (e.g., when registering new effects) works cleanly with stdlib logging and matches the rest of the codebase.flixopt/__init__.py (3)
6-6: LGTM: Standard logging import added.The import is correctly placed and enables the module-level logger initialization below.
27-27: LGTM: Public API extended with get_logger.The import correctly exposes the new
get_loggerhelper function from the config module.
38-38: LGTM: get_logger added to public API.Correctly exposes the function in
__all__for public use.flixopt/results.py (2)
5-5: LGTM: Logging import added.Correctly placed in the standard library imports section.
30-30: LGTM: Module-level logger initialized.Correctly creates a logger using the 'flixopt' namespace, which will inherit configuration from the module-level setup in
__init__.py.tests/test_config.py (2)
3-3: LGTM: Test logging import added.Import correctly placed to support the test suite's logging migration.
11-11: LGTM: Test logger migrated to standard logging.Correctly creates a logger using the 'flixopt' namespace, enabling the test suite to verify the new standard logging implementation.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
flixopt/linear_converters.py (1)
1128-1147: Fix logging format incheck_bounds(currently raisesTypeError)
logger.warninghere still uses Loguru-style{}placeholders with positional args. With stdlib logging,record.getMessage()doesmsg % args, so this will raise aTypeErrorwhenever a bound is violated, instead of just logging a warning.Update the messages to use
%splaceholders (or an f-string with no extra args). For example:@@ - if not np.all(value_arr > lower_bound): - logger.warning( - "'{}.{}' <= lower bound {}. {}.min={}, shape={}", - element_label, - parameter_label, - lower_bound, - parameter_label, - float(np.min(value_arr)), - np.shape(value_arr), - ) - if not np.all(value_arr < upper_bound): - logger.warning( - "'{}.{}' >= upper bound {}. {}.max={}, shape={}", - element_label, - parameter_label, - upper_bound, - parameter_label, - float(np.max(value_arr)), - np.shape(value_arr), - ) + if not np.all(value_arr > lower_bound): + logger.warning( + "'%s.%s' <= lower bound %s. %s.min=%s, shape=%s", + element_label, + parameter_label, + lower_bound, + parameter_label, + float(np.min(value_arr)), + np.shape(value_arr), + ) + if not np.all(value_arr < upper_bound): + logger.warning( + "'%s.%s' >= lower bound %s. %s.max=%s, shape=%s", + element_label, + parameter_label, + upper_bound, + parameter_label, + float(np.max(value_arr)), + np.shape(value_arr), + )(Adjust wording as desired; the key is to use
%-style placeholders that match stdlib logging.)
♻️ Duplicate comments (2)
flixopt/config.py (2)
11-17: Moveescape_codesimport to module scope to fix Ruff I001 and avoid per-call importsRuff is correctly flagging the in-function
from colorlog.escape_codes import escape_codesimport inColoredMultilineFormatter.format. Pulling it into the existing module-leveltryblock alongsideimport colorlogboth satisfies the linter and avoids repeated imports on every log call.Suggested change:
-try: - import colorlog - - COLORLOG_AVAILABLE = True -except ImportError: - COLORLOG_AVAILABLE = False +try: + import colorlog + from colorlog.escape_codes import escape_codes + + COLORLOG_AVAILABLE = True +except ImportError: + COLORLOG_AVAILABLE = False + escape_codes = None @@ -if COLORLOG_AVAILABLE: - - class ColoredMultilineFormatter(colorlog.ColoredFormatter): +if COLORLOG_AVAILABLE: + + class ColoredMultilineFormatter(colorlog.ColoredFormatter): @@ - def format(self, record): - """Format multi-line messages with colors and box-style borders.""" - # Split into lines - lines = record.getMessage().split('\n') - - # Format time with date and milliseconds (YYYY-MM-DD HH:MM:SS.mmm) - import datetime - from colorlog.escape_codes import escape_codes + def format(self, record): + """Format multi-line messages with colors and box-style borders.""" + # Split into lines + lines = record.getMessage().split('\n') + + # Format time with date and milliseconds (YYYY-MM-DD HH:MM:SS.mmm) + import datetimeAfter this, the formatter continues to use the module-level
escape_codessymbol whenCOLORLOG_AVAILABLEis True.Also applies to: 85-88
38-72: Restore exception and stack traces in custom formattersBoth
MultilineFormatter.formatandColoredMultilineFormatter.formatoverridelogging.Formatter.formatbut only userecord.getMessage(). Anyexc_infoorstack_infopassed to the logger is silently dropped, sologger.exception(..., exc_info=True)no longer shows tracebacks, which is a significant regression for debugging.You can keep the box-style layout and still include exception/stack information by extending the
lineslist before building the box:class MultilineFormatter(logging.Formatter): @@ def format(self, record): """Format multi-line messages with box-style borders for better readability.""" - # Split into lines - lines = record.getMessage().split('\n') + # Split into lines + lines = record.getMessage().split('\n') + # Append exception and stack information if present + if record.exc_info: + lines.extend(self.formatException(record.exc_info).split('\n')) + if record.stack_info: + lines.extend(record.stack_info.rstrip().split('\n')) @@ if COLORLOG_AVAILABLE: @@ class ColoredMultilineFormatter(colorlog.ColoredFormatter): @@ def format(self, record): """Format multi-line messages with colors and box-style borders.""" - # Split into lines - lines = record.getMessage().split('\n') + # Split into lines + lines = record.getMessage().split('\n') + # Append exception and stack information if present + if record.exc_info: + lines.extend(self.formatException(record.exc_info).split('\n')) + if record.stack_info: + lines.extend(record.stack_info.rstrip().split('\n'))This restores full exception/stack visibility while preserving your multi-line box formatting.
Also applies to: 77-116
🧹 Nitpick comments (1)
flixopt/results.py (1)
341-360: Simplify logger level restoration inflow_systemThe temporary suppression via
flixopt_logger.setLevel(logging.CRITICAL + 1)is fine, but you reset the level both in theexceptblock (Line 353) and again infinally(Line 359). Theexceptreset is redundant becausefinallyalways runs. You can safely drop the reset in theexceptblock to reduce duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
flixopt/__init__.py(2 hunks)flixopt/components.py(1 hunks)flixopt/config.py(7 hunks)flixopt/core.py(1 hunks)flixopt/elements.py(2 hunks)flixopt/interface.py(1 hunks)flixopt/linear_converters.py(1 hunks)flixopt/results.py(3 hunks)flixopt/structure.py(3 hunks)test_deprecations.py(1 hunks)tests/test_config.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- flixopt/core.py
- flixopt/components.py
🧰 Additional context used
🧬 Code graph analysis (6)
flixopt/linear_converters.py (2)
flixopt/components.py (1)
LinearConverter(32-247)flixopt/core.py (1)
TimeSeriesData(35-163)
flixopt/interface.py (4)
flixopt/config.py (1)
CONFIG(152-702)flixopt/structure.py (1)
flow_system(329-347)flixopt/results.py (1)
flow_system(341-360)flixopt/flow_system.py (1)
FlowSystem(40-1618)
flixopt/structure.py (1)
flixopt/core.py (2)
TimeSeriesData(35-163)get_dataarray_stats(576-600)
tests/test_config.py (1)
flixopt/config.py (19)
CONFIG(152-702)MultilineFormatter(38-72)reset(517-543)Modeling(447-458)Solving(460-481)Logging(180-445)enable_console(255-325)enable_file(328-374)disable(377-387)format(48-72)format(80-116)exploring(614-632)debug(596-611)notebook(659-677)Plotting(483-512)production(635-656)silent(576-593)to_dict(546-573)change_logging_level(705-726)
flixopt/elements.py (1)
flixopt/config.py (1)
CONFIG(152-702)
flixopt/results.py (2)
flixopt/flow_system.py (3)
FlowSystem(40-1618)from_dataset(556-612)_connect_network(986-1021)flixopt/structure.py (1)
from_dataset(821-854)
🪛 GitHub Check: lint
flixopt/config.py
[failure] 86-87: Ruff (I001)
flixopt/config.py:86:13: I001 Import block is un-sorted or un-formatted
🔇 Additional comments (10)
flixopt/__init__.py (1)
64-67: Central logger default setup is appropriateInitializing a
flixoptlogger at WARNING with aNullHandlerkeeps the library silent by default while allowingCONFIG.Loggingto attach real handlers later. This matches the behavior exercised intests/test_config.py.test_deprecations.py (1)
8-8: ImportingDEPRECATION_REMOVAL_VERSIONfromflixopt.configis correctUpdating the tests to source
DEPRECATION_REMOVAL_VERSIONfromflixopt.configmatches the new central definition and keeps the deprecation message checks aligned with the actual library constant.flixopt/elements.py (1)
7-16: Elements module logging/config integration looks consistentSwitching to
logging.getLogger('flixopt')and importingDEPRECATION_REMOVAL_VERSIONfromflixopt.configaligns this module with the new centralized logging and deprecation configuration design used elsewhere in the package.Also applies to: 45-45
tests/test_config.py (1)
3-168: Tests comprehensively exercise the new config/logging APIThese tests validate default silence, console/file handlers, the custom SUCCESS level, multiline formatting, all key presets,
reset(),to_dict(), and the deprecation path forchange_logging_level, with proper isolation viaCONFIG.reset(). They are well-aligned with the new implementation inflixopt.config.flixopt/interface.py (3)
8-8: LGTM: Standard logging import added.The migration from Loguru to Python's standard logging library is appropriate and aligns with the PR's security objectives.
16-17: LGTM: Import consolidation improves readability.Grouping imports from the same module into a single statement is a best practice. All imported symbols (CONFIG, DEPRECATION_REMOVAL_VERSION, Interface, register_class_for_io) are used in the file.
25-25: Logger initialization is correctly implemented and migration from loguru is verified complete.The
logging.getLogger('flixopt')at line 25 follows best practices. Verification confirms:
- loguru has been completely removed from dependencies and codebase
- colorlog is properly integrated as the replacement (pyproject.toml, config.py)
- No loguru references remain anywhere
- Standard logging is consistently used across all 17+ modules
flixopt/structure.py (3)
9-9: LGTM: Standard logging import added.Consistent with the project-wide migration from Loguru to Python's standard logging library.
27-28: LGTM: Import updates reflect configuration refactoring.The imports correctly reflect that
DEPRECATION_REMOVAL_VERSIONhas been moved fromflixopt.coretoflixopt.config, and the consolidation of imports from.coreimproves readability.
38-38: LGTM: Logger initialization is correct and consistent.The module-level logger using the shared name
'flixopt'enables centralized logging configuration and is consistent with the project-wide logging migration pattern.
- Changed from "no handlers" to "NullHandler" which is more accurate
- NullHandler is technically a handler, just one that suppresses all output
2. Renamed variable (line 65):
- Changed from _logger to logger
- Since the variable is only used for initialization and logging.getLogger() returns the same instance everywhere, using logger is cleaner and
more conventional
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
examples/05_Two-stage-optimization/two_stage_optimization.py (1)
10-20: Example logger initialization is now Ruff‑compliant and consistent
logger = logging.getLogger('flixopt')is placed after the import block and reuses the shared application logger, resolving the earlier E402 and matching the new logging config.flixopt/config.py (2)
11-17: Moveescape_codesimport to module scope and avoid function‑level import
ColoredMultilineFormatter.formatstill importsescape_codesinside the method, which will trip Ruff (I001) and repeatedly re‑import on each log call. Pull this import into the existing module‑leveltryblock and reuse the name here.Suggested change:
-try: - import colorlog - - COLORLOG_AVAILABLE = True -except ImportError: - COLORLOG_AVAILABLE = False +try: + import colorlog + from colorlog.escape_codes import escape_codes + + COLORLOG_AVAILABLE = True +except ImportError: + COLORLOG_AVAILABLE = False + escape_codes = None @@ if COLORLOG_AVAILABLE: - def format(self, record): + def format(self, record): @@ - # Format time with date and milliseconds (YYYY-MM-DD HH:MM:SS.mmm) - import datetime - - from colorlog.escape_codes import escape_codes + # Format time with date and milliseconds (YYYY-MM-DD HH:MM:SS.mmm) + import datetime + # `escape_codes` is imported at module level when colorlog is availableThis keeps lint happy and avoids repeated in‑method imports while preserving behavior.
Also applies to: 76-118
38-73: Custom formatters currently drop exceptions and stack tracesBoth
MultilineFormatter.formatandColoredMultilineFormatter.formatignorerecord.exc_infoandrecord.stack_info, sologger.exception(..., exc_info=True)produces messages with no traceback, which is a serious diagnostics regression.You can preserve the box layout while restoring this information by folding the formatted exception/stack into
linesbefore applying your multi‑line formatting, e.g.:- # Split into lines - lines = record.getMessage().split('\n') + # Split into lines, including exception/stack info when present + lines = record.getMessage().split('\n') + if record.exc_info: + lines.extend(self.formatException(record.exc_info).split('\n')) + if record.stack_info: + lines.extend(record.stack_info.rstrip().split('\n')) @@ - # Single line - return standard format + # Single line - return standard format if len(lines) == 1: @@ - # Multi-line - use box format + # Multi-line - use box formatand the same pattern inside
ColoredMultilineFormatter.format. This restores parity with the standardlogging.Formatterwhile keeping the new box/colored layout.Also applies to: 76-118
🧹 Nitpick comments (1)
tests/test_config.py (1)
81-100: Formatter and stream‑selection tests are useful; consider adding exception coverage laterThe multiline, stderr, and non‑colored output tests validate the most important behaviors; once exception/stack formatting is added to the formatters, an extra assertion covering
logger.exception(..., exc_info=True)output would close the gap.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
examples/05_Two-stage-optimization/two_stage_optimization.py(1 hunks)flixopt/__init__.py(2 hunks)flixopt/aggregation.py(3 hunks)flixopt/calculation.py(3 hunks)flixopt/color_processing.py(1 hunks)flixopt/components.py(1 hunks)flixopt/config.py(7 hunks)flixopt/core.py(1 hunks)flixopt/effects.py(2 hunks)flixopt/interface.py(1 hunks)flixopt/io.py(2 hunks)flixopt/modeling.py(1 hunks)flixopt/network_app.py(2 hunks)flixopt/results.py(3 hunks)flixopt/solvers.py(1 hunks)tests/test_config.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- flixopt/io.py
- flixopt/solvers.py
- flixopt/results.py
- flixopt/init.py
- flixopt/components.py
- flixopt/color_processing.py
- flixopt/aggregation.py
- flixopt/network_app.py
- flixopt/core.py
- flixopt/calculation.py
🧰 Additional context used
🧬 Code graph analysis (3)
flixopt/modeling.py (2)
flixopt/config.py (1)
CONFIG(154-711)flixopt/structure.py (1)
Submodel(1362-1541)
tests/test_config.py (1)
flixopt/config.py (19)
CONFIG(154-711)MultilineFormatter(38-73)reset(526-552)Modeling(456-467)Solving(469-490)Logging(182-454)enable_console(257-328)enable_file(331-382)disable(385-395)format(48-73)format(81-118)exploring(623-641)debug(605-620)notebook(668-686)Plotting(492-521)production(644-665)silent(585-602)to_dict(555-582)change_logging_level(714-735)
flixopt/interface.py (3)
flixopt/structure.py (3)
Interface(279-956)register_class_for_io(43-52)flow_system(329-347)flixopt/results.py (1)
flow_system(342-361)flixopt/flow_system.py (1)
FlowSystem(40-1618)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (3.10)
🔇 Additional comments (9)
flixopt/effects.py (1)
10-10: Stdlib logger wiring forflixoptlooks correct and consistentUsing
logging.getLogger('flixopt')at module level and the existing info log inEffectCollection.add_effects()integrates cleanly with the new centralized logging setup; no issues spotted here.Please just confirm that the shared
'flixopt'logger is configured once in your logging config (as suggested in the PR description) and that tests exercisingEffectCollection.add_effects()still behave as expected (log level, no unexpected output).Also applies to: 29-29
flixopt/modeling.py (1)
1-10: Stdlib logger wiring in modeling module looks correctUsing
logging.getLogger('flixopt')at module scope aligns with the new central logging configuration and is safe; no further changes needed here.flixopt/interface.py (1)
8-18: Interface module now correctly targets the shared ‘flixopt’ loggerSwitching to
logging.getLogger('flixopt')and consolidating imports is consistent with the new logging API and doesn’t affect the public interface.Also applies to: 25-26
tests/test_config.py (5)
3-11: Test module correctly targets the configured ‘flixopt’ loggerImporting
CONFIG/MultilineFormatterand then gettinglogging.getLogger('flixopt')ensures all tests exercise the same logger configuration as the library.
17-24: Good coverage of default silence and basic console/file behaviorsSetup/teardown via
CONFIG.reset()plus tests for “silent by default”, console logging, file logging, console+file, disable, and the customSUCCESSlevel match the intended semantics ofCONFIG.Loggingand should catch regressions in handler/level handling.Also applies to: 33-80
101-136: Preset tests accurately encode the contract for exploring/debug/notebook/production/silentThese tests pin down how each preset affects logging, plotting, and solver flags, which is valuable given the richer CONFIG surface; they look consistent with the implementations in
flixopt.config.
137-160: Reset, to_dict, and attribute modification tests align with CONFIG designVerifying that
reset()restores defaults and clears handlers, and thatto_dict/attribute mutations behave as expected, matches the documented behavior of CONFIG and strengthens confidence in config changes.
161-166: Deprecation test forchange_logging_levelis appropriateAsserting a
DeprecationWarningwith the documented message ensures this legacy helper won’t silently linger past its deprecation window.flixopt/config.py (1)
525-553: CONFIG.reset and presets are wired coherently with the logging helpers
reset()restoring defaults and callingLogging.disable(), and the presets (silent,debug,exploring,production,notebook) composing the Logging helpers with Solving/Plotting flags, form a clear and consistent configuration story that your tests intests/test_config.pyalready validate.Also applies to: 585-603, 605-621, 623-642, 643-666, 668-686
…ng works correctly: - test_exception_logging - Tests colored output includes exception tracebacks - test_exception_logging_non_colored - Tests non-colored output includes exception tracebacks Both tests verify that when using logger.exception(), the output contains: - The error message - The exception type (ValueError) - The exception details - The full traceback
Description
Brief description of the changes in this PR.
Type of Change
Related Issues
Closes #(issue number)
Testing
Checklist
Summary by CodeRabbit
New Features
Breaking Changes
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.