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

Curses UX renders during @console_rule execution with interleaved console output #6571

Closed
stuhood opened this issue Sep 28, 2018 · 2 comments
Assignees

Comments

@stuhood
Copy link
Member

stuhood commented Sep 28, 2018

A @console_rule is implemented as a normal @rule with a private/anonymous output type. To run a @console_rule, we request the private/anonymous type for the "goal" (ie, list) in Session.run_console_rules, and triggers a normal execution of the engine with the python Scheduler.product_request method, which eventually calls into the rust Scheduler::execute method.

To make an initial interleaved ([0]: see below) integration of the curses UX from #6223, I think that what we'll want to do is add parameter(s) to the Scheduler::execute method that indicate whether to render the UX. If the parameter(s) indicates that we should render the UX (possibly with additional information about "where" to render it, depending on how we want to integrate with testing), we should construct a Display to use, similar to how this is accomplished in the demo UX. The Display struct itself is relatively light, so just creating one for each call to execute should be fine.

Still in Scheduler::execute, we should adjust the loop added by #6570 to:

  1. poll the Future representing the entire execution using a poll timeout that matches the rendering frequency we want
  2. call the Display::render method on the display, and render the output of self.core.graph.heavy_hitters in a fashion similar to the loop in the UI demo

At the end of Scheduler::execute, we should call Display::finish, as in the demo.


One medium-size sticking point is that heavy_hitters takes a parameter corresponding to the "top-k most relevant running nodes reachable from some roots". This is because the graph executes asynchronously, and thus contains thousands of concurrently running tasks. Rather than rendering all of them, we should choose a k that corresponds to the number of cores on the host. Then, since the Display (currently) operates in terms of "workers" (think threads), we need to map the k output tasks we get onto the Display's idea of workers. A few potential approaches to resolving this:

  1. Sort the heavy_hitter task strings after they have been computed, and display.update k synthetic workers using their indexes (ie, pretend that the k heavy hitters are running on k threads, with no attempt at stability other than sorting). This is the easiest one, and is probably what the first cut of the PR should do.
  2. Maintain a Map<(heavy hitter task), (worker id)> outside of Display, and "assign" a task to a synthetic worker for as long as it remains in the top k. This is significantly more stable/readable than (1), because a long running task will stay in the same (relative) position on the screen.
  3. Refactor Display to not actually uniquely identify workers, and instead internally manage something similar to (2), to cause a task to stay in a stable position for multiple consecutive render calls. This option is more handwavey, but probably mostly still looks like (2).

The end result of this PR should be that a command like:

./pants --no-v1 --v2 list ::

...renders the curses UX while the engine is doing work.

It's likely that for the purposes of testing, or when a TTY is not detected, that a global option will be needed that renders a simplified form of the output that can be more easily tested in https://github.com/pantsbuild/pants/blob/56cb981afb5c0a9118fb64fca70c394fce8c33d6/tests/python/pants_test/engine/legacy/test_console_rule_integration.py


[0] This issue does not discuss actually handling the interleaving of python @rule Console or Logger output with the curses UX: this will mean that if an @rule triggers log messages, or if a @console_rule renders output to the console, it will likely "break" the ascii code used by the UI. This is fine for the purposes of this issue, and will be handled in a followup issue: #6004.

@stuhood stuhood added P3 - M2 and removed P3 - M1 labels Sep 28, 2018
stuhood pushed a commit that referenced this issue Sep 29, 2018
### Problem

The root futures of an engine request currently execute directly on the main thread using the built in "block this thread" executor. Having this work block the main thread makes it challenging to:

1. support doing other work with the main thread (which is the easiest place to render output), as in #6571.
2. support cleanly exiting when we are interrupted, as in #6368.

### Solution

Run root futures on the tokio runtime's executor, and have the futures send their result back to the main thread via an `mpsc` queue. The main thread can poll the queue at whatever frequency it wants while simultaneously checking for kill switches or rendering output.
@wisechengyi wisechengyi self-assigned this Oct 16, 2018
@wisechengyi
Copy link
Contributor

iiuc this Display isn't hooked up with the engine yet (#6223 (review)). Should I prioritize this first (likely behind a flag), then incrementally add more features requested on the same review?

@stuhood
Copy link
Member Author

stuhood commented Oct 16, 2018

@wisechengyi : That's mentioned above, somewhat:

The Display struct itself is relatively light, so just creating one for each call to execute should be fine.

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

No branches or pull requests

2 participants