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

put chainrules core in a requires block, rm it from deps #107

Merged
merged 2 commits into from
Jun 19, 2023

Conversation

vpuri3
Copy link
Contributor

@vpuri3 vpuri3 commented Jun 14, 2023

fix #106

@codecov
Copy link

codecov bot commented Jun 14, 2023

Codecov Report

Patch coverage: 33.33% and project coverage change: -0.77 ⚠️

Comparison is base (a25656d) 87.08% compared to head (8db7851) 86.32%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #107      +/-   ##
==========================================
- Coverage   87.08%   86.32%   -0.77%     
==========================================
  Files           3        3              
  Lines         209      212       +3     
==========================================
+ Hits          182      183       +1     
- Misses         27       29       +2     
Impacted Files Coverage Δ
src/AbstractFFTs.jl 50.00% <33.33%> (-50.00%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@stevengj
Copy link
Member

Why is coverage warning that we don't have test coverage for the requires block?

@vpuri3
Copy link
Contributor Author

vpuri3 commented Jun 18, 2023

added tests to check conditional loading of ChainRulesCore on ^1.9

@stevengj
Copy link
Member

Invalidations test is failing with

ERROR: LoadError: Failed to precompile SnoopCompile [aa65fe97-06da-5843-b5b1-d5d13cad87d2] to "/home/runner/.julia/compiled/v1.9/SnoopCompile/jl_fFICKt".

which seems like not the fault of this PR?

@vpuri3
Copy link
Contributor Author

vpuri3 commented Jun 18, 2023

correct, that can't be related to this PR. it looks like a compile error in TypedSyntax.jl, Cthulu.jl.

https://github.com/JuliaMath/AbstractFFTs.jl/actions/runs/5300882552/jobs/9594723411?pr=107#step:5:215

@stevengj stevengj merged commit 3a3f0e4 into JuliaMath:master Jun 19, 2023
@vpuri3 vpuri3 deleted the req branch June 19, 2023 13:58
@devmotion
Copy link
Member

I wonder if this PR was a good idea? ChainRulesCore is (supposed to be) a lightweight interface package (and I've even heard some people argue it should not be put into an extension on Julia >= 1.9 even though that's the common pattern now). Masking it with Requires as done in this PR will 1) break pre-compilation on Julia < 1.9 and 2) add an additional dependency on Julia >= 1.9.

@vpuri3
Copy link
Contributor Author

vpuri3 commented Jun 27, 2023

My reasoning for putting extension packages in a requires block is that loading CRC in here will prompt loading CRC extensions of every downstream package. As these extensions can be arbitrarily many and arbitrarily large, its worth adding a Requires dependency than paying the cost of all that code loading.

@vpuri3
Copy link
Contributor Author

vpuri3 commented Jun 27, 2023

break pre-compilation on Julia < 1.9

can you explain?

@devmotion
Copy link
Member

Requires breaks pre-compilation.

@devmotion
Copy link
Member

My reasoning for putting extension packages in a requires block is that loading CRC in here will prompt loading CRC extensions of every downstream package.

ChainRulesCore will be loaded in most projects with AbstractFFTs on Julia <= 1.8 anyway I assume since e.g. SpecialFunctions and many other core packages depend on ChainRulesCore.

stevengj pushed a commit that referenced this pull request Jun 27, 2023
@vpuri3
Copy link
Contributor Author

vpuri3 commented Jul 2, 2023

Requires breaks pre-compilation.

I'm not aware of this. Is there an issue or discourse on this topic?

@devmotion
Copy link
Member

I'm not aware of a "standard" reference (e.g., I just happened to learn about the deficiencies of Requires over the years and I assume many other people also "just know") but there a few open issues in Requires.jl that point to different problems. A few problems, including the issue with precompilation, are listed in JuliaLang/Pkg.jl#1285.

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.

rm ChainRulesCore dependency and include("chainrulesext") in a Requires.@require block
3 participants