Optionally render the `--dynamic-ui` with Prodash #14221

thejcannon · 2022-01-20T20:59:12Z

I've taken @stuhood 's PR #13481 and adapted it to hide behind an experimental flag.

I can't say I understand some/most/any of Stu's changes 😉 but if needed I'm happy to learn ❤️

~~Also, I've done my best to make commits incremental, so it's likely worth reviewing commits.~~

stuhood · 2022-01-20T21:03:02Z

Thanks a lot! Yea, feel free to run with this: I can close the original in favor of this one.

src/rust/engine/ui/src/console_ui.rs

stuhood

Overall this looks great! If you need help getting it green, let me know, but looks like you're on the right track.

src/python/pants/option/global_options.py

thejcannon

@stuhood I'm going to open issues for the TODOs so they can be discussed

src/rust/engine/ui/src/console_ui.rs

thejcannon · 2022-01-21T20:17:06Z

So the only difference (other than the obvious "style" of swimlane) this provides is the fact that the prodash implementation seemingly leaves the terminal at the lowest row, whereas when the indicatif implementation finishes, there's usually some N rows left empty below the current terminal.

I consider the prodash behavior ideal.

(Although that might change if we try and fix #14093 with prodash, I don't understand the underlying behavior of these very well)

src/rust/engine/workunit_store/src/lib.rs

stuhood

Thanks, looks good!

Let me know when it's otherwise ready to land, and I can push a change for the stderr race.

src/rust/engine/ui/Cargo.toml

src/python/pants/engine/internals/native_engine.pyi

src/rust/engine/ui/src/console_ui.rs

stuhood · 2022-01-25T22:25:00Z

src/rust/engine/ui/src/console_ui.rs

+        indicatif
+          .multi_progress_task
+          .map_err(|e| format!("Failed to render UI: {}", e))
+          .boxed();


Suggested change

indicatif

.multi_progress_task

.map_err(|e| format!("Failed to render UI: {}", e))

.boxed();

indicatif

.multi_progress_task

.map_err(|e| format!("Failed to render UI: {}", e))

.await

I think that this needs to await the multi_progress_task, but I'm surprised that you're not getting an error for an unused/unawaited future here, so I might be wrong.

Adding the await leads to compiler warnings/errors which I don't quite understand.

I'm going to attempt a refactor to have both cases here share some code, so this might get unified with prodash. We'll see.

OK yeah awaiting causes some kind of deadlock or equivalent "nothing is happening" scenario 😒

This blocks landing, because otherwise the UI will not necessarily be torn down properly before we move on to doing other things, which could trigger race conditions. If this were prodash, then that might be ok... but can't land that for the default.

I'll take a look.

thejcannon · 2022-01-26T19:39:07Z

Moving out of draft state @stuhood

thejcannon · 2022-01-27T01:38:16Z

src/rust/engine/ui/src/console_ui.rs

+    let classify_tasks = |mut current_ids: HashSet<SpanId>,
+                          handler: &mut dyn FnMut(SpanId, TaskState)| {
+      for span_id in heavy_hitters.keys() {
+        let update = current_ids.remove(span_id);
+        handler(
+          *span_id,
+          if update {
+            TaskState::Update
+          } else {
+            TaskState::New
+          },
+        )
+      }
+      for span_id in current_ids {
+        handler(span_id, TaskState::Remove);
+      }
+    };


It's way out of scope for this PR, but honestly heavy_hitters() could be refactored to be more like this lambda, where it simply calls into a function param with the relevant info.

Likely to be a performance bump from not constructing and moving the HashMap.

Yea, there is absolutely optimization that can happen here. But I expect that it will be easier to do once we've reduced back to one implementation: prodash's datastructure might mean that we don't even need to keep the "running" graph... or can directly keep prodash nodes in the "running" graph.

Ah yes, that would truly be ideal. 🙌

[ci skip-build-wheels]

Eric-Arellano · 2022-01-28T01:23:19Z

What's the path forward to stabilizing Prodash? It looks awesome, I'm excited to try it out.

If several of us maintainers / early adopters try it this week without issue, how about we switch the default to Prodash in 2.10 before stable release? Still keep the feature so users have an escape hatch. Then in 2.11 we drop the old UI?

stuhood · 2022-01-28T03:22:10Z

What's the path forward to stabilizing Prodash? It looks awesome, I'm excited to try it out.

If several of us maintainers / early adopters try it this week without issue, how about we switch the default to Prodash in 2.10 before stable release? Still keep the feature so users have an escape hatch. Then in 2.11 we drop the old UI?

I haven't spent enough time using it to get a good sense for when it will be ready to become the default. I do think that it could potentially become the default within pantsbuild/pants sooner (next few days), but I'm also not sure that now is necessarily the ideal time to be making that push (unless @thejcannon feels like he can dedicate some time to it?).

I agree that we should have a path toward stabilizing/making-default though: I'd tentatively suggest that it should either become the default or be removed within two or three stable releases, even if we don't start the push today.

thejcannon · 2022-01-28T12:54:34Z

At a minimum we'll want to fix

pants/src/rust/engine/ui/src/console_ui.rs

Line 190 in 4335962

// TODO: Apply a fix similar to #14093 before landing.

before turning it on in this repo.

It's probably also not a bad idea to try and get Stu's changes to prodash in prodash itself. We can probably help that along.

Afterwards, I think the path forward would be much clearer.

stuhood · 2022-03-03T00:38:50Z

FWIW, I've been using this mostly successfully, but Byron/prodash#10 is an issue, and breaks rendering for longer messages. We'd need to fix it, or truncate ourselves... not the end of the world.

But in general, I don't think that it's currently in a place where it is better than indicatif. We'd need to invest time in actually using the nesting to improve rendering... but I'm not going to be in that place in the next few weeks.

I'm going to switch back to the default until we have time to improve it. But if it is hampering our ability to ship any improvements elsewhere before we have time to fix the issues, I think that we should bias toward removing it.

Eric-Arellano · 2022-03-03T01:54:52Z

Ah, I've been using this for the past month too and indeed encountered Byron/prodash#10 but kept punting on opening an issue, glad to see that's accounted for!

thejcannon commented Jan 20, 2022

View reviewed changes

src/rust/engine/ui/src/console_ui.rs Outdated Show resolved Hide resolved

stuhood reviewed Jan 20, 2022

View reviewed changes

src/python/pants/option/global_options.py Outdated Show resolved Hide resolved

thejcannon commented Jan 20, 2022

View reviewed changes

src/rust/engine/ui/src/console_ui.rs Outdated Show resolved Hide resolved

src/rust/engine/ui/src/console_ui.rs Outdated Show resolved Hide resolved

thejcannon changed the title ~~WIP: Stu's Dyanmic UI with Prodash PR, but under a flag~~ WIP: Dynamic UI with Prodash Jan 20, 2022

thejcannon commented Jan 21, 2022

View reviewed changes

src/rust/engine/ui/src/console_ui.rs Outdated Show resolved Hide resolved

thejcannon commented Jan 21, 2022

View reviewed changes

src/rust/engine/ui/src/console_ui.rs Outdated Show resolved Hide resolved

thejcannon commented Jan 24, 2022

View reviewed changes

src/rust/engine/workunit_store/src/lib.rs Outdated Show resolved Hide resolved

This was referenced Jan 25, 2022

Render dynamic UI with prodash #13481

Closed

PoC: Adding goal name to dynamic UI output #14249

Closed

stuhood approved these changes Jan 25, 2022

View reviewed changes

thejcannon added 2 commits January 25, 2022 20:01

rebase, sorry my history got rekd

d3604e2

Most issues addressed

2172020

thejcannon force-pushed the prodash branch from c9d3197 to 2172020 Compare January 26, 2022 15:01

thejcannon added 2 commits January 26, 2022 12:08

Helper for renfer

103aefc

fmt

3f870ae

thejcannon marked this pull request as ready for review January 26, 2022 19:39

thejcannon added 3 commits January 26, 2022 13:42

pyi

dd295e2

clippy

c355f48

clippy again

859e684

thejcannon commented Jan 27, 2022

View reviewed changes

stuhood changed the title ~~WIP: Dynamic UI with Prodash~~ Optionally render the --dynamic-ui with Prodash Jan 27, 2022

Await teardown for Indicatif.

fa3264d

[ci skip-build-wheels]

stuhood enabled auto-merge (squash) January 28, 2022 00:33

auto-merge was automatically disabled January 28, 2022 01:06
Pull Request is not mergeable

stuhood merged commit 4335962 into pantsbuild:main Jan 28, 2022

thejcannon deleted the prodash branch March 3, 2022 00:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optionally render the `--dynamic-ui` with Prodash #14221

Optionally render the `--dynamic-ui` with Prodash #14221

thejcannon commented Jan 20, 2022 •

edited

Loading

stuhood commented Jan 20, 2022

stuhood left a comment

thejcannon left a comment

thejcannon commented Jan 21, 2022 •

edited

Loading

stuhood left a comment

stuhood Jan 25, 2022

thejcannon Jan 26, 2022

thejcannon Jan 26, 2022

stuhood Jan 27, 2022 •

edited

Loading

thejcannon commented Jan 26, 2022

thejcannon Jan 27, 2022 •

edited

Loading

stuhood Jan 28, 2022 •

edited

Loading

thejcannon Jan 28, 2022

Eric-Arellano commented Jan 28, 2022

stuhood commented Jan 28, 2022 •

edited

Loading

thejcannon commented Jan 28, 2022

stuhood commented Mar 3, 2022 •

edited

Loading

Eric-Arellano commented Mar 3, 2022

Optionally render the --dynamic-ui with Prodash #14221

Optionally render the --dynamic-ui with Prodash #14221

Conversation

thejcannon commented Jan 20, 2022 • edited Loading

stuhood commented Jan 20, 2022

stuhood left a comment

Choose a reason for hiding this comment

thejcannon left a comment

Choose a reason for hiding this comment

thejcannon commented Jan 21, 2022 • edited Loading

stuhood left a comment

Choose a reason for hiding this comment

stuhood Jan 25, 2022

Choose a reason for hiding this comment

thejcannon Jan 26, 2022

Choose a reason for hiding this comment

thejcannon Jan 26, 2022

Choose a reason for hiding this comment

stuhood Jan 27, 2022 • edited Loading

Choose a reason for hiding this comment

thejcannon commented Jan 26, 2022

thejcannon Jan 27, 2022 • edited Loading

Choose a reason for hiding this comment

stuhood Jan 28, 2022 • edited Loading

Choose a reason for hiding this comment

thejcannon Jan 28, 2022

Choose a reason for hiding this comment

Eric-Arellano commented Jan 28, 2022

stuhood commented Jan 28, 2022 • edited Loading

thejcannon commented Jan 28, 2022

stuhood commented Mar 3, 2022 • edited Loading

Eric-Arellano commented Mar 3, 2022

Optionally render the `--dynamic-ui` with Prodash #14221

Optionally render the `--dynamic-ui` with Prodash #14221

thejcannon commented Jan 20, 2022 •

edited

Loading

thejcannon commented Jan 21, 2022 •

edited

Loading

stuhood Jan 27, 2022 •

edited

Loading

thejcannon Jan 27, 2022 •

edited

Loading

stuhood Jan 28, 2022 •

edited

Loading

stuhood commented Jan 28, 2022 •

edited

Loading

stuhood commented Mar 3, 2022 •

edited

Loading