JuliaDynamics / ResumableFunctions.jl

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

Broken on Julia 1.10 (fix included) #60

Closed maleadt closed 1 year ago

maleadt commented 1 year ago

This package's inspection of method slots is broken on Julia 1.10 (after https://github.com/JuliaLang/julia/pull/49113). Specifically, local slots are now not part of the code info anymore, so optimization would need to be disabled for this to work:

diff --git a/src/utils.jl b/src/utils.jl
index 9815847..486dbc4 100755
--- a/src/utils.jl
+++ b/src/utils.jl
@@ -40,7 +40,7 @@ function get_slots(func_def::Dict, args::Dict{Symbol, Any}, mod::Module) :: Dict
   func_def[:body] = postwalk(x->transform_nosave(x, nosaves), func_def[:body])
   func_expr = combinedef(func_def) |> flatten
   @eval(mod, @noinline $func_expr)
-  codeinfos = @eval(mod, code_typed($(func_def[:name])))
+  codeinfos = @eval(mod, code_typed($(func_def[:name]), Tuple; optimize=false))
   for codeinfo in codeinfos
     for (name, type) in collect(zip(codeinfo.first.slotnames, codeinfo.first.slottypes))
       name ∉ nosaves && (slots[name] = type)

It would be really great if this could be applied and put in a minor release, because currently packages that depend on ResumableFunctions.jl fail on 1.10 and thus cannot be tested by PkgEval anymore.

maleadt commented 1 year ago

Awaiting that, I'm going to disable testing of ResumableFunctions.jl and its dependents. That's not great, but it's also great to have recurring crashes on the PkgEval reports for a bug that has been triaged already (decreasing the SNR of those reports).

cc'ing some relevant people, whose packages depend on ResumableFunctions.jl: @mileslucas @marcom @gerlero @detrin @Krastanov @mohamed82008 @RohitRathore1

Feel free to ping me when ResumableFunctions.jl is fixed, or when a dependent package doesn't depend on ResumableFunctions.jl anymore.

detrin commented 1 year ago

Thanks for letting me know. Will you support this in future or does it mean that ResumableFunctions.jl is done for 1.10 onward?

maleadt commented 1 year ago

Will you support this in future or does it mean that ResumableFunctions.jl is done for 1.10 onward?

That is not up to me, but to the ResumableFunctions.jl maintainers. The fix I posted above should get the package working again on (the current version of) Julia 1.10.

detrin commented 1 year ago

Will you support this in future or does it mean that ResumableFunctions.jl is done for 1.10 onward?

That is not up to me, but to the ResumableFunctions.jl maintainers. The fix I posted above should get the package working again on (the current version of) Julia 1.10.

Right, I missed that you are not the author.

Krastanov commented 1 year ago

I forked the repository into a new Semicoroutines.jl https://github.com/QuantumSavory/Semicoroutines.jl

This fix is made, updated CI tests are now running, and I will be going through the orphaned pull requests here to make sure any abandoned improvements can be merged in.

As it is an important part of another project of mine, I plan to provide support at least for the next few years. I am certainly interested in having help from other contributors (and maybe merging things back here if ResumableFunctions.jl is revided).

The fork will be registered in 3 days at which point it would be a drop-in replacement for ResumableFunctions.jl (with a simple update to your Project.toml file and renaming your import statements): https://github.com/JuliaRegistries/General/pull/82530#issuecomment-1528010268

A similar fork is being prepared for SimJulia

@mileslucas @marcom @gerlero @detrin @mohamed82008 @RohitRathore1

PallHaraldsson commented 1 year ago

@BenLauwens For those reading this (open) issue. I blocked the registration of the fork, and then unblocked it since I was getting in the way, and the fork just now got merged into the registry. See discussion there in full, and https://github.com/JuliaRegistries/General/pull/82530#issuecomment-1531193618

So the issue is fixed, in the fork, unsure if the issue should be closed here... I think you may want to point to the fork in the README here. OR could someone take over your package (if done quickly enough then unregistering the fork is maybe allowed as an exception?)? I think you were just missing in action, and maybe not against the fork. It seems bad to me to have two packages, one abandoned, or worse if it then get developed further.

marcom commented 1 year ago

First up I'd like to thank @BenLauwens for creating this package and keeping it going all these years, it provided something I have always felt should be in the base language itself. I'd also like to thank @Krastanov for offering to maintain Semicoroutines.jl in the future, I have now also switched over to it so as to be able to keep working with julia-master.

BenLauwens commented 1 year ago

I regret sincerely this fork! I have proposed in a private conversation to discuss how we can solve the maintenance of the package without a hostile takeover but apparently there is no interest.

