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

Use SnoopCompileBot #2832

Merged
merged 20 commits into from
Jul 1, 2020
Merged

Use SnoopCompileBot #2832

merged 20 commits into from
Jul 1, 2020

Conversation

daschw
Copy link
Member

@daschw daschw commented Jun 30, 2020

This should automatically update the precompile files using SnoopCompileBot and a corresponding Github Action as described in https://timholy.github.io/SnoopCompile.jl/stable/bot/
This removes the current precompile file and hopefully PRs with new precompile files are opened after this gets merged.
@timholy or @aminya it would be great if you could take a look at this and check if I'm doing something wrong here.

@daschw
Copy link
Member Author

daschw commented Jun 30, 2020

The SnoopCompile runs can be checked at https://github.com/daschw/Plots.jl/actions/runs/153066245 and here you can take a look at the PR that would be created: daschw#4

@daschw
Copy link
Member Author

daschw commented Jun 30, 2020

I guess we should decide if we want to have different precompile files per OS (does this make sense?). Otherwise we don't have to run the action on all OSs. And, since nightly is already 1.6 I should add a run for some Julia 1.5 version

@timholy
Copy link
Contributor

timholy commented Jun 30, 2020

LGTM but when it comes to the bot, @aminya is the real expert.

@timholy
Copy link
Contributor

timholy commented Jun 30, 2020

Also worth saying, I think you guys will see some gains wrt to load times within another month or so on nightly...there are still several crucial PRs to merge, the most important being JuliaLang/julia#35877 (which is currently on hold for good reasons). Outside of Julia & Pkg, only JuliaWeb/HTTP.jl#549 is waiting to be merged (as long as you're using at least FixedPointNumbers 0.8.1, ColorTypes 0.10.4). For me the combination of all of those drops TTFP by almost an additional factor of 1.5-2 compared to 1.5rc1.

EDIT: these changes should also make the precompile files more effective, which is why I brought it up here. Currently you can "ask" for something to be precompiled but it doesn't always "take," if any MethodInstances it depends on got invalidated. So stomping out invalidations reduces the number of failures to precompile. There are other reasons to fail but this is the biggest one in practice.

@daschw
Copy link
Member Author

daschw commented Jun 30, 2020

Sounds great and huge thanks to everyone who's involved in this!

@daschw
Copy link
Member Author

daschw commented Jun 30, 2020

Looking at https://github.com/daschw/Plots.jl/pull/4/files now it seems like that the precompile files are identical for each OS, so probably we do not require multi-os.

daschw and others added 2 commits June 30, 2020 23:22
Co-authored-by: Amin Yahyaabadi <aminyahyaabadi74@gmail.com>
Co-authored-by: Amin Yahyaabadi <aminyahyaabadi74@gmail.com>
version: # NOTE: the versions below should match those in your botconfig
- '1.3'
- '1.4'
# - '1.5'
Copy link
Contributor

@aminya aminya Jun 30, 2020

Choose a reason for hiding this comment

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

Enabling this since nightly is 1.6 now? 🤔

Suggested change
# - '1.5'
- '1.5.0-rc1'

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

All 1.5 tests are failing there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe try -'1.5.0-rc1'.

@SaschaMann shouldn't 1.5 install '1.5.0-rc1' by default? julia-setup fails to do this.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, let's try

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

With 1.5.0-0 I get


Run julia-actions/setup-julia@latest
0s
    PLOTS_TEST: true
Run julia-actions/setup-julia@latest
  with:
    version: 1.5.0-0
    arch: x64
    show-versioninfo: false
  env:
    GKS_ENCODING: utf8
    GKSwstype: 100
    PLOTS_TEST: true
##[error]Unexpected HTTP response: 404

https://github.com/daschw/Plots.jl/runs/824362165

Choose a reason for hiding this comment

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

My bad, it should've been ^1.5.0-0. Sorry! 🤦

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

But that's an issue with SnoopCompile.jl cc: @aminya

Co-authored-by: Amin Yahyaabadi <aminyahyaabadi74@gmail.com>
@daschw
Copy link
Member Author

daschw commented Jun 30, 2020

@aminya I guess, once we're sure this works I should uncomment

    #  - 'master'  # NOTE: to run the bot only on pushes to master

in SnoopCompile.yml?

@aminya
Copy link
Contributor

aminya commented Jun 30, 2020

@aminya I guess, once we're sure this works I should uncomment

    #  - 'master'  # NOTE: to run the bot only on pushes to master

in SnoopCompile.yml?

If you want to get precompile updates on pushes to other branches (under JuliaPlots/Plots or when users fork this repository) don't uncomment this. But if you want precompile updates only on pushes to master, then do that.

I don't think this be an issue but may happen in rare cases: Once this PR is merged and bot PR is merged as well, it also may worth mentioning in the contribution section of readme or docs to set should_precompile=false in case some error happens because of precompilation. This is very rare, but it is good to know.

@daschw
Copy link
Member Author

daschw commented Jun 30, 2020

it also may worth mentioning in the contribution section of readme or docs to set should_precompile=false

Where exactly do we set that?

@aminya
Copy link
Contributor

aminya commented Jun 30, 2020

it also may worth mentioning in the contribution section of readme or docs to set should_precompile=false

Where exactly do we set that?

https://github.com/daschw/Plots.jl/pull/4/files#diff-2270ce88ed8290c3b808f5ea76728946R1
Again, this is very rare, and for Plots, this will not be a thing as it has used hardcoded precompile signatures for a long time.

@daschw
Copy link
Member Author

daschw commented Jul 1, 2020

OK, this seems to work now. Thanks a lot @aminya @timholy and @SaschaMann for your support!

@daschw daschw merged commit 065089a into JuliaPlots:master Jul 1, 2020
@daschw daschw deleted the snoopibot branch October 12, 2020 09:28
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.

4 participants