Adding a logger widget for logging in run_tardis #2700
Conversation
|
*beep* *bop* Hi, human. The Click here to see the build log. |
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
*beep* *bop* Hi, human. I'm the @tardis-bot and couldn't find your records in my database. I think we don't know each other, or you changed your credentials recently. Please add your name and email to In case you need to map an existing alias, follow this example. |
4 similar comments
|
*beep* *bop* Hi, human. I'm the @tardis-bot and couldn't find your records in my database. I think we don't know each other, or you changed your credentials recently. Please add your name and email to In case you need to map an existing alias, follow this example. |
|
*beep* *bop* Hi, human. I'm the @tardis-bot and couldn't find your records in my database. I think we don't know each other, or you changed your credentials recently. Please add your name and email to In case you need to map an existing alias, follow this example. |
|
*beep* *bop* Hi, human. I'm the @tardis-bot and couldn't find your records in my database. I think we don't know each other, or you changed your credentials recently. Please add your name and email to In case you need to map an existing alias, follow this example. |
|
*beep* *bop* Hi, human. I'm the @tardis-bot and couldn't find your records in my database. I think we don't know each other, or you changed your credentials recently. Please add your name and email to In case you need to map an existing alias, follow this example. |
|
@DeekshaMohanty can you please fix the |
|
*beep* *bop* 7 G004 [ ] Logging statement uses f-string
2 I001 [*] Import block is un-sorted or un-formatted
2 W291 [*] Trailing whitespace
2 W605 [*] Invalid escape sequence: `\A`
1 RET505 [ ] Unnecessary `else` after `return` statement
1 RET506 [ ] Unnecessary `else` after `raise` statement
1 E902 [ ] No such file or directory (os error 2)
1 E999 [ ] SyntaxError: Expected an expression
1 F401 [*] `IPython.display.display` imported but unused
1 UP004 [*] Class `FilterLog` inherits from `object`
1 UP008 [*] Use `super()` instead of `super(__class__, self)`
1 TRY300 [ ] Consider moving this statement to an `else` block
Complete output(might be large): .mailmap:1:38: E999 SyntaxError: Expected an expression
docs/quickstart.ipynb:cell 12:1:39: W291 [*] Trailing whitespace
docs/quickstart.ipynb:cell 12:5:35: W291 [*] Trailing whitespace
docs/quickstart.ipynb:cell 16:10:26: W605 [*] Invalid escape sequence: `\A`
docs/quickstart.ipynb:cell 16:11:40: W605 [*] Invalid escape sequence: `\A`
tardis/io/logger/colored_logger.py:1:1: E902 No such file or directory (os error 2)
tardis/io/logger/logger.py:1:1: I001 [*] Import block is un-sorted or un-formatted
tardis/io/logger/logger.py:257:17: UP004 [*] Class `FilterLog` inherits from `object`
tardis/simulation/base.py:1:1: I001 [*] Import block is un-sorted or un-formatted
tardis/simulation/base.py:8:29: F401 [*] `IPython.display.display` imported but unused
tardis/simulation/base.py:155:14: UP008 Use `super()` instead of `super(__class__, self)`
tardis/simulation/base.py:199:13: RET506 Unnecessary `else` after `raise` statement
tardis/simulation/base.py:263:17: G004 Logging statement uses f-string
tardis/simulation/base.py:270:9: RET505 Unnecessary `else` after `return` statement
tardis/simulation/base.py:451:13: G004 Logging statement uses f-string
tardis/simulation/base.py:549:13: G004 Logging statement uses f-string
tardis/simulation/base.py:638:21: G004 Logging statement uses f-string
tardis/simulation/base.py:641:13: G004 Logging statement uses f-string
tardis/simulation/base.py:646:13: G004 Logging statement uses f-string
tardis/simulation/base.py:697:13: TRY300 Consider moving this statement to an `else` block
tardis/simulation/base.py:699:26: G004 Logging statement uses f-string
Found 21 errors.
[*] 8 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option).
|
|
Points 6 and 7 for the issue linked to this PR need some further discussion. #2701 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2700 +/- ##
==========================================
- Coverage 70.99% 70.87% -0.13%
==========================================
Files 209 208 -1
Lines 15650 15639 -11
==========================================
- Hits 11111 11084 -27
- Misses 4539 4555 +16 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
This is a great addition @DeekshaMohanty - thanks for taking initiative on this!
I've left some comments, please address them when you get a chance. We need to make sure that we address all the points in #2701
| @@ -116,7 +116,7 @@ | |||
| "sim = run_tardis(\"tardis_example.yml\", \n", | |||
| " virtual_packet_logging=True,\n", | |||
| " show_convergence_plots=True,\n", | |||
| " export_convergence_plots=True,\n", | |||
| " export_convergence_plots=False, #TODO: to avoid double plots\n", | |||
There was a problem hiding this comment.
Maybe we need to replace this parameter with a new parameter (maybe called export_static_output) which can show convergence plots and logger-widget (or atleast a fixed height html text area) for static HTML documentation website.
And setting this param to True should be controllable by identifying if the notebook execution is being done by nbsphinx/nbconvert - there should be some setting to detect that but needs research.
tardis/io/logger/logger.py
Outdated
|
|
||
| logging_level = ( | ||
| log_level if log_level else tardis_config["debug"]["log_level"] | ||
| tardis_config["debug"].get("specific_log_level", specific_log_level) |
tardis/io/logger/logger.py
Outdated
| "WARNING/ERROR": Output(layout=Layout(height='300px', overflow_y='auto')), | ||
| "INFO": Output(layout=Layout(height='300px', overflow_y='auto')), | ||
| "DEBUG": Output(layout=Layout(height='300px', overflow_y='auto')), | ||
| "ALL": Output(layout=Layout(height='300px', overflow_y='auto')) |
There was a problem hiding this comment.
you can define a function that returns a Output widget and call for each of the keys, instead of repeating code.
tardis/io/logger/logger.py
Outdated
| tab.set_title(2, "DEBUG") | ||
| tab.set_title(3, "ALL") | ||
|
|
||
| display(tab) |
There was a problem hiding this comment.
do we really need to do it global level? shouldn't display be inside some function/class that gets called/instantiated?
If not, maybe move it together with other global level code at L184
tardis/io/logger/logger.py
Outdated
| elif record.levelno == logging.DEBUG: | ||
| color = 'blue' | ||
| else: | ||
| color = 'black' |
There was a problem hiding this comment.
it will be cleaner if colors come from a dictionary, instead of defining here
tardis/simulation/base.py
Outdated
| @@ -517,7 +517,7 @@ def log_plasma_state( | |||
| plasma_state_log["next_w"] = next_dilution_factor | |||
| plasma_state_log.columns.name = "Shell No." | |||
|
|
|||
| if is_notebook(): | |||
| if False and is_notebook(): #TODO: remove it with something better | |||
There was a problem hiding this comment.
I think we can simply remove the if block since we are controlling display now
…NG/ERROR" instead of the initial five tabs
… INFO, DEBUG and ALL.
…8, where log level "ALL" was not added.
2. Moved the display logic to a function 3. Used a dictionary to manage colors for log levels. 4. Removed the if block from base.py
DeekshaMohanty
force-pushed
the
logger_widget
branch
from
a992a4b
to
a95096c
Compare
| display(tab) | ||
|
|
||
|
|
||
| def remove_ansi_escape_sequences(text): |
There was a problem hiding this comment.
Why do we need this function?
| return ansi_escape.sub("", text) | ||
|
|
||
|
|
||
| class WidgetHandler(logging.Handler): |
There was a problem hiding this comment.
Not a good name. Should be more specific for logging.
| html_output = clean_log_entry | ||
|
|
||
| if record.levelno in (logging.WARNING, logging.ERROR): | ||
| with log_outputs["WARNING/ERROR"]: |
There was a problem hiding this comment.
Repetitive code. Would be better as some kind of dictionary.
|
|
||
| logger.addHandler(widget_handler) | ||
| logging.getLogger("py.warnings").addHandler(widget_handler) | ||
| create_and_display_log_tab(log_outputs) |
There was a problem hiding this comment.
The lines 233-254 are strange because they are not in a class or function. Why are they set up like this? There seems to be a lot of calls to logger and logging.
| create_and_display_log_tab(log_outputs) | ||
|
|
||
|
|
||
| class FilterLog(object): |
There was a problem hiding this comment.
Is a full class necessary here, or could it just be a filter function?
|
has been superseded by panel implementation |
📝 Description
Fixes #2701
Type: 🚀
feature🚦 Testing
How did you test these changes?
☑️ Checklist
build_docslabel