mne-tools / mne-bids-pipeline

Automatically process entire electrophysiological datasets using MNE-Python.
https://mne.tools/mne-bids-pipeline/
BSD 3-Clause "New" or "Revised" License
140 stars 67 forks source link

ENH: More compact logging #764

Closed larsoner closed 1 year ago

larsoner commented 1 year ago

Before merging …

@hoechenberger feel free to review/merge if you're happy.

I have been bothered by too much redundancy and overuse of horizontal space in our logging recently. Now that we use rich we can make better use of rule to avoid redundancy. In particular, currently we print the name of the step at each logging line and it takes up almost a third of the horizontal space (here shown in 132x43 terminal, the largest in the Ubuntu Linux default Terminal drop-down):

Screenshot from 2023-07-17 13-35-08

With .rule we can cleanly log the start of each step as a title and link everything using a consistent color instead (green):

Screenshot from 2023-07-17 14-17-41

So I think overall this becomes both easier to follow and more compact / less redundant.

Also adds [link] support to make the Report HTML Files clickable.

hoechenberger commented 1 year ago

I actually prefer the horizontal dividers that expand across the entire screen. Is that an option for you still?

larsoner commented 1 year ago

The horizonal dividers (rules) are still there. Can you clarify what you mean?

larsoner commented 1 year ago

... you mean without the title in the rule itself? Yes we can do that instead of having the title in the rule. It's a little less compact vertically but it'll work. I'll try that and attach a screenshot to see if it's more what you're looking for

larsoner commented 1 year ago
  1. Current solution:

    Screenshot 2023-07-18 at 7 13 49 AM
  2. Remove emoji from title in rule:

    Screenshot 2023-07-18 at 7 16 42 AM
  3. Rule all the way across:

    Screenshot 2023-07-18 at 7 25 18 AM

I prefer (1) or (2) over (3), but if you feel strongly about it I can live with (3). Or maybe @drammock can weigh in and decide!

drammock commented 1 year ago

Of the options shown I prefer 2. I would further do the same thing with "done" lines that was done in titles (remove emoji and set "done (58 s)" in green within a rule. And (no surprise here) I'd remove all emojis (what's the value / added information of seeing a stack of hourglass emojis?). My 2 cents

larsoner commented 1 year ago

And (no surprise here) I'd remove all emojis (what's the value / added information of seeing a stack of hourglass emojis?).

In typical (re-)runs they do add information and are not always hourglasses (see below)

I would further do the same thing with "done" lines that was done in titles (remove emoji and set "done (58 s)" in green within a rule.

This part I'm not a big fan of because I think it's a bit distracting having what ends up being double separated lines (not 100% to spec but you get the idea):

Screenshot 2023-07-18 at 8 02 47 AM

But a varaint of this would be to draw "over to" the done line using a box char which helps end the section (now my fav I think):

Screenshot 2023-07-18 at 8 08 32 AM

To me it's very easy to see where each section begins and ends, doesn't waste space, etc. But I can live with the double-bar separation if people want.

hoechenberger commented 1 year ago

But a varaint of this would be to draw "over to" the done line using a box char which helps end the section (now my fav I think):

I like this approach, +1 from me to go this route!

hoechenberger commented 1 year ago

what's the value / added information of seeing a stack of hourglass emojis?

I agree. The original idea was to display the hourglasses while the step is still running, and changing the glass to a checkmark once the step has completed. But we never implemented it that way.

I believe this might now be possible thanks to our switch to rich. In fact, I would love to replace the hourglass with some sort of a spinner, and then change to a checkmark once done. @larsoner Would you be willing to look into options for doing this?

larsoner commented 1 year ago

I believe this might now be possible thanks to our switch to rich. In fact, I would love to replace the hourglass with some sort of a spinner, and then change to a checkmark once done. @larsoner Would you be willing to look into options for doing this?

I'd actually rather not do this -- CIs (and maybe logging tools?) don't respond well to this. In practical terms, our CircleCI logs will become terrible...

I guess we could add an option to disable such live updates. But to me having the time a thing started is enough not to worry about status. So I'd rather let someone motivated to do it work on it :)

larsoner commented 1 year ago

(Also FYI it can be hard to get line updating right in a parallel context... I think we eventually made it work with tqdm in MNE but it's one more complication we'd have to think about when implementing)

hoechenberger commented 1 year ago

In that case, could we leave the hourglasses for the time being? I'd like to revisit this later after doing some research regarding what's possible with rich. I believe it does have support for "parallel logging", but I've never used it. (But I might be mistaken)

larsoner commented 1 year ago

In that case, could we leave the hourglasses for the time being?

Yes and to me there is information that might justify keeping it even with live updating. Differnt step execution types -- skipped, cached, cached-but-forced-to-rerun, running, etc. -- have different icons so in reruns you can quickly see what recomputed. Maybe at some point clear enough text can replace this but in the meantime the emojis make it easy to group steps / see what was run versus skipped/cached quickly.

larsoner commented 1 year ago

... marking for merge-when-green in the meantime!

larsoner commented 1 year ago

(but @drammock let me know if you really want the double-separator and I can unmark, or we can do it in a follow-up PR. Or @drammock could be a good PR for you, too -- hopefully it's easy enough to see how to tweak stuff given this PR :) )

drammock commented 1 year ago

the double-line all the way across is not necessary. I'm happy with the line connecting to "done" and the "done" being green.