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

Add option to attach Subsystems to streaming workunits #8720

Merged
merged 6 commits into from
Dec 4, 2019

Conversation

gshuflin
Copy link
Contributor

@gshuflin gshuflin commented Nov 26, 2019

Problem

Now that we have the infrastructure to stream workunits while pants is running, we need a way to be able to tell some component of the code to do something useful with those workunits. We also want plugins to be able to receive streamed workunits.

Solution

A global Subsystem with a specific method handle_workunits can be registered to receive streaming workunits, by using the new global option --streaming-workunit-handlers, which expects a list of Python import paths as strings. At the beginning of a pants run, pants_local_runner will dynamically import all specified Subsystem classes, and every time StreamingWorkunitHandler receives workunits from the engine, it will pass them along to the handle_workunits method of all registered Subsystems.

The string arguments need to be a fully-qualified import path including the class of the Subsystem itself. For instance, ./pants --v2 --no-v1 --streaming-workunits-handlers="['pants.reporting.workunits.Workunits']" binary examples/src/python/example/hello/main/ will register a Subsystem subclass Workunits defined in the module pants.reporting.workunits to receive streaming workunits.

@gshuflin gshuflin changed the title Hook up to workunits Add option to attach Subsystems to streaming workunits Nov 26, 2019
@gshuflin gshuflin force-pushed the workunit_subsystem branch 2 times, most recently from 6897ef2 to b97278e Compare November 27, 2019 20:32
@gshuflin gshuflin marked this pull request as ready for review November 27, 2019 20:46
src/python/pants/init/extension_loader.py Outdated Show resolved Hide resolved
src/python/pants/option/global_options.py Outdated Show resolved Hide resolved
src/python/pants/subsystem/subsystem.py Outdated Show resolved Hide resolved
src/python/pants/subsystem/subsystem.py Outdated Show resolved Hide resolved
@gshuflin gshuflin force-pushed the workunit_subsystem branch 4 times, most recently from 809adde to 910a7ec Compare December 3, 2019 17:31
Comment on lines 508 to 509
"""For instance, `--streaming-workunits-handlers="['pants.reporting.workunit.Workunits']"` will """
"""register a Subsystem called Workunits defined in the module "pants.reporting.workunit"."""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: don't use """ unless you are doing a multiline string like this:

"""Line 1
Line 2
Line 3"""

What you are instead using is implicit string concatenation. I think you used """ because you need to use " in the string itself. Instead, what you can use is \".

src/python/pants/subsystem/subsystem.py Show resolved Hide resolved
src/python/pants/subsystem/subsystem.py Outdated Show resolved Hide resolved
src/python/pants/subsystem/subsystem.py Show resolved Hide resolved
@@ -63,8 +63,6 @@ def register_options(cls, register):
register('--zipkin-max-span-batch-size', advanced=True, type=int, default=100,
help='Spans in a Zipkin trace are sent to the Zipkin server in batches.'
'zipkin-max-span-batch-size sets the max size of one batch.')
register('--stream-workunits', advanced=True, type=bool, default=False,
help="If set to true, report workunit information while pants is running")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's necessary in this case, but just bringing it up in case it is: Should this option removal follow some kind of deprecation cycle?

continue

try:
callables.append(subsystem.handle_workunits)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there some way we can define an interface or mixin or something of the sort to capture all these constraints? (i.e. the component should have a method called handle_workunits, it should be a global subsystem...).

continue

try:
callables.append(subsystem.handle_workunits)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add checking for the signature of handle_workunits?

@blorente
Copy link
Contributor

blorente commented Dec 4, 2019

Also none of my comments are blockers :)

@gshuflin gshuflin merged commit 1dcae2f into pantsbuild:master Dec 4, 2019
@gshuflin gshuflin deleted the workunit_subsystem branch December 4, 2019 21:51
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

Successfully merging this pull request may close these issues.

4 participants