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

BSP: Streaming compilation and test results asynchronously to IDEs #14885

Closed
tdyas opened this issue Mar 23, 2022 · 2 comments · Fixed by #15415
Closed

BSP: Streaming compilation and test results asynchronously to IDEs #14885

tdyas opened this issue Mar 23, 2022 · 2 comments · Fixed by #15415
Assignees
Labels
backend: JVM JVM backend-related issues bsp estimate: <1W

Comments

@tdyas
Copy link
Contributor

tdyas commented Mar 23, 2022

Background

The BSP protocol provides a mechanism for BSP servers to send streaming results of compilation tasks and test tasks to BSP
clients (i.e., IDEs) via async notifications over the BSP connection. Task start and finish notifications are mandatory; progress notifications are recommended.

Pants currently streams compilation results during ./pants check as a side effect of the fact that the various backend-specific types used for compilation implement EngineAwareReturnType. If an error occurs, the type implementing EngineAwareReturnType will change its level to ERROR, thus causing the dynamic UI to render the error.

This approach may not work for BSP because for BSP Pants should send asynchronous notifications of not only build results and but also logs from build actions as they occur. It would also be good if the BSP rules were decoupled from the language backends so there was no direct knowledge of BSP in the language compilation rules.

BSP supports the following types of task-related notifications. A "task ID" in BSP is a unique ID that can also specify the task IDs for its parents. Thus, there is a parent-child relationship between tasks.

The types of task-related notifications include:

Task notifications can carry data related to the specific kind of task they represent. BSP predefines data for compilation and tests:

Design Needed

For this issue, we should design how streaming notifications of engine actions should occur in a way that is decoupled from the ultimate destination of those streaming notifications. The two initial users of this design would be the Pants dynamic UI and BSP rules.

@stuhood
Copy link
Member

stuhood commented Mar 30, 2022

At a high level, what I think this will look like is:

  1. Implementing EngineAwareReturnType for relevant BSP compilation types (either directly on FallibleClasspathEntry, or via a wrapper around that type, for example), and implementing EngineAwareReturnType.metadata by exposing BSP-specific objects which identify e.g. the build target and session.
    • The exposed metadata types should likely be fairly high level: BSP @rules which consume FallibleClasspathEntry should execute any parsing that is needed to store pre-parsed/interpreted metadata, so that the BSP goal's consumer can stay dumb as to how the metadata types are provided.
  2. Wrapping use of a StreamingWorkunitHandler WorkunitsCallback around the entire BSP BuiltinGoal, and correlating the user_metadata (as recorded in step 1) of received workunits back to particular tasks that kicked them off.
    • After matching on relevant user_metadata, other workunit fields like the name (usually: @rule method name) and artifacts like stdout/stderr can be consumed. But see 1, regarding exposing higher-level pre-interpreted data as metadata instead.

So, AFAICT, this should be unblocked, and the bulk of the work will involve figuring out where to add EngineAwareReturnType. @tdyas I can probably take a stab at that after a pairing session?

@stuhood stuhood added the bsp label Apr 4, 2022
@stuhood stuhood changed the title BSP design: streaming compilation and test results asynchronously to IDEs BSP: Streaming compilation and test results asynchronously to IDEs Apr 7, 2022
@stuhood
Copy link
Member

stuhood commented May 12, 2022

As mentioned in #15415, a full unification of the task and workunit models is deferred to #15426: as our BSP integration continues to evolve, it may still make sense. But it doesn't unblock anything currently.

stuhood added a commit that referenced this issue May 13, 2022
Adds BSP [task notifications](https://build-server-protocol.github.io/docs/specification.html#task-notifications) for compile tasks, to render individual Pants (coarsened) targets as progress within the compile for a particular BSP `BuildTarget`. 

#14885 discusses unifying the `StreamingWorkunitHandler` infrastructure with task notifications (which is expanded on in #15426). While that might still be a good idea in the medium term, deciding how to filter workunits to make them fully useful to BSP is challenging, and so was broken out into #15426.

Fixes #14885. 

[ci skip-rust]
[ci skip-build-wheels]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend: JVM JVM backend-related issues bsp estimate: <1W
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants