Skip to content
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

ENH: More compact logging #764

Merged
merged 6 commits into from
Jul 18, 2023
Merged

ENH: More compact logging #764

merged 6 commits into from
Jul 18, 2023

Conversation

larsoner
Copy link
Member

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
Copy link
Member

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

@larsoner
Copy link
Member Author

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

@larsoner
Copy link
Member Author

... 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
Copy link
Member Author

  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
Copy link
Member

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

This reverts commit 075418c.
@larsoner
Copy link
Member Author

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
Copy link
Member

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!

Co-authored-by: Richard Höchenberger <[email protected]>
@hoechenberger
Copy link
Member

hoechenberger commented Jul 18, 2023

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
Copy link
Member Author

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 :)

Co-authored-by: Richard Höchenberger <[email protected]>
@larsoner
Copy link
Member Author

(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
Copy link
Member

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
Copy link
Member Author

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 larsoner enabled auto-merge (squash) July 18, 2023 13:11
@larsoner
Copy link
Member Author

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

@larsoner
Copy link
Member Author

(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 :) )

@larsoner larsoner merged commit 721162a into mne-tools:main Jul 18, 2023
@drammock
Copy link
Member

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

@larsoner larsoner deleted the log branch July 18, 2023 13:47
larsoner added a commit to larsoner/mne-bids-pipeline that referenced this pull request Jul 18, 2023
* upstream/main:
  ENH: More compact logging (mne-tools#764)
larsoner added a commit to larsoner/mne-bids-pipeline that referenced this pull request Jul 18, 2023
* upstream/main:
  ENH: More compact logging (mne-tools#764)
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.

4 participants