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

Re-generate precompile statemenst before 1.0 release #2642

Closed
bkamins opened this issue Mar 5, 2021 · 10 comments · Fixed by #2718
Closed

Re-generate precompile statemenst before 1.0 release #2642

bkamins opened this issue Mar 5, 2021 · 10 comments · Fixed by #2718
Labels
CI non-breaking The proposed change is not breaking priority
Milestone

Comments

@bkamins
Copy link
Member

bkamins commented Mar 5, 2021

Following JuliaLang/julia#38136 change.

@bkamins bkamins added CI non-breaking The proposed change is not breaking priority labels Mar 5, 2021
@bkamins bkamins added this to the 1.0 milestone Mar 5, 2021
@bkamins bkamins mentioned this issue Mar 5, 2021
19 tasks
@bkamins
Copy link
Member Author

bkamins commented Mar 5, 2021

The problem is caused by precompilation:

   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.7.0-DEV.659 (2021-03-05)
 _/ |\__'_|_|_|\__'_|  |  Commit e49567fa11 (0 days old master)
|__/                   |

julia> using DataFrames

julia> DataFrames.precompile(true)
ERROR: Wrapping `Vararg` directly in UnionAll is deprecated (wrap the tuple instead).
Stacktrace:
 [1] UnionAll(v::TypeVar, t::Any)
   @ Core ./boot.jl:254
 [2] precompile(all::Bool)
   @ DataFrames ~/.julia/packages/DataFrames/oQ5c7/src/other/precompile.jl:168
 [3] top-level scope
   @ REPL[2]:1

@timholy (is https://github.com/timholy/SnoopCompile.jl already fixed not to generate such entries?)

CC @nalimilan

@nalimilan
Copy link
Member

Let's just remove the offending line for now. Then when we update the precompilation directives before 1.0 we can check what to do.

@bkamins
Copy link
Member Author

bkamins commented Mar 5, 2021

For now it is not a super big problem - it is only a warning on nightly so I assume we will release DataFrames.jl 1.0 earlier. There are very many offending lines but only with precompile(true).

@timholy
Copy link
Contributor

timholy commented Mar 5, 2021

I think SnoopCompile already handles this (not 100% sure though). But your precompile file, once generated, is decoupled from SnoopCompile. Your file also looks hand-edited (which is great, I usually do that too), but that means you get the valuable prize 🎁 of the privilege of maintaining it, too! 😉

My current approach is often just to run code at build time that forces compilation, without explicit precompile directives. But you do need to make sure that they don't get interpreted. Currently a single loop suffices to force compilation, but of course that's subject to change. See https://github.com/JuliaImages/ImageCore.jl/blob/62b56174b6b2b51e960115e82ee12326f0e0df30/src/precompile.jl for an example.

@nalimilan
Copy link
Member

Actually we took the simple and lazy approach of generating precompilation directives based on what running the full test suite (with a threshold of course!), so we can't really run it on each user's machine. :-) But the manual editing is minimal, it's essentially a copy/paste.

@bkamins
Copy link
Member Author

bkamins commented Mar 5, 2021

Yes - the manual editing is mainly because test suite uses packages that are not dependencies of the package (to check that the generic integration API works correctly).

@timholy
Copy link
Contributor

timholy commented Mar 5, 2021

Oh, I'm a big fan of curating the output from SnoopCompile. I'd go so far as to say these days all I do is look at the list of precompiles and use it as inspiration to write the precompile file by hand. You can often replace a really weird method with a more normal one as long as there's a chain of good inference connecting them. That's the main reason that in modern versions of SnoopCompile I'm advocating eating your vegetables before dessert: use it first to improve inferrability and only after that add precompiles. I'm not sure it would would happen for a package of DataFrame's size and complexity, but often I find that the precompiles reduce down to a much smaller set once the inference issues are resolved.

That said, I looked at inference in DataFrames with a very early draft of the modern SnoopCompile. With a couple of exceptions that mostly seemed to have been well-considered, I didn't find a lot to worry about from the standpoint of inferrability. There are some pretty dramatic gains to be had by preventing constant-propagation on your boolean-valued kwargs, but I'm a little uneasy about proposing that this be something you change in this package; instead I kind of think this is something that Julia itself should take care of. JuliaLang/julia#38983 and especially JuliaLang/julia#38983 (comment)

@bkamins
Copy link
Member Author

bkamins commented Mar 5, 2021

I have to tackle the following issues #2597, #2566, and #2516 to improve the internal code.

After I am done with this (hopefully I will be able to do it myself) then, if you allow, I would consult you to get approval 😄.

Then I assume it would be a good moment to write precompile file.

But before doing these things we need to finalize the scope of functionality we add for 1.0 release (so that we are sure that we are optimizing compile time for final design). All this should happen soon (as my personal hard deadline is 1.0 release of DataFrames.jl before JuliaCon 2021). Thank you for being so supportive and responsive.

@timholy
Copy link
Contributor

timholy commented Mar 5, 2021

That sounds like the sensible path. Latency matters but functionality is more pressing. And it's very unlikely that latency considerations will change whether you can or cannot implement some bit of functionality.

@bkamins bkamins changed the title Fix Vararg usage Re-generate precompile statemenst before 1.0 release Apr 12, 2021
@bkamins
Copy link
Member Author

bkamins commented Apr 12, 2021

This should be done after #2714

@bkamins bkamins linked a pull request Apr 13, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI non-breaking The proposed change is not breaking priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants