Add ability to customize worker prefix#157
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #157 +/- ##
==========================================
- Coverage 79.43% 79.10% -0.33%
==========================================
Files 10 10
Lines 1969 1977 +8
==========================================
Hits 1564 1564
- Misses 405 413 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
IanButterworth
changed the title
JamesWrigley
left a comment
JamesWrigley
left a comment
There was a problem hiding this comment.
Related PR: JuliaParallel/DistributedNext.jl#17
| end | ||
| ``` | ||
| """ | ||
| const worker_output_hook = Ref{Union{Nothing, Function}}(nothing) |
There was a problem hiding this comment.
I don't think I like that we expect the handler to fully take over the role of printing stuff as well. We may very well change it in the future to better support logging or something, so I would prefer that the callback instead be a transform that takes in the string and returns a (possibly different) string for Distributed to handle in its own way.
In which case we could have:
| const worker_output_hook = Ref{Union{Nothing, Function}}(nothing) | |
| worker_output_hook::Function = identity |
There was a problem hiding this comment.
Good call. I switched it to a transform that just returns the string, so Distributed still owns the println (and a throwing hook now falls back to the default prefix instead of eating output).
Two deliberate tweaks from the exact suggestion:
- (ident, line) not just line. The test harness needs ident to label a line with which test that worker is running.
- defaults to nothing, not identity. So unset means "use the normal From worker N: prefix" rather than printing raw un-prefixed lines. Default behavior stays unchanged.
|
Regarding the design in JuliaParallel/DistributedNext.jl#17, I'm not sure if we should support per-module callbacks. I could maybe see it being useful if a higher-level tool like Dagger wanted to customize the output a bit more (CC @jpsamaroo). |
|
I would also be ok with keeping this a strictly internal feature for the Julia tests 🤷 |
|
If we keep it internal and non-public, then we have the added benefit of being able to make breaking changes to it (or even remove it entirely) in the future. |
`redirect_worker_output` is documented to receive `ident` as an `AbstractString`, and the `cluster.jl` call site already passes `"$pid"`. The `connect` path in `managers.jl` passed the raw `Int` pid instead, so any consumer that treated `ident` as a string (e.g. `tryparse(Int, ident)`) would misbehave. Pass `"$pid"` here too so the two call sites agree. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add an internal, unstable `worker_output_hook` Ref that, when set, transforms each line of a worker's output into the string that should be printed for it. `Distributed` still owns the actual printing, so a hook may return a `StyledStrings`-styled line and it will render correctly on IOs that do or do not support color. When the hook is unset (the default) the standard prefix is used. A hook that throws falls back to the default prefix so worker output is never swallowed. The default prefix indent is also reduced from six spaces to a single space. The hook is deliberately not exported and carries no compatibility guarantees; it exists so test harnesses can label worker output with richer context (e.g. which test a worker is running). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
48fc61d to
14f8129
Compare
Label each line of a test worker's output with the test it is currently running instead of just its worker id, making it much easier to tell which suite a stray log line or precompile message came from. Worker output is routed through Distributed, so an internal `Distributed.worker_output_hook` is installed that transforms each line into a de-emphasized `"<test> (<worker>): "` prefix (falling back to the worker id when the worker is between tests). The node-1 tests run in-process and do not pass through that redirection, so their stdout/stderr are captured with a `Pipe` and prefixed the same way via `with_output_prefix`. Prefixes are styled with `StyledStrings` so they are only colorized on IOs that support it. The embedding test is updated for the reduced single-space default worker prefix introduced in the corresponding Distributed change. Depends on JuliaLang/Distributed.jl#157. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Point the bundled Distributed at JuliaLang/Distributed.jl#157, which adds the internal `worker_output_hook` used by the test harness. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
3 participants
For JuliaLang/julia#60372