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

Broken on Julia 1.10 (fix included) #60

Closed
maleadt opened this issue Apr 24, 2023 · 17 comments
Closed

Broken on Julia 1.10 (fix included) #60

maleadt opened this issue Apr 24, 2023 · 17 comments

Comments

@maleadt
Copy link

maleadt commented Apr 24, 2023

This package's inspection of method slots is broken on Julia 1.10 (after JuliaLang/julia#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
Copy link
Author

maleadt commented Apr 25, 2023

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.

Krastanov added a commit to QuantumSavory/Semicoroutines.jl that referenced this issue Apr 25, 2023
@detrin
Copy link

detrin commented Apr 25, 2023

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
Copy link
Author

maleadt commented Apr 26, 2023

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
Copy link

detrin commented Apr 28, 2023

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
Copy link
Member

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): JuliaRegistries/General#82530 (comment)

A similar fork is being prepared for SimJulia

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

@PallHaraldsson
Copy link

PallHaraldsson commented May 2, 2023

@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 JuliaRegistries/General#82530 (comment)

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
Copy link

marcom commented May 4, 2023

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
Copy link
Collaborator

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
Copy link

PallHaraldsson commented May 4, 2023

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
Copy link
Member

Krastanov commented May 4, 2023

@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
Copy link
Member

Krastanov commented May 5, 2023

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: marcom/PkgEval.jl@b32b1eb

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
Copy link
Collaborator

@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
Copy link
Member

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.

This was referenced May 9, 2023
Krastanov added a commit to Krastanov/PkgEval.jl that referenced this issue May 9, 2023
As reported in JuliaDynamics/ResumableFunctions.jl#60 ResumableFunctions.jl was relying on internal julia implementation details that changed in 1.10. A fix is now upstreamed and released as non-breaking (v0.6.2).
@Krastanov
Copy link
Member

ResumableFunctions.jl will shortly be removed from the PkgEval blacklist JuliaCI/PkgEval.jl#225

maleadt pushed a commit to JuliaCI/PkgEval.jl that referenced this issue May 10, 2023
As reported in JuliaDynamics/ResumableFunctions.jl#60 ResumableFunctions.jl was relying on internal julia implementation details that changed in 1.10. A fix is now upstreamed and released as non-breaking (v0.6.2).
@detrin
Copy link

detrin commented May 10, 2023

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
Copy link
Member

@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
Copy link

detrin commented May 10, 2023

@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.

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

No branches or pull requests

6 participants