-
Notifications
You must be signed in to change notification settings - Fork 161
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
Conversation
0d5f30f
to
a4f533e
Compare
a4f533e
to
b7f25a9
Compare
Codecov ReportAttention: Patch coverage is
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. |
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.
b7f25a9
to
f429d09
Compare
There was a problem hiding this 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 |
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* removed necessary waitExposed in tests * better naming: NotificationsBar / NotificationsWindow Co-authored-by: Benedikt Best <[email protected]>
6070ebf
to
91e5f76
Compare
Co-authored-by: Benedikt Best <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
thank you @btbest and @emilmelnikov for the discussions/reviews! |
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.Currently this will show the number of labels and oob in pixel classification, object classification (and related workflows - tracking...) as well as multicut.