-
Notifications
You must be signed in to change notification settings - Fork 651
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
Coloured logs #4573
Coloured logs #4573
Conversation
✅ Deploy Preview for nextflow-docs-staging canceled.
|
Wondering if we can use the https://jline.github.io/jline2/apidocs/reference/jline/Terminal.html#isAnsiSupported() |
Signed-off-by: Phil Ewels <[email protected]>
Signed-off-by: Phil Ewels <[email protected]>
Signed-off-by: Phil Ewels <[email protected]>
Signed-off-by: Phil Ewels <[email protected]>
Skip processes with zero tasks if we are out of display rows Signed-off-by: Phil Ewels <[email protected]>
Signed-off-by: Phil Ewels <[email protected]>
Phil, I had an idea about how we could improve the percentage tracking. Until a process receives all the tasks it will execute, the percentage is always overestimating the true percentage. What if we show the percentage as |
I already implemented this event hook for array jobs and automatic cleanup, so just need to copy some code from there |
@bentsherman I go back and forth on this a little. I'm not sure which is best, the current output:
or what you're proposing:
Even if the I guess you could go halfway and remove the percentages and keep the
..but I sort of feel that you're still "lying" about the proportion of tasks done. So not sure how much of an improvement it is over the current behaviour. Am I missing something / wrong about anything here? |
The third option is what I'm proposing, but without the check mark:
The question marks in the percentage and the lack of a check mark tell you that this process hasn't received all of its tasks yet, so the true progress is unknown. I think that's reasonable |
Ah sorry, the check mark was me manually copying and pasting stuff for the example. That's not there in the current behaviour (see above screenshots). |
I see, well my point is basically the same |
Yup, agreed. I like it! Feel free to push whatever to this branch to add it 👍🏻 |
Signed-off-by: Ben Sherman <[email protected]>
Signed-off-by: Ben Sherman <[email protected]>
Okay that is done. Another idea I had related to this is to not include failed tasks in the percentage and total count. When a task fails and is retried, the percentage increases slightly (e.g. it goes from 3/4 = 75% to 4/5=80%), giving the illusion that the pipeline made progress when it didn't. Instead, the number of failed tasks should not influence either the total or completed count, they should just be noted to the side as they already are. |
Here's an example process that runs 4 tasks where 1 task fails and retries:
Then the process shows 100% when the retried task succeeds:
If instead the failed task is ignored, it simply reduces the number of total tasks, allowing the process to reach 100% while still noting the number of ignored tasks:
|
Can we keep change in the logic separated from the colouring? |
You mean like a separate PR? Sure |
Thanks |
d0a84ec
to
89455f2
Compare
@bentsherman are you ok to yank your commits out of this PR again? I'll leave it to you to handle that bit. Shout if you want me to do anything. |
Signed-off-by: Paolo Di Tommaso <[email protected]>
Signed-off-by: Paolo Di Tommaso <[email protected]>
modules/nextflow/src/main/groovy/nextflow/trace/AnsiLogObserver.groovy
Outdated
Show resolved
Hide resolved
Signed-off-by: Paolo Di Tommaso <[email protected]>
Signed-off-by: Paolo Di Tommaso <[email protected]>
final labelSpaces = tagMatch ? tagMatch.group(2) : '' | ||
final labelNoTag = LBL_REPLACE.matcher(label).replaceFirst("") | ||
final labelFinalProcess = labelNoTag.tokenize(':')[-1] | ||
final labelNoFinalProcess = labelFinalProcess.length() > 0 ? labelNoTag - ~/$labelFinalProcess$/ : labelNoTag |
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.
What labelNoTag - ~/$labelFinalProcess$/
does? just strips labelFinalProcess
from labelNoTag
?
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.
Yup - I'm basically breaking that string into components so that I can colour them differently. I wanted to highlight the final process name (after the last :
) as that's usually the most interesting bit.
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 added a chunk of comments a bit further up with an example, and what each variable will end up with.
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 think the regex is not necessary, have a look at feb0cc5
|
||
def fmt = ansi() | ||
fmt.a("\n") | ||
fmt.bg(Color.CYAN).fg(Color.BLACK).bold().a(" N E X T F L O W ").reset() |
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.
@pditommaso this isn't quite the same as the bgRgb
that was here before. The previous RGB value was the exact Nextflow colour and would appear the same on every terminal - irrespective of what theme is being used. The same for the fgRbg
with (0,0,0).
The problem with terminal colours is that "cyan" and "black" are interpreted differently by everyone's setup, depending on what theme they use. For example, in a dark mode termainal, "black" is actually... white 👀 By using the exact rgb values it always looks the same and it ensures that it'll be readable.
This is minor, but I wanted to point out why I did this particular bit the way I did (and why it was different to everything else).
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.
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.
Umm, this stuff is little more of a wrapper around an escape code. Considering this is the only place where it's used, is there any chance to find out the code and use it directly?
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.
Ok - Yup, can do 👍🏻
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.
Ok, got it to work after a bit of trial and error in 32e3a16
Now looks exactly as in the second screenshot above.
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.
Not working for me. Either we give up with the background color or we may need to use a different library for Ansi colors. This can be a better choice
Signed-off-by: Phil Ewels <[email protected]>
Use raw ANSI codes to get precise RGB codes. Signed-off-by: Phil Ewels <[email protected]>
Signed-off-by: Phil Ewels <[email protected]>
Signed-off-by: Paolo Di Tommaso <[email protected]>
Signed-off-by: Paolo Di Tommaso <[email protected]>
Signed-off-by: Paolo Di Tommaso <[email protected]>
Signed-off-by: Paolo Di Tommaso <[email protected]>
I suspect they have different escape codes .. |
I think it's because the Apple Terminal program only supports 256 colours, rather than Basically, Apple Terminal is kind of crap 😅 |
Signed-off-by: Phil Ewels <[email protected]>
You went wild! 😆 |
Signed-off-by: Paolo Di Tommaso <[email protected]>
Signed-off-by: Phil Ewels <[email protected]> Signed-off-by: Ben Sherman <[email protected]> Signed-off-by: Paolo Di Tommaso <[email protected]> Co-authored-by: Ben Sherman <[email protected]> Co-authored-by: Paolo Di Tommaso <[email protected]> Co-authored-by: Dr Marco Claudio De La Pierre <[email protected]> Signed-off-by: Niklas Schandry <[email protected]>
Signed-off-by: Phil Ewels <[email protected]> Signed-off-by: Ben Sherman <[email protected]> Signed-off-by: Paolo Di Tommaso <[email protected]> Co-authored-by: Ben Sherman <[email protected]> Co-authored-by: Paolo Di Tommaso <[email protected]> Co-authored-by: Dr Marco Claudio De La Pierre <[email protected]> Signed-off-by: Niklas Schandry <[email protected]>
To do:
-ansi false
is setNO_COLOR
See: #3976
I was never going to be able to back down from such a challenge 😆
CleanShot.2023-12-12.at.00.01.19-converted.mp4
Does a few things:
process >
) at < 180 cols width to save horizontal spaceKnown limitations:
NO_COLOR
env variables etc.