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

Release v0.12.167 breaks RecursiveFactorization on Julia v1.11+ #520

Closed
staticfloat opened this issue Jan 4, 2024 · 12 comments
Closed

Release v0.12.167 breaks RecursiveFactorization on Julia v1.11+ #520

staticfloat opened this issue Jan 4, 2024 · 12 comments

Comments

@staticfloat
Copy link

If you try to precompile RecursiveFactorization against LoopVectorization v0.12.167+, you will receive the following error:

ERROR: LoadError: UndefVarError: `register_size` not defined in `LoopVectorization`                                                      
Stacktrace:                                                         
  [1] getproperty                                                                                                                        
    @ Base ./Base.jl:42 [inlined]
  [2] pick_threshold()
    @ RecursiveFactorization ~/.julia/packages/RecursiveFactorization/cDP6H/src/lu.jl:82
  [3] lu!
    @ RecursiveFactorization ~/.julia/packages/RecursiveFactorization/cDP6H/src/lu.jl:89 [inlined]

X-ref: https://github.com/JuliaLinearAlgebra/RecursiveFactorization.jl/blob/987606ff6e15d8a4f9645481c7a540d252c674da/src/lu.jl#L82

@staticfloat
Copy link
Author

I argue that the disabling of Julia v1.11 should be done in a major release, to make it easier for others to set compat bounds due to changes like this.

I have opened a PR to General to yank the last two releases, to hopefully minimize the breakage: JuliaRegistries/General#98171

@chriselrod
Copy link
Member

chriselrod commented Jan 4, 2024

I already made this PR yesterday: JuliaLinearAlgebra/RecursiveFactorization.jl#89

I have opened a PR to General to yank the last two releases, to hopefully minimize the breakage:

I confess I didn't look into it too much, but was told LV was already broken on 1.11.
But as that breakage was related to using Expr(:new, ...), I guess it may only happen in certain situations; that code path won't always be hit.

@staticfloat
Copy link
Author

Fixing RecursiveFactorization is good, but this is a breaking change and should be done in a major version bump so that compat bounds are easier to set. As an example, if I have a compat bound that forces an older version of RecursiveFactorization, I can still get this newer version of LV quite easily. It would be better to release this as 0.13 or something, then go and change all older versions of RF to only be compatible with the 0.12 series.

@staticfloat
Copy link
Author

I confess I didn't look into it too much, but was told LV was already broken on 1.11.

Perhaps there is something about it that is broken, but I can say that as of right now, if you run Pkg.add("RecursiveFactorization") on v1.11+, it fails to precompile because of this.

@chriselrod
Copy link
Member

You can merge the yank.
I tried a minimal example of Expr(:new, ...) locally, and it worked fine, but some form of this is now invalid.

I don't know the details, but I also don't think we should really bother maintaining this library.

@maleadt
Copy link

maleadt commented Jan 6, 2024

I also don't think we should really bother maintaining this library

Maybe we should then set compat bounds to prevent installation of LoopVectorization.jl on 1.11 so that downstream users are inclined to get rid of the LV.jl dependency? This issue currently causes multiple crashes on PkgEval, making it much harder to figure out when things regress. The alternative is blacklisting packages that use LoopVectorization.jl, which we don't want either because it means reducing coverage of PkgEval.

@maleadt
Copy link

maleadt commented Jan 6, 2024

Alternatively, can we not keep all of LoopVectorization.jl but just nuke the @turbo macro? That should keep RecursiveFactorization etc functional, without triggering the bad code generation that is incompatible with 1.11.

@maleadt
Copy link

maleadt commented Jan 6, 2024

#523

@ranocha
Copy link
Member

ranocha commented Jan 7, 2024

I also don't think we should really bother maintaining this library

Maybe we should then set compat bounds to prevent installation of LoopVectorization.jl on 1.11 so that downstream users are inclined to get rid of the LV.jl dependency?

Is there an alternative that provides the same speedups as LoopVectorization.jl?

@chriselrod
Copy link
Member

Is there an alternative that provides the same speedups as LoopVectorization.jl?

No.

Alternatively, can we not keep all of LoopVectorization.jl but just nuke the @turbo macro? That should keep RecursiveFactorization etc functional, without triggering the bad code generation that is incompatible with 1.11.

Would be nice to avoid the lengthy compile times, as it is part of a long dependency chain that reduces the benefit of parallel precompilation, but that's fine.

@maleadt
Copy link

maleadt commented Jan 8, 2024

Is there an alternative that provides the same speedups as LoopVectorization.jl?

No.

Hopefully we can just keep LoopVectorization.jl with #524.

@maleadt
Copy link

maleadt commented Jan 10, 2024

The release has been yanked, so I guess this can be closed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants