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

Fix tcollect of ProgressLoggingFoldable #34

Merged

Conversation

andreas-theo
Copy link
Contributor

Hello,

I have been carrying this patch to Transducers in my dev-ed version of Transducers for months now, but never found the time to submit a PR. This PR fixes #10 and allows one to define transducers wrapped with withprogress to use progress loggers. This is very nice, as it allows one to use the built-in support for ProgressLogging.jl in Pluto and VS Code together with Transducers and Folds.

For example

using Logging: global_logger
using TerminalLoggers: TerminalLogger
using Transducers

global_logger(TerminalLogger())

julia> tcollect(data |> withprogress |> Map() do (y, x); y+x end)
Progress: 100%|██████████████████████████████████████████████████████████████████████████| Time: 0:00:00
9-element Vector{Int64}:
 3
 4
 5
 4
 5
 6
 5
 6
 7

* Calling collect on a Foldable that is wrapped with withprogress to
allow progress logging fails because ProgressLoggingFoldable is not
iterable.
* We add a method of split_into_chunks for when coll is a
ProgressLoggingFodlable that partitions the foldable enclosed withing
the ProgressLoggingFoldable struct, collects it, and then rewraps with
a call to withprogress to allow logging to continue.
* This allows the creation of transducers with progress logging and fixes JuliaFolds2/Transduces.jl#10
@andreas-theo andreas-theo changed the title Fix tcollect withprogress Fix tcollect of ProgressLoggingFoldable May 6, 2024
@MasonProtter
Copy link
Member

Hi @andreas-theo, thanks for submitting this. I was going to complain that this wasn't really a good solution because it'll only update the progress meter once one of the serial chunks finishes, but then I looked at the old implementation and found that it actually does the same thing. So I guess this isn't actually any worse than how it used to work.

Still not sure I really like this version, but I guess it's good enough for now, and I can later consider the best way to make it update more often in a future patch.

@MasonProtter MasonProtter merged commit 32961e9 into JuliaFolds2:master May 7, 2024
9 of 10 checks passed
@andreas-theo
Copy link
Contributor Author

andreas-theo commented May 7, 2024

Indeed, I agree. I'd be open to implementing a better solution with some guidance from your side.

@MasonProtter
Copy link
Member

Hi Andreas, sorry for the late reply. I'd love to help guide you with this, but life has been pretty hectic recently and I haven't had much time for Transducers.

I'm hoping that in a couple weeks I can write up some thoughts on how it can be handled.

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.

tcollect(withprogress(itr)) is currently broken
2 participants