PallHaraldsson commented 1 year ago

Is it too late to abort the fork (since still rather new), and merge into this package (I did block the registration, then thinking this package was abandoned, and I getting in way of progress, unblocked). Did you plan to develop your package further; in incompatible way with the new one? I'm undecided on why name is better, I find most regrettable that a new package was needed, even if just perceived, at least if for non-technical reasons.

Krastanov commented 1 year ago

@BenLauwens , I do not think it is fair to call this a "hostile" action. All over the docs, readmes, and descriptions, we are linking to the original, and if you look at the pull requests we have on the fork, we are specifically trying to make them easy to backport if the original gets back into a publicly maintained state. The very post above in which I am saying I am making a fork says that I would be eager to contribute fixes back to the original library.

I have also sent you an email (about 10 days ago) in which I asked whether you are interested in having people volunteer as maintainers. I did not get an answer to that email. Hector also send you an email in which I was cc-ed, and in that email-thread I also said that I want it to be easy to get back to the original libraries when they get back to publicly maintained status.

The software you have created is wonderfully useful, which is why I have some of my projects now depend on it. However, you have not been responsive to issues and bugs that I have, so it seems pretty natural for me to make a fork. That way my projects can have functional dependencies.

If ResumableFunctions.jl and SimJulia.jl start seeing public development, I would be eager to abandon the forks I made and contribute any changes back to the original repositories.

In case that email fell victim to a spam filter, here is a copy of it:

Hi, Ben,

I am a fan of the SimJulia ecosystem you have created and use it for a couple of projects. For the last couple of months I have been thinking about making a friendly fork of it and to merge a few of the PRs that have accumulated over the last couple of years. I wanted to check if you have any opinion on this. If you prefer, I would be happy to provide this type of maintenance work directly to your original projects, but I am not quite sure what your preference would be. Let me know if you have any thoughts on this.

Best, Stefan

I stand by it. If you prefer maintenance to happen directly on this repo, I would be happy to help with that.

Krastanov commented 1 year ago

For reference (copying from the another thread): Ben has now merged the fix. When a new version with the fix gets registered, we should also undo the PkgEval commit that blacklisted all ResumableFunctions dependents: https://github.com/marcom/PkgEval.jl/commit/b32b1ebe58f9a833fe0c1d37918d9f71f1891374

I probably will have some time for that next week, if no one else gets around it (but we do need the fixed version to be registered (as non-breaking) before then)

Edit: wrong link, here is the correct PkgEval repo https://github.com/JuliaCI/PkgEval.jl

BenLauwens commented 1 year ago

@Krastanov you are added as collaborator. I am quite busy the next weeks, so feel free to register the fixed version and undo the PkgEval commit.

Thanks for your commitment.

Krastanov commented 1 year ago

Thanks! Happy to help! For other folks reading this conversation:

  1. I and a couple other volunteers will try to do some day-to-day support for this package with Ben's blessing. No big changes planned.
  2. The minor CI/Testing/Linting/Static Analysis improvements and polish from the fork will be backported today or over the weekend
  3. The Semicoroutines fork will be kept in sync in the near term as there are already a few projects that depend on it. I a few months or a year if/when ResumableFunctions proves healthy, I am committing to personally send pull requests to any dependents that update their Project.toml to depend on the healthy library.
  4. In a couple of days we will take care of the PkgEval blacklist too.
Krastanov commented 1 year ago

ResumableFunctions.jl will shortly be removed from the PkgEval blacklist https://github.com/JuliaCI/PkgEval.jl/pull/225

detrin commented 1 year ago

Thanks! Happy to help! For other folks reading this conversation:

  1. I and a couple other volunteers will try to do some day-to-day support for this package with Ben's blessing. No big changes planned.
  2. The minor CI/Testing/Linting/Static Analysis improvements and polish from the fork will be backported today or over the weekend
  3. The Semicoroutines fork will be kept in sync in the near term as there are already a few projects that depend on it. I a few months or a year if/when ResumableFunctions proves healthy, I am committing to personally send pull requests to any dependents that update their Project.toml to depend on the healthy library.
  4. In a couple of days we will take care of the PkgEval blacklist too.

Thanks for doing this work. I will switch to your package if you plan on maintaining it.

Krastanov commented 1 year ago

@detrin, no need to switch anymore, this package is mostly revived and we are on track to move it to a github organization so that it has better chances for long term support

detrin commented 1 year ago

@detrin, no need to switch anymore, this package is mostly revived and we are on track to move it to a github organization so that it has better chances for long term support

Alright, thanks I understand.