Skip to content

Add logging widget to status bar #2928

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Dec 3, 2024
Merged

Conversation

k-dominik
Copy link
Contributor

@k-dominik k-dominik commented Nov 19, 2024

Log messages that are emitted using the log level 23 (or lazyflow.USER_LOGLEVEL) will show up in the status bar. Double clicking that area will open the log history.

image

Currently this will show the number of labels and oob in pixel classification, object classification (and related workflows - tracking...) as well as multicut.

  • Format code and imports.
  • Add docstrings and comments.
  • Add tests.
  • Update user documentation.
  • [couldn't find one] Reference relevant issues and other pull requests.
  • Rebase commits into a logical sequence.

Copy link

codecov bot commented Nov 19, 2024

Codecov Report

Attention: Patch coverage is 98.70130% with 1 line in your changes missing coverage. Please review.

Project coverage is 56.50%. Comparing base (cca71b7) to head (605636b).
Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
ilastik/shell/gui/ilastikShell.py 98.57% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2928      +/-   ##
==========================================
+ Coverage   56.45%   56.50%   +0.04%     
==========================================
  Files         535      535              
  Lines       62264    62310      +46     
  Branches     7711     7714       +3     
==========================================
+ Hits        35154    35208      +54     
+ Misses      25345    25342       -3     
+ Partials     1765     1760       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Log messages that are emitted using the log level 23 (or
lazyflow.USER_LOGLEVEL) will show up in the status bar. Double clicking
that area will open the log history.
@k-dominik k-dominik changed the title Logging widget Add logging widget to status bar Nov 19, 2024
@k-dominik k-dominik marked this pull request as ready for review November 19, 2024 16:06
@k-dominik k-dominik requested a review from btbest December 2, 2024 16:40
Copy link
Contributor

@btbest btbest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like it.

What do you think about clearing the bar when switching applets (in ilastikShell.setSelectedAppletDrawer I guess)? To me it feels a bit odd/unresponsive for the feedback to hang around there for the rest of the workflow; or respectively in Autocontext to hang around and then get replaced in stage 2.

# This level can be used to issue user-facing log messages
# log messages at this level get special treatment in ilastik:
# those are shown in the status bar
USER_LOGLEVEL = 23
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get that it's 23 to position it between INFO and WARN, and I think that's a good place. But look at the full message in the log:

Level 23 2024-12-02 18:15:34,494 parallelVigraRfLazyflowClassifier 4464 21876 Training took, 0.007 seconds

Such a good setup for an easter egg number. "Level 9001"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative way to implement this is to prepend a special well-known string prefix to a log message (could be something that still reads well in isolation, e.g. NOTICE: or USER INFO: ). This way, one could show WARN/ERROR messages in the status bar too if needed (maybe even in a red-ish color or something).

It is unfortunate that Python's default logging doesn't support attribute metadata (e.g. like the structlog package does).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emilmelnikov: Good point. I considered such a pattern but thought for some reason that concept would be even more alien than doing the custom log level. But since you also thought of it :)...

@btbest: had to google 9001 and am still not sure I get the reference :D...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Come to think of it, a more conventional way to do this would be to implement an ad-hoc structured logging: optionally prepend {attr1=val1,attr2=val2} to a message, then try to find and parse it into a dict:

def parse_log_attrs(line: str) -> tuple[dict[str, str], str]:
    prefix, sep, msg = line.partition(" ")
    if not (sep and len(prefix) >= 2 and prefix[0] == "{" and prefix[-1] == "}"):
        return {}, line
    attrs = {}
    for kv in prefix[1:-1].split(","):
        key, sep, val = kv.partition("=")
        if not (key and sep and val):
            return {}, line
        attrs[key] = val
    return attrs, msg

This is not as flexible as a full-blown proper JSON structured logging system, but it should be enough to smuggle a bunch of simple tags, and everything else should work as before.

Copy link
Contributor Author

@k-dominik k-dominik Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea and if we see that we use that "feature" (notifications-bar) more I think we should consider something like this. For now I prefer to keep it conceptually simple. But whenever we find we need more fine-grained control, we'll have this idea in the backs of our heads.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@btbest: had to google 9001 and am still not sure I get the reference :D...

It would be over 9000 😅

* removed necessary waitExposed in tests
* better naming: NotificationsBar / NotificationsWindow

Co-authored-by: Benedikt Best <[email protected]>
@k-dominik k-dominik requested a review from btbest December 3, 2024 15:21
Copy link
Contributor

@btbest btbest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

@k-dominik
Copy link
Contributor Author

thank you @btbest and @emilmelnikov for the discussions/reviews!

@k-dominik k-dominik merged commit 64229ec into ilastik:main Dec 3, 2024
16 checks passed
@k-dominik k-dominik deleted the logging-widget branch December 3, 2024 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants