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

Remove all the rule overload generation stuff #356

Merged
merged 3 commits into from
Jun 1, 2021
Merged

Remove all the rule overload generation stuff #356

merged 3 commits into from
Jun 1, 2021

Conversation

oxinabox
Copy link
Member

@oxinabox oxinabox commented May 26, 2021

We are moving all this stuff into its own package
Requires JuliaDiff/ChainRulesOverloadGeneration.jl#1 to be merged and a relase tagged first so that the docs don't hit a page in those docs that doesn't exist.

This will close #340

It will break invenia/Nabla.jl#189
I don't think it will break any code in the wild.
So we could perhaps do this as a nonbreaking release.
Otherwise: we can merge this once we are ready to tag a breaking release.
Probably in the interests of responsibility we should do that.

I don't think this is worth deprecating, because working out how to do that without a circular dependency seems hard.

Waiting for JuliaRegistries/General#37582

@codecov-commenter
Copy link

codecov-commenter commented May 26, 2021

Codecov Report

Merging #356 (b70ead7) into master (7510c4f) will decrease coverage by 0.88%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #356      +/-   ##
==========================================
- Coverage   86.71%   85.82%   -0.89%     
==========================================
  Files          14       13       -1     
  Lines         542      494      -48     
==========================================
- Hits          470      424      -46     
+ Misses         72       70       -2     
Impacted Files Coverage Δ
src/ChainRulesCore.jl 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7510c4f...b70ead7. Read the comment docs.

@oxinabox
Copy link
Member Author

This doesn't seem to have much decrease on load time for me.
But also mu load times are for some reason much much higher than @simeonschaub reported in #341

Current Master

(ChainRulesCore) pkg> precompile

julia> @time using ChainRulesCore
  0.833778 seconds (1.45 M allocations: 82.013 MiB, 1.14% gc time)

This PR

Precompiling project...
  1 dependency successfully precompiled in 2 seconds (4 already precompiled)

julia> @time using ChainRulesCore
  0.796725 seconds (1.33 M allocations: 74.891 MiB, 2.00% gc time)

my system


julia> versioninfo()
Julia Version 1.6.2-pre.0
Commit dd122918ce* (2021-04-23 21:21 UTC)
Platform Info:
  OS: macOS (x86_64-apple-darwin20.3.0)
  CPU: Intel(R) Core(TM) i7-8559U CPU @ 2.70GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-11.0.1 (ORCJIT, skylake)

docs/src/use_in_ad_system.md Outdated Show resolved Hide resolved
@simeonschaub
Copy link
Member

Sorry, I just got my new laptop and was having some issues with my setup, so it took me a bit longer to respond.

ChainRulesCore#master:

julia> @time using ChainRulesCore
  0.129288 seconds (183.98 k allocations: 11.294 MiB, 7.01% compilation time)

ChainRulesCore#ox/nogen:

julia> @time using ChainRulesCore
  0.033548 seconds (50.97 k allocations: 3.213 MiB, 11.55% compilation time)

🎉

julia> versioninfo()
Julia Version 1.7.0-DEV.1170
Commit d0a6b3a814 (2021-05-26 19:20 UTC)
Platform Info:
  OS: Linux (x86_64-pc-linux-gnu)
  CPU: AMD Ryzen 7 PRO 4750U with Radeon Graphics
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-12.0.0 (ORCJIT, znver2)

@oxinabox
Copy link
Member Author

Cool, I will have to workout why things are so slow for me, but that is a question for another day

@simeonschaub
Copy link
Member

simeonschaub commented May 27, 2021

I think @IanButterworth did some experiments a while back and figured out that sometimes I/O can be unusually slow on Darwin. Could definitely be wrong about this though.

Copy link
Member

@simeonschaub simeonschaub left a comment

Choose a reason for hiding this comment

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

LGTM!

@IanButterworth
Copy link

some experiments a while back and figured out that sometimes I/O can be unusually slow on Darwin.

I don't know if it explains all of the slowness on MacOS generally, but yeah, particularly JuliaLang/julia#35907 (comment) which MacOS devs confirmed was to be expected because MacOS does more things during IO

@mzgubic
Copy link
Member

mzgubic commented May 28, 2021

Are we ready to merge this with #358? And then tag a breaking release?

@oxinabox
Copy link
Member Author

oxinabox commented May 28, 2021

Are we ready to merge this with #358? And then tag a breaking release?

We can't merge this til JuliaRegistries/General#37582 or there will be a broken link in docs.
we also should check all out downstreams are upgraded to not use deprecations so the update can be smooth.
So lets do this on Monday.

@oxinabox oxinabox merged commit 2c69796 into master Jun 1, 2021
@oxinabox oxinabox deleted the ox/nogen branch June 1, 2021 11:27
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.

Avoid __init__ to reduce load time
6 participants