-
-
Notifications
You must be signed in to change notification settings - Fork 347
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
Multi-line status prompt ticker #3577
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lihaoyi
force-pushed
the
multi-prompt
branch
from
September 19, 2024 03:50
eb81ce7
to
64f8973
Compare
lihaoyi
changed the title
[WIP] Multi-line status ticker
Multi-line status prompt ticker
Sep 19, 2024
lihaoyi
force-pushed
the
multi-prompt
branch
from
September 26, 2024 04:02
0a06d54
to
3a8a4e7
Compare
Merged
lihaoyi
added a commit
that referenced
this pull request
Sep 26, 2024
This pulls in the new prompt from #3577 so we can try it out internally
lihaoyi
added a commit
that referenced
this pull request
Oct 9, 2024
…3697) This re-implements the ticker log prefixes that were left out in #3577, both for meta-build prefixes and nested-evaluation prefixes, e.g. the log prefix now looks like `[build.mill-1]` `[mill-build/build.mill-1]` etc. * We no longer need `dynamicTickerPrefix`, because we now perform this context management by wrapping nested `PrefixLogger`s * Required some gross mangling to consolidate `Logger` and `ColorLogger` interfaces in order to make things properly stackable * We needed to move the aggregation of keys to a top-down process, rather than bottom up, since it is not easy to take an already prefixed line `[1] my logs line` and append to the prefix `[1-2] my logs line`. Thus we need to make sure the log line is properly prefixed at the bottom-most logger, and the log line is passed up via `unprefixedSystemStreams` to avoid redundant prefixing <img width="976" alt="Screenshot 2024-10-09 at 7 36 05 PM" src="https://github.com/user-attachments/assets/edc78141-5c09-4b5e-85ab-4cfda1603003">
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #3546
This PR introduces a new
MultilinePromptLogger
that allows the status of multiple in-progress threads to be reported and viewed during a Mill evaluation:Screen.Recording.2024-09-24.at.3.01.58.PM.mov
MultilinePromptLogger
is basically a minimal translation of the currentticker
API to work with a multi-line prompt. There's probably a lot of other interesting improvements we can make now that we are multi-line, but this is a decent start that lets people know what their parallel build is doing. The UI works correctly both during scrolling and not-scrolling, and uses the same minimal ANSI codes that the existing ticker uses, so hopefully we won't hit any more edge cases that we aren't already hitting todayFrom an implementation perspective,
MultilinePromptLogger
is mostly a drop in replacement for the oldPrintLogger
.User-facing notes:
It prints multiple
ticker
lines at vertically at the bottom of the terminal, and has the logs printed aboveIt requires that you use
.endTicker()
after every.ticker("...")
call, so we can know where we should clear the ticker status (previously they always just got overriden by the next.ticker
call)We need to introduce a
withPaused{...}
block so that when you're running REPLs and stuff the prompt is not shownWe only can support logs which end with newlines. This means things like interactive progress bars and other ANSI magic won't work in the scrollback. This is a limitation of the current implementation that is hard to fix without going for more advanced techniques, but should be enough for the vast majority of logs and is a similar limitation as a lot of other tools.
Console dimensions are propagated from the mill client to the mill server via a
terminfo
file specific to each server. This is used to size the prompt so it takes up the whole width and about 1/3 of the height. This happens every 100ms, so there may be some delay, but at least it means the prompt will right-size itself on next render. The regular cadence also means it shouldn't be a performance bottleneckFor non-interactive runs which lack a terminal, prompt WxH defaults to 120x50, and we tweak the rendering: we no longer render blank lines to preserve prompt height stability, we render less frequently and only if the statuses change, and we add a footer so the bottom of the prompt is clearly marked.
--ticker
now defaults totrue
even for non-interactive runs, since the periodic display of prompt (I set to 60s intervals, if prompt changes) is useful e.g. for debugging stuck background jobs or CI runs.Implementation Notes
The logger needs to extend
AutoCloseable
, since it uses a background thread to keep the prompt UI up to date. This renders every 100ms (arbitrary) to try and balance between prompt readability and latency. We have additional delays built in to hiding a status line and then finally removing it, to try and preserve the height so it doesn't bounce up and down too much as the set of running tasks changesWe re-use the
ProxyStream
classes to combine the stderr/stdout before re-splitting them. This allows us to perform some buffering, while simultaneously maintaining ordering of writes to each stream, while also allowing us to detect quiescence so we can only bother writing out a prompt when everything else is done and it won't get immediately over-written.ProxyStream.Pumper
needed some tweaks to add hooks and remove client-specific assumptionsI experimented with having the ticker logic live in the Mill client rather than server, which would make more sense, but we need the server to have the ability to enable/disable the ticker logic to run
console
and similar interactive tasks, and so it has to live in the serverThe old ticker remains available at
--disable-prompt
. Further improvements can come after 0.12.0.