timholy / ProgressMeter.jl

Progress meter for long-running computations
MIT License
694 stars 91 forks source link

add support for using @showprogress on Threads.@threads for loops #284

Closed Fro116 closed 7 months ago

Fro116 commented 10 months ago

This lets you use the @showprogress macro on parallel for loops. For example,

@showprogress Threads.@threads for i in 1:100
    sleep(0.1)
end
codecov[bot] commented 10 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (5b3bd1d) 96.81% compared to head (0ca12d5) 97.03%. Report is 4 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #284 +/- ## ========================================== + Coverage 96.81% 97.03% +0.22% ========================================== Files 1 1 Lines 533 539 +6 ========================================== + Hits 516 523 +7 + Misses 17 16 -1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Fro116 commented 10 months ago

My bad, I never tested on older versions of Julia. The Threads.@threads had a different expression tree before v1.3.0-rc1 and so the new test I added was failing on older builds.

I'll gate the Threads.@threads test behind a @static if VERSION >= v"1.3.0-rc1" statement, the same way we do for some Threads.@spawn tests in test_threads.jl.

Fro116 commented 10 months ago

Take three. I updated to fix the code coverage errors.

The last run failed on the nightly ubuntu-latest build because the test case "test_threads.jl ProgressUnknown" errored out. I'm not sure why. That test should not touch any of the code in this pr. I can't repro it, and it didn't fail in the first run (https://github.com/timholy/ProgressMeter.jl/actions/runs/6859117251/job/18660114093?pr=284). Is the test flaky on master?

MarcMush commented 10 months ago

Recently yes Ref #281

Fro116 commented 10 months ago

Okay, afaict this passes everything except for the flaky test. Should be good to go, but let me know if there's anything else I should look into.

MarcMush commented 8 months ago

LGTM