JuliaDynamics / ResumableFunctions.jl

C# style generators a.k.a. semi-coroutines for Julia.
Other
160 stars 19 forks source link

fix: scoping #100

Open thofma opened 3 months ago

thofma commented 3 months ago

What a ride.

It is still not finished, but all tests are passing for now. Actually, lots of tests themselves had scoping issues and should not have been valid before. Also some examples in the manual were actually wrong.

Todo

@Krastanov I poked at the problem using JuliaLowering (after I built the unique julia master version for which some combination of packages works). I don't think it is in a state to be used here. I already spent too much time thinking about scope, so I cooked up this solution here.

Closes #14, #32, #69, #70, #93

Krastanov commented 3 months ago

What a ride.

Wow, indeed! You are delving in a hacky mess that has not been touched probably by anyone except for Ben, the original author. If this is merged, it will certainly be a breaking release, but for the better.

@gerlero , as one of the main current users, a check from you will be very helpful.

The original bounty is for a hopefully much cleaner fix that can be made possible in the future, but you are right that the infrastructure library on which it would depend is probably not anywhere close to ready. But I should be able to figure out some form of award for this more restricted set of fixes. I can not commit anything explicit right now, please give me a week to go through some logistical questions on my end. FYI, I will not be very responsive for the next week as I am running a summer workshop (qnumerics.org).

codecov-commenter commented 3 months ago

Codecov Report

Attention: Patch coverage is 86.75799% with 29 lines in your changes missing coverage. Please review.

Project coverage is 80.14%. Comparing base (932dca3) to head (d4d39af).

Files with missing lines Patch % Lines
src/utils.jl 86.41% 25 Missing :warning:
src/macro.jl 85.71% 2 Missing :warning:
src/transforms.jl 90.47% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #100 +/- ## ========================================== + Coverage 79.78% 80.14% +0.36% ========================================== Files 6 6 Lines 455 680 +225 ========================================== + Hits 363 545 +182 - Misses 92 135 +43 ```

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

Krastanov commented 3 months ago

@thofma , I added you to the repository with triage access so that CI is not auto-blocked for you. Apologies for the spam.

thofma commented 3 months ago

Thanks.

I got the upstream tests working (will add some of the examples as tests here). But I cannot wrap my hand around the error for Fronts.jl. Even with ResumabelFunctions 0.6.9 I get:

bash> julia --proj=$(mktemp) -e 'using Pkg; Pkg.develop("Fronts"); Pkg.test("Fronts")'
[...]
Error During Test at /bla/dev/Fronts/test/test_boltzmann.jl:38
  Got exception outside of a @test
  StackOverflowError:
  Stacktrace:
       [1] _broadcast_getindex_evalf(::typeof(DifferentiationInterfaceForwardDiffExt.myvalue), ::Type, ::Int64)
         @ Base.Broadcast ./broadcast.jl:709
[...]

So the error happens even without the changes here?

Krastanov commented 3 months ago

Fronts.jl is by @gerlero (pinging in case you have time to look at this).

Their last CI is from two months ago, but it passes on julia 1.10 (and it fails on nightly but with an unrelated error)

thofma commented 3 months ago

The only errors in the "breakage" CI run are the ones that are already present without this PR, see https://github.com/JuliaDynamics/ResumableFunctions.jl/pull/101.

gerlero commented 3 months ago

Fronts.jl is by @gerlero (pinging in case you have time to look at this).

Fronts is currently broken for a different reason. So, test failures are unfortunately to be expected. I'll look into it as soon as I can.

gerlero commented 3 months ago

Reporting back: I made a hotfix for the latest and LTS failures. Fronts still fails with nightly, but it now should work with the current releases.

thofma commented 2 months ago

Tests are looking good, thanks for the quick fix.

thofma commented 2 months ago

@Krastanov Modulo cleaning up and documenting the approach, I am happy with the functionality. I linked the issues that are resolved with this in OP. But there are many other things that got fixed along the way (see the added tests).

Note that in my experience, just using JuliaLowering to rename the variables is only part of the solution. One then still has to resolve the challenges arising from "let", "for" and "named tuples".

thofma commented 2 months ago

@Krastanov Any update on this?

Krastanov commented 2 months ago

Apologies for the long wait on this. The start of the semester was a bit hectic, but it was unprofessional of me to not warn you about the delay.

It will take me a bit to go over the complete review, but this looks very promising. As we had already discussed, the original bounty had slightly different scope. The main difference that is left compared to the original scope is that JuliaLowering can provide more resilient and clean way to create the state machine (in particular, an implementation that is much more probable to NOT deviate from Julia semantics in some weird edge cases). But in comparison to all the heavy lifting you have already done, that is a small component. I would like to reserve 400$ of the original bounty for that final push to JuliaLowering (when JuliaLowering is ready), but after review I will send the rest to you, either together with the bounties already being processed for you, or as a separate transfer (depending on how quickly we process the first round of bounties).

This is slightly informal and potentially uncomfortable position, as the decision on what the award is, happens after most of the work has been done. I acknowledge that. Please do not hesitate to contest what I have described above.

thofma commented 2 months ago

Sounds good to me, but could you wait with the review till I have cleaned it up a bit? I will let you know once it is ready.

Krastanov commented 2 months ago

Whoops, just saw your other message. My bad! Anyway, my first round review was pretty superficial (I did not feel like I wasted time) and I probably pointed out things you already were planning to clean up.

thofma commented 1 week ago

I updated the PR, hopefully addressing all your comments @Krastanov. I also added a fix for a julia regression on >= 1.11 (https://github.com/JuliaLang/julia/issues/54664).

Let me know if there some anything else that needs to be done.

Krastanov commented 4 days ago

This looks great! I should be able to give a more thorough review towards the end of next week. In the meantime I will prod the NumFOCUS folks again on the bounty processing, as it seems the previous round has still not been sent out (should have been out last week).