-
-
Notifications
You must be signed in to change notification settings - Fork 636
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
Add UI to engine execution #6647
Conversation
|
This doesn't block this PR, but it will definitely be an important followup. I suspect that this will significantly improve the readability of the UI, because with no ordering (or even with sorting) something that is rendered 100 times in a row (ie, for about 10 seconds) might hop around to 100 different positions in the task ordering. But again, not a blocker: easy as a followup. |
Existing issue: cursor is lost after any pants operation. |
I think that that qualifies as the "interleaving" mentioned on the ticket. Fixing that is out of scope for this one. EDIT: Oh, nevermind: I see what you mean. But yes: fine with fixing that in a followup. |
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.
Thanks! Looks great.
src/rust/engine/ui/src/display.rs
Outdated
@@ -244,7 +244,7 @@ impl EngineDisplay { | |||
|
|||
fn render_for_pipe(&self) { | |||
// TODO: Handle non-tty output w polling interval adjustment + summary rendering. | |||
panic!("TODO"); | |||
// Nothing needs to be printed to pipe by default. |
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.
So, while this definitely shouldn't panic, I think that rather than disabling this at this level, we should ensure that when we're inspecting the "should we render the UX" global option, we check whether we're in a tty, and then we explicitly disable the UI rendering if we do not have a TTY.
This information is passed from the pantsd client to the pantsd server via: https://github.com/pantsbuild/pants/blob/master/src/python/pants/java/nailgun_protocol.py#L285-L314 ... but I think that the gist is still that looking at isatty()
for stdin/stderr/stdout
should tell you whether to render.
Having done that, we should maybe just remove this method entirely if possible...?
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.
Should've clarify a bit that this is not required for this change. Just that I don't want pants to crash on pipe. There is already logic above it which does not call this function at all if it is tty.
pants/src/rust/engine/ui/src/display.rs
Lines 261 to 268 in 298d821
pub fn render(&mut self) { | |
// TODO: Split this fork out into sub-types of EngineDisplay. | |
if self.is_tty { | |
self.render_for_tty() | |
} else { | |
self.render_for_pipe() | |
} | |
} |
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. But if my understanding is correct, we should just remove support for attempting to render to a pipe entirely? Maybe that can be a followup ticket: to basically just have the EngineDisplay
constructor return a Result::Err
if you attempt to create a display against a pipe?
src/python/pants/engine/scheduler.py
Outdated
@@ -559,14 +560,15 @@ def products_request(self, products, subjects): | |||
product_results[product].append(state.value) | |||
return product_results | |||
|
|||
def product_request(self, product, subjects): | |||
def product_request(self, product, subjects, render_v2_engine_ui=False): |
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.
As discussed on #6654, I think that you should add a @console_rule
specific method here, and remove the TODO that Daniel added:
pants/src/python/pants/engine/scheduler.py
Lines 541 to 542 in 3d22e95
# TODO: consider adding a new top-level function adjacent to products_request used for running console tasks, | |
# so that this code doesn't need to exist in this form. |
The idea would be that: a call to product_request(s)
would never render the UI, but a call to this new method would, and would also address that TODO. It would also not support batching like product_requests
(plural) does.
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.
Made a new method specifically for console rules. However it is not clear to me whether the entire if
statement was specifically for console rules, thus can be removed?
pants/src/python/pants/engine/scheduler.py
Lines 591 to 613 in 1b77e56
if throw_root_states: | |
unique_exceptions = tuple({t.exc for t in throw_root_states}) | |
# TODO: consider adding a new top-level function adjacent to products_request used for running console tasks, | |
# so that this code doesn't need to exist in this form. | |
if len(unique_exceptions) == 1 and isinstance(unique_exceptions[0], GracefulTerminationException): | |
raise unique_exceptions[0] | |
exception_noun = pluralize(len(unique_exceptions), 'Exception') | |
if self._scheduler.include_trace_on_error: | |
cumulative_trace = '\n'.join(self.trace(request)) | |
raise ExecutionError( | |
'{} encountered:\n{}'.format(exception_noun, cumulative_trace), | |
unique_exceptions, | |
) | |
else: | |
raise ExecutionError( | |
'{} encountered:\n {}'.format( | |
exception_noun, | |
'\n '.join('{}: {}'.format(type(t).__name__, str(t)) for t in unique_exceptions)), | |
unique_exceptions | |
) |
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.
Thanks!
src/rust/engine/ui/src/display.rs
Outdated
@@ -244,7 +244,7 @@ impl EngineDisplay { | |||
|
|||
fn render_for_pipe(&self) { | |||
// TODO: Handle non-tty output w polling interval adjustment + summary rendering. | |||
panic!("TODO"); | |||
// Nothing needs to be printed to pipe by default. |
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. But if my understanding is correct, we should just remove support for attempting to render to a pipe entirely? Maybe that can be a followup ticket: to basically just have the EngineDisplay
constructor return a Result::Err
if you attempt to create a display against a pipe?
Ready for another look. Thanks |
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.
Awesome, thanks!
:param product: product type for the request. | ||
:param subject: subject for the request. | ||
:param v2_ui: whether to render the v2 engine UI | ||
:return: A dict from product type to lists of products each with length matching len(subjects). |
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.
This return type is likely overkill, but easy to fix in a followup.
Initially connection between engine execution and the swimlane display. (#6571)
Limitation: having the same task show up in the same lane across updates isn't implemented.