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

Precompilation: Prettier printing, opt-out auto #2091

Merged

Conversation

IanButterworth
Copy link
Member

@IanButterworth IanButterworth commented Oct 7, 2020

An attempt to make the firehose more manageable https://www.dropbox.com/s/r3x7p3dk46n0o5b/Julia%20Precomp%20-%20Prettier%20printing.mov?dl=0

Some things to note:

  1. The code could definitely be tidier. This is more of a UX demo.

  2. No throwing. This doesn't throw errors, not even for direct deps (unlike master), but it shows when errors occur. It's proven really challenging to make the errors and stack traces throw cleanly from within the parallel process, and I thought that perhaps the pkg repl isn't the best place to show the full stack traces anyway. So the failing deps are just highlighted, and the user can go and load the code to see the errors in full.

  3. Erroring packages are suspended during auto-precompilation. (This isn't new to this PR, but the video illustrates this nicely). I threw an error into ImageCore. Note how the errors that happen in the first precompile mean that those failing packages are skipped in subsequent auto precomp sessions. Then to force full precompilation, precompile is called.

  4. Need a basic mode for IJulia, CI etc. We'd need to disable this for dumb terminals and just do a linear print like it used to do.

  5. Timing tweaking. The successful packages only stay for a moment after succeeding. Increasing that might make it less busy, but would limit how much could be on screen at one time

Unrelated. Note the long pause at 1:53 immediately after pkg> add looks a bit like invalidation-related lag? Anyone else see that?


Edit: Given auto is now enabled in this PR, Ctrl-C interrupts are handled, and some other fixes are included
Closes #2093
Closes #2102
Fixes #2090

@IanButterworth IanButterworth changed the title Precompilation: Prettier printing Precompilation: Prettier printing? Oct 7, 2020
@KristofferC
Copy link
Member

Coolest shit ever!

@timholy
Copy link
Member

timholy commented Oct 7, 2020

That's really fun. The biggest problem I foresee is that people are then going to ask for rm_precompile just so they can watch it precompile again.

What's the difference between the white-bold and the rest? I expected the ones underneath to be dependencies but it seems they are not.

Note the long pause at 1:53 immediately after pkg> add looks a bit like invalidation-related lag? Anyone else see that?

I have seen something like that occasionally and wondered the same thing. It's a bit unexpected because precompilation doesn't load new code in the parent session, right?

@IanButterworth
Copy link
Member Author

What's the difference between the white-bold and the rest?

White bold = direct dependencies from the Project.toml
The rest = indirect dependencies, but not specifically of the packages above/near them.. the ordering is first-come first-served so rather unsorted

@IanButterworth
Copy link
Member Author

Perhaps the indent of the gray indirect deps is inaccurately making it look tree-like. I'll remove that.

precompilation doesn't load new code in the parent session, right?

As far as I know, yeah. Could it just be because I'm dev-ing Pkg, so it's not in the sysimage?

@KristofferC
Copy link
Member

You can run with --trace-compile=pkgprec.jl and then run tail on that. Maybe it will shown if new things are compiled. But I'm not sure how that acts with invalidations.

@haampie
Copy link

haampie commented Oct 7, 2020

Using the 128 core machine once more:

https://asciinema.org/a/6I9AN6uBTIHFoX4d7uCfnqh1q

@IanButterworth
Copy link
Member Author

@haampie still a bit of a firehose, but does seem more comprehendible

On the lag, I just built this branch into the julia sysimage and it disappeared 👍🏻

@KristofferC
Copy link
Member

The code could definitely be tidier. This is more of a UX demo.

It's pretty isolated code though so tidiness isn't too important. As long as it works well :P

No throwing. This doesn't throw errors, not even for direct deps (unlike master), but it shows when errors occur.

Perhaps it can throw in the very end if something failed? Just so if someone runs Pkg.precompile in their CI script it will abort if a package fails to compile.

Need a basic mode for IJulia, CI etc. We'd need to disable this for dumb terminals and just do a linear print like it used to do.

Maybe check if stdout / stderr (whatever is used) is a Base.TTY?

src/API.jl Outdated Show resolved Hide resolved
src/API.jl Outdated Show resolved Hide resolved
@fredrikekre
Copy link
Member

Perhaps the indent of the gray indirect deps is inaccurately making it look tree-like. I'll remove that.

Perhaps instead indent everything? Then it looks more like it is under the Precompiling project... "header".

@IanButterworth
Copy link
Member Author

Yeah, I agree. I did actually end up doing that. It's already committed.

@IanButterworth
Copy link
Member Author

@KristofferC

Perhaps it can throw in the very end if something failed? Just so if someone runs Pkg.precompile in their CI script it will abort if a package fails to compile.

But if multiple packages fail, and it throws at the end, we'd have to figure out how to show a stack trace for all packages that failed which could be a huge mess, otherwise the CI session wouldn't show the actual error and would be a bit opaque to the user. Instead, this approach reserves throwing proper package load errors to linear load time, which would be a lot more manageable. I can't think of a CI use case that would include Pkg.precompile() but not package loading.

This approach to not throw does change the current behavior though, so perhaps that's an unacceptable breaking change for a 1.x release?

@KristofferC
Copy link
Member

Couldn't we just throw an error in the end showing what packages failed? I don't understand the part about stack traces.

@IanButterworth
Copy link
Member Author

We could do that, but then it would be opaque on CI why the package failed. The stack trace wouldn't show the root reason why.

Happy to go with your judgement though 👍

@IanButterworth
Copy link
Member Author

I think this is finally ready to go

@KristofferC
Copy link
Member

Did you see JuliaLang/julia#37906 by the way? It removes the need to use TOMLCache. What order do you prefer, merge that first, wait until the nightly update and fix up things here, or merge this, bump Pkg on master, merge that PR and then fix Pkg again?

@IanButterworth IanButterworth changed the title Precompilation: Prettier printing, opt-out auto, auto control functions Precompilation: Prettier printing, opt-out auto Oct 11, 2020
@IanButterworth
Copy link
Member Author

Oh cool. Happy to hold off until that's merged & nightlied

src/API.jl Outdated Show resolved Hide resolved
@KristofferC
Copy link
Member

I merged the TOMLCache PR so if you want, you could remove all references to TOMLCache from here and then we wait until e.g. tomorrow when nightlies are updated to rerun CI here and merge.

@IanButterworth
Copy link
Member Author

I've added interrupt handling for Ctrl-C. Whether the interrupt happens during the print loop or precomp tasks it will exit simply after a pause while the top level tasks stop.
It will end pretty un-gracefully if Ctrl-C is button mashed.. but that's always going to be hard to handle.

Fixes #2090

@IanButterworth
Copy link
Member Author

I merged #2102 in here as there were mutually exclusive bugs preventing CI success

@DilumAluthge
Copy link
Member

DilumAluthge commented Oct 12, 2020

@ianshmean Can you add "closes #2102" somewhere in the first post of this PR?

Edit: fix a mistake

@IanButterworth
Copy link
Member Author

@DilumAluthge I think you meant #2102, which I added up top

@DilumAluthge
Copy link
Member

@DilumAluthge I think you meant #2102, which I added up top

Yep you are correct.

@DilumAluthge
Copy link
Member

Looks like Travis is passing now. @KristofferC is this good to merge once AppVeyor passes?

@DilumAluthge
Copy link
Member

I'm not sure why AppVeyor is reporting a failure. If I look at the AppVeyor test log:

[00:59:49]     Testing Pkg tests passed 
[00:59:49] C:\julia\bin\julia --project=test/coverage -e "using Pkg; Pkg.instantiate(); using Coverage; Codecov.submit(Codecov.process_folder())"
[00:59:50]    Updating registry at `C:\Users\appveyor\.julia\registries\General`

So it looks like the Pkg tests passed, but then something went wrong while AppVeyor was trying to upload to Codecov.

@DilumAluthge
Copy link
Member

@KristofferC or @fredrikekre Can you restart the AppVeyor job?

@KristofferC KristofferC merged commit 5098fd4 into JuliaLang:master Oct 12, 2020
@IanButterworth IanButterworth deleted the ib/precomp_pretty_printing branch October 12, 2020 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants