Skip to content

Conversation

@hendrikmakait
Copy link
Member

@hendrikmakait hendrikmakait commented Nov 11, 2022

This PR adds a ForwardOutput worker plugin that forwards all output from stdout and stderr on all workers to all clients using the internals implemented for distributed.worker.print. This plugin is not intended for chatty workloads and may put significant load on the scheduler.

Note that this plugin is related to #7276 but takes a slightly different route as it tries to avoid adding additional client methods that start to bloat the client interface. This comes at the cost of forwarding to all clients. This is the same behavior that we already have with distributed.print(). Unless somebody is particularly interested in using this plugin in a multi-client environment, I would leave this as is until we have added client plugins that allow adding functionality on the client without the need for dedicated methods.

  • Tests added / passed
  • Passes pre-commit run --all-files

@hendrikmakait hendrikmakait marked this pull request as draft November 11, 2022 09:39
@github-actions
Copy link
Contributor

github-actions bot commented Nov 11, 2022

Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

       15 files  ±0         15 suites  ±0   6h 27m 34s ⏱️ - 4m 41s
  3 217 tests +1    3 131 ✔️ ±0    84 💤 ±0  2 +1 
23 789 runs  +8  22 878 ✔️ +7  909 💤 ±0  2 +1 

For more details on these failures, see this check.

Results for commit 3c84bbd. ± Comparison against base commit ffc418e.

♻️ This comment has been updated with latest results.

@hendrikmakait hendrikmakait self-assigned this Nov 11, 2022
@hendrikmakait hendrikmakait marked this pull request as ready for review November 11, 2022 10:47
@hendrikmakait
Copy link
Member Author

I just realized that this does not capture any logging.StreamHandler that has been initialized before the plugin was installed.

@hendrikmakait hendrikmakait marked this pull request as draft November 11, 2022 14:28
@hendrikmakait
Copy link
Member Author

I just realized that this does not capture any logging.StreamHandler that has been initialized before the plugin was installed.

Fixed by monkey-patching instead of replacing the stream objects.

@hendrikmakait hendrikmakait marked this pull request as ready for review November 11, 2022 16:05
@fjetter
Copy link
Member

fjetter commented Nov 15, 2022

FWIW I don't really care about the logging stream handlers. We can forward logging on different channels. If it works like this, I'm OK with it though

@fjetter fjetter merged commit ef13425 into dask:main Nov 15, 2022
@hendrikmakait
Copy link
Member Author

The issue I encountered with logging.StreamHandlers generalized to any object that stores a reference to objects sys.std{out|err} reference before we replace them, so this might occur in a few other places that follow the same pattern. It wasn't much effort to tweak since all the heavy-lifting of forwarding the logs had been implemented, it merely became a small exercise in monkey-patching instance methods in the right spot.

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.

Plugin to pipe stdout/stderr to a log event

2 participants