ENH: Add python loggers#973
Conversation
Gui-FernandesBR
left a comment
Gui-FernandesBR
left a comment
There was a problem hiding this comment.
does it change current behavior?
If I open the getting start notebook, does it work just like as it should?
|
@Gui-FernandesBR yes, it does change current behavior. The getting started notebook would not work the same because .info() / .all_info() calls are now silent due to the nullhandler. To get the same output, the user would need to configure a handler before running simulation. |
yes that's my concern... Is there a way where we keep the same behavior for current code, but also benefit from built-in loggers without duplicating code? thta's the real challenge |
|
Hm... makes sense. Keeping print() for explicitly user-requested output What do u think? |
yes but that would imply in us duplicating code verytime we want to print a message, right? I wonder how other simulator libraries deal with the same situation. If you could bring us some references so we can base our opinion on different examples... I'd appreciate it a lot. |
|
I'll look into other libraries for references, once I have the answer I'll come back, tks for review |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #973 +/- ##
===========================================
+ Coverage 80.27% 81.20% +0.93%
===========================================
Files 104 114 +10
Lines 12769 14726 +1957
===========================================
+ Hits 10250 11958 +1708
- Misses 2519 2768 +249 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@YuriCastroDev you let me know when this PR is ready for review, ok? Thx a lot for your contribution! |
|
@Gui-FernandesBR The PR is ready for your review! Thanks for your support! :) |
There was a problem hiding this comment.
Pull request overview
This PR migrates runtime/progress output across RocketPy from scattered print() calls to Python’s logging module, adding a library-friendly default configuration (silent unless configured) and a small helper API to enable console logging.
Changes:
- Add
rocketpy.utils.enable_logging()to configure a StreamHandler and set therocketpylogger level. - Replace
print()usage with module-level loggers (logging.getLogger(__name__)) across simulation, environment, plots, and utilities. - Update tests to validate and capture logging output via
caplog, and add new unit tests forenable_logging().
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_utils.py | Adds unit tests and a logger-reset fixture for rocketpy.utils.enable_logging(). |
| tests/unit/test_rail_buttons_bending_moments.py | Migrates a plot-skip assertion from stdout capture (capsys) to logging capture (caplog). |
| tests/unit/simulation/test_flight_comparator.py | Updates comparator output assertions from print() to logging via caplog. |
| rocketpy/utils.py | Introduces enable_logging() helper for end-user logging activation. |
| rocketpy/utilities.py | Replaces flutter analysis print() output with structured logger.info() messages. |
| rocketpy/tools.py | Adds module logger and replaces __main__ doctest result prints with log records. |
| rocketpy/stochastic/stochastic_model.py | Replaces report print() with logger.info(). |
| rocketpy/simulation/monte_carlo.py | Replaces _SimMonitor/MonteCarlo progress prints with logger.*() calls; keeps reprint() as compatibility shim. |
| rocketpy/simulation/flight.py | Adds module logger; emits progress/completion via logging alongside existing verbose printing. |
| rocketpy/simulation/flight_data_importer.py | Converts attribute/unit conversion prints to debug/info logging. |
| rocketpy/simulation/flight_data_exporter.py | Converts export status prints to info logging. |
| rocketpy/simulation/flight_comparator.py | Converts comparator status/summary prints to logging and updates messages accordingly. |
| rocketpy/sensors/sensor.py | Converts sensor data export prints to info logging (including multi-file saves). |
| rocketpy/sensitivity/sensitivity_model.py | Converts optional-dependency import error prints to error logging. |
| rocketpy/rocket/rocket.py | Converts motor-association warnings and overwrite warning from print to logger warnings. |
| rocketpy/rocket/aero_surface/nose_cone.py | Converts nose cone length adjustment print to warning log. |
| rocketpy/plots/rocket_plots.py | Converts plot “section header” prints to info logs. |
| rocketpy/plots/monte_carlo_plots.py | Converts missing-data skip prints to warning logs. |
| rocketpy/plots/flight_plots.py | Converts various plot availability/status prints to info/warning logs. |
| rocketpy/plots/environment_plots.py | Converts plot status prints to info logs. |
| rocketpy/plots/compare/compare_flights.py | Converts save/“not implemented” prints to info/warning logs. |
| rocketpy/plots/aero_surface_plots.py | Converts plot preface prints to info logs. |
| rocketpy/motors/tank_geometry.py | Converts spherical-cap warning print to warning log. |
| rocketpy/mathutils/vector_matrix.py | Adds module logger and converts __main__ doctest result prints to logs. |
| rocketpy/mathutils/function.py | Adds module logger and converts plotting error/doctest __main__ prints to logs. |
| rocketpy/environment/tools.py | Adds module logger and converts __main__ doctest result prints to logs. |
| rocketpy/environment/fetchers.py | Converts elevation-fetch print to debug logging. |
| rocketpy/environment/environment.py | Converts elevation/topographic/export status prints to info/debug logs. |
| rocketpy/environment/environment_analysis.py | Converts requirement/unit-system/export/save prints to warning/info/error logs. |
| rocketpy/init.py | Adds a NullHandler for library-safe default silence and exposes rocketpy.utils. |
| CHANGELOG.md | Adds a changelog entry for the logging adoption enhancement. |
|
|
||
| _SimMonitor.reprint(msg, end="\r", flush=True) | ||
| logger.debug(msg) |
Co-authored-by: Gui-FernandesBR <63590233+Gui-FernandesBR@users.noreply.github.com>
Co-authored-by: Gui-FernandesBR <63590233+Gui-FernandesBR@users.noreply.github.com>
Co-authored-by: Gui-FernandesBR <63590233+Gui-FernandesBR@users.noreply.github.com>
Co-authored-by: Gui-FernandesBR <63590233+Gui-FernandesBR@users.noreply.github.com>
Co-authored-by: Gui-FernandesBR <63590233+Gui-FernandesBR@users.noreply.github.com>
| sim_monitor.reprint( | ||
| "Simulation Interrupt, files from simulation " | ||
| f"{sim_idx} saved." | ||
| logger.warning( | ||
| "Simulation interrupt. Files from simulation %d saved.", | ||
| sim_idx, |
There was a problem hiding this comment.
Have you checked if these changes behave well regarding parallel monte carlo?
The sim_monitor was in use to have a shared instance that makes sure concurrent messaging will not end up mangled in the shell output. Perhaps the logging API can do this out of the box, I am not sure.
One way to check would be either:
- Running the slow test that specifically has a parallel monte carlo run with a
-sflag to capture the output and keep an eye on the logged messages. E.g.:
pytest -m slow -s --runslow tests/integration/simulation/test_monte_carlo.py- Or, run the notebook in
docs/notebooks/monte_carlo_analysis/monte_carlo_class_usage.ipynb, which has a parallel monte carlo run.
From a very quick search, here is what the Python docs has to say about it. It seems that multiprocessing logging might not be straightforward.
There was a problem hiding this comment.
I'll check this and get back to you. Any other edge case you'd like me to verify besides the parallel monte carlo run, or is this the main concern?
There was a problem hiding this comment.
I believe the other comments already address anything else I had in mind. Good work on using "%" strings rather than f-strings, it should be better for performance.
There was a problem hiding this comment.
I ran the verification as you suggested with:
pytest -m slow -s --runslow tests/integration/simulation/test_monte_carlo.py
The progress ticker was indeed silent because _SimMonitor runs in a separate process and logging doesn't work in that context. I fixed it by keeping print() in those methods, the output looks correct and all 4 tests passed.
Commit: 2fb3fe3
There was a problem hiding this comment.
Do you agree that these message will stop making sense here?
The prints existed to separated each plot from the next one.
If we turn these into info logs, the user may not seeing them (unless they implement a handler).
On the other hand, if a user consumes the log without seeing the plot, it doesn't really make sense at all (i.e. reading "airfoil lift curve" doesn't make as much sense as reading "plotting airfoil lift curve")
In other words, we need to rethink what we really want to display in each message within the plots module.
| print( | ||
| "Due to the chosen bluffness ratio, the nose " | ||
| f"cone length was reduced to {self.length} m." | ||
| logger.warning( | ||
| "Due to the chosen bluffness ratio, the nose cone length was " | ||
| "reduced to %.4f m.", | ||
| self.length, | ||
| ) | ||
| self.fineness_ratio = self.length / (2 * self.base_radius) |
There was a problem hiding this comment.
we are suprassing somehting really important here, no?
There was a problem hiding this comment.
Indeed, since this is a warning about an automatic change, I used UserWarning "the code worked, but something happened that you should know about".
Commit: a29f05f
| logger.info("All the %d tests passed!", res.attempted) | ||
| else: | ||
| print(f"{res.failed} out of {res.attempted} tests failed.") | ||
| logger.warning("%d out of %d tests failed.", res.failed, res.attempted) |
There was a problem hiding this comment.
please search and replace in the entire codebase...
logger.error("%d out of %d tests failed.", res.failed, res.attempted)There was a problem hiding this comment.
could you kindly add a documentation within docs/ folder teaching how to use it?
There was a problem hiding this comment.
the docstring already helps a lot, but beginners may benefit from a more detailed instruction, perhaps adding it to the getting started notebook could help!
There was a problem hiding this comment.
Sure, it'll be a pleasure! After addressing all the remaining comments, I'll take care of it.
There was a problem hiding this comment.
I was thinking about adding something to the PR template, but haven't fully thought it out yet, my idea would be to remind reviewers to always check print() vs logger.* usage. What do u think? any ideas?
Gui-FernandesBR
left a comment
Gui-FernandesBR
left a comment
There was a problem hiding this comment.
I already reviewd 23/31 files. there are some changes I already see as important.
But overall the Pr looks great!
|
@YuriCastroDev please let me know when you fix all the comments. I will mark the PR as "draft' once again |
Pull request type
Checklist
black rocketpy/ tests/) has passed locallypytest tests -m slow --runslow) have passed locallyCHANGELOG.mdhas been updated (if relevant)Current behavior
RocketPy uses bare
print()calls scattered across the codebase to report simulation progress, warnings, and errors. This approach has no severity levels, cannot be redirected to a file, and cannot be silenced or filtered by the end user. Closes #450.New behavior
Internal runtime events now use Python's built-in
loggingmodule following best practices for libraries, while user-requested output (.info(),.all_info()) continues to useprint()unchanged.What changed:
rocketpy/__init__.pyregisters aNullHandler, so the library is silent by default with no output unless the user configures a handler.logger = logging.getLogger(__name__), creating a hierarchy (rocketpy.simulation.flight,rocketpy.environment.environment, etc.) that users can filter individually.DEBUG: high-frequency solver ticks, internal detailINFO: operation confirmations (simulation completed, files saved)WARNING: unexpected but recoverable situations (missing motor, geometry auto-corrected, feature silently skipped)ERROR: optional dependency unavailable, feature brokenrocketpy/prints/folder kept asprint()methods likeflight.info()and
rocket.all_info()continue to print directly to the terminal as before._SimMonitor.reprint()inMonteCarlokept for backwards compatibility and now delegates tologger.info().rocketpy/utils.pywithenable_logging()helper function so users can activate logging with a single line:Note on verbose parameter: Flight(verbose=True) retains its original real-time terminal progress ticker. The same information is also emitted via logger.debug(), so users who configure a logging handler will capture it as well.
Tests updated: 3 existing tests migrated from capsys to caplog where the output source changed from print() to logger.*. 6 new unit tests added for rocketpy.utils.enable_logging().
Breaking change
Additional information