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

Move SparseArrays support to an extension #638

Merged
merged 1 commit into from
Oct 17, 2023
Merged

Conversation

devmotion
Copy link
Member

This PR moves support for SparseArrays to an extension.

In Julia >= 1.10, SparseArrays won't be included in the system image anymore. Therefore, to reduce loading times SparseArrays was or is about to be demoted from a regular to a weak dependency for many packages in the ecosystem such as FillArrays, Distances and PDMats. However, if a package in the dependency stack loads SparseArrays all these extensions will be compiled and loaded.

Ideally, if possible SparseArrays support in ChainRules should also be moved to an extension. I haven't checked yet if there are any obstacles (e.g., whether SparseArrays is also used for rules of functions not defined in SparseArrays itself). Since, however, mainly AD backends depend on ChainRules it might be a bit less important though.

@codecov
Copy link

codecov bot commented Oct 16, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (efc2f86) 93.90% compared to head (518b07a) 93.90%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #638   +/-   ##
=======================================
  Coverage   93.90%   93.90%           
=======================================
  Files          14       15    +1     
  Lines         903      903           
=======================================
  Hits          848      848           
  Misses         55       55           
Files Coverage Δ
src/ChainRulesCore.jl 100.00% <ø> (ø)
src/accumulation.jl 97.05% <ø> (-0.17%) ⬇️
src/projection.jl 97.26% <ø> (+0.21%) ⬆️
ext/ChainRulesCoreSparseArraysExt.jl 96.29% <96.29%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@devmotion
Copy link
Member Author

devmotion commented Oct 16, 2023

The PDMatsExtras test errors are unrelated (and fixed by invenia/PDMatsExtras.jl#35).

@mcabbott
Copy link
Member

This seems fine. Are you able to time any improvements in loading time? Not sure whether SparseArrays is already out of system image, on master I see this:

julia> @time_imports using ChainRulesCore
      0.5 ms  Compat
      0.2 ms  Compat  CompatLinearAlgebraExt
     16.0 ms  ChainRulesCore

@devmotion
Copy link
Member Author

devmotion commented Oct 16, 2023

Not sure whether SparseArrays is already out of system image

This is an upcoming change in Julia 1.10: https://github.com/JuliaLang/julia/blob/master/HISTORY.md#build-system-changes

I assume @time_imports doesn't show loading time of stdlibs and/or packages in the system image since LinearAlgebra is another dependency that does not show up in your output.

@devmotion
Copy link
Member Author

devmotion commented Oct 16, 2023

With Julia 1.10.0-beta3 I see (no startup file, after precompilation):

main

julia> @time_imports using ChainRulesCore
               ┌ 6.0 ms SuiteSparse_jll.__init__()
     23.4 ms  SuiteSparse_jll 69.28% compilation time
               ┌ 4.0 ms SparseArrays.CHOLMOD.__init__() 85.89% compilation time
     90.1 ms  SparseArrays 3.82% compilation time
      0.3 ms  Compat
      0.2 ms  Compat  CompatLinearAlgebraExt
      8.0 ms  ChainRulesCore

This PR

julia> @time_imports using ChainRulesCore
      0.9 ms  Compat
      0.5 ms  Compat  CompatLinearAlgebraExt
     14.8 ms  ChainRulesCore

That confirms my general impression from other packages: SparseArrays significantly increases loading time.

Copy link
Member

@oxinabox oxinabox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems good to me.
Bump the minor version and tag a release once you merge

@devmotion
Copy link
Member Author

Bump the minor version

Already done 🙂 I'll merge and release.

@devmotion devmotion merged commit 26b7748 into main Oct 17, 2023
22 of 27 checks passed
@devmotion devmotion deleted the dw/sparsearrays branch October 17, 2023 08:54
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

Successfully merging this pull request may close these issues.

3 participants