make failure to precompile a method return a value instead of a unconditionally warn

[KristofferC]

Seeing this get spammed in the console as a user looks scary and is kinda annoying.
It is also annoying when looking through logs, here is a zoomed out view of a typical PkgEval log (these are all precompile warnings):

If someone wants to opt into showing these unconditionally, one can do something like #39922 (which I thing was a better idea in the first place rather than unconditionally logging).

[Sacha0]

Thanks Kristoffer! :)

I lack meaningful feelings about the maxlog bit.

[KristofferC]

I noticed now that this always returns true. That seems like a bug or was it made to unconditionally return true to deal with the @assert precompile in the wild? Not being able to tell if the precompile call was successful seems like a too heavy-handed approach to the issue. Added a commit that makes it return what it used to.

[maleadt]

That seems like a bug or was it made to unconditionally return true to deal with the @assert precompile in the wild?

Yeah that was intentional, #39905 (comment), c0f9666#commitcomment-47798892.

[KristofferC]

So we had a situation where users could choose based on the return value of precompile if they should warn, error, or whatever? Now it unconditionally warns and you cannot know if your precompile statement worked. Isn't the current situation much worse?

[KristofferC]

@nanosoldier runtests(ALL, vs = ":master")

[nanosoldier]

Your package evaluation job has completed - possible new issues were detected. A full report can be found here. cc @maleadt

[KristofferC]

Doesn't look so bad?

[KristofferC]

@timholy, do you have any thoughts about this. I think the current situation with 1) unconditional logging to warn and 2) inability to see if the precompile statement worked, is a significantly worse situation than before where a user could conditionally enable some logging and see if the precompile statement returned true.

[timholy]

I like the idea of still returning the status, independent of warning. It makes it easier to do automatic package analysis.

[KristofferC]

So you are ok with the current iteration (effectively reverting to 1.6 behavior)?

[vtjnash]

I don't think this is a good idea, and is now reverting #39905. Rather than its initially stated intent of reducing spam, this instead increases the error rate for PkgEval.

[KristofferC]

Rather than its initially stated intent of reducing spam, this instead increases the error rate for PkgEval.

I already tested this (https://github.com/JuliaCI/NanosoldierReports/blob/master/pkgeval/by_hash/06a31a0_vs_5584620/report.md) and there were only two packages that started failing due to it. Definitely worth it to me.

And reverting #39905 is the point because after that PR the situation is much worse (unconditional logging with no possibility of seeing the result of precompile for use with tooling).

Also, if we don't do this now, we are arguably stuck with the current bad behaviour since it then gets into a release.

[KristofferC]

@timholy, any thoughts?

[KristofferC]

I'm marking this with 1.7 because I think the current situation is so bad that it needs to be addressed before 1.7. The way you indicate a success/failure in these type of functions is via a return value, not via logging.

[vtjnash]

It has been a regular cause of PkgEval issue. Breaking 2 packages is still unneeded breakage.

[timholy]

If we have a list of packages, can we just remove the @asserts?

The recommendation to use that has long been gone from the SnoopCompile documentation, but I saw someone doing that recently. Poked around and noticed JuliaLang/www.julialang.org#1363

[timholy]

And FWIW I like the idea of precompile not lying about success.

[KristofferC]

It has been a regular cause of PkgEval issue. Breaking 2 packages is still unneeded breakage.

Well, I ran the whole suite and got 2 breakages so it doesn't seem like a big issue anymore. And these packages explicitly put @assert there to have them fail when their precompile statements stopped working so this is not breaking. We cannot turn every function that returns a boolean into a warning just because some people adds @asserts on them.

And it is not unneeded, it is to fix a regression in usability and ergonomics of precompile before that regression makes it into a release.

If we have a list of packages, can we just remove the @asserts?

Looking at the posted log, the only one is GridLayoutBase 0.4.1 (which is an old version) because some package is holding it back from the latest version. The latest version does not have the issue. Ironically, the change to precompile has caused the latest version to be buggy though (https://github.com/jkrumbiegel/GridLayoutBase.jl/blob/22c9f4f0d8fc61560f765e201bf6e0caeff38a1f/src/precompile.jl#L10) since it is using the return value of precompile.

[timholy]

If needed I am willing to help get a GridLayoutBase 0.4.2 out the door that shouldn't be blocked by Pkg [compat].

[JeffBezanson]

Could we make the warning opt-in instead?

[KristofferC]

You could just branch on the return value then and call whatever function emitting a warning that you like? Feels a bit pointless for Base to provide that.

[timholy]

In this vein, it's worth noting the pushback in timholy/SnoopCompile.jl#243 and JuliaGraphics/ColorTypes.jl#232.

[KristofferC]

The pushback there seemed to be mostly about implementation where a macro was used that expanded quite a lot of code on all precompile statements instead of just using a function that calls precompile and warns based on the return value.

Since the objection here seems to be about PkgEval which is not an issue from testing, I will merge this to get back to 1.6 status in a few days unless someone objects.

[vtjnash]

From PkgEval, there were 2 packages currently to fix still that block merging this

But I am confused, since the pushback you describe is what this code now implements, and what this PR deletes

And the solution you describe that packages should adopts is what this code does now (it removes the assert statements)

Both of those facts will be broken by merging this PR, in addition to those 2 packages.

[vtjnash]

The proposal from the triage call was to replace this unconditional warning with something that requires you to opt-in (e.g. during testing?) with a global const ENABLE_PRECOMPILE_WARNINGS = Ref(false) flag, or similar

[timholy]

But I am confused, since the pushback you describe is what this code now implements, and what this PR deletes

And the solution you describe that packages should adopts is what this code does now (it removes the assert statements)

I was raising it as an objection to my own proposal, not an argument in favor of my proposal (i.e., I was striving for balanced consideration, not winning the argument).

I would be happy with having the warning issued by Base, but I still favor restoring the accurate return value. Suppressing data just to work around a transient issue seems silly.

[KristofferC]

From PkgEval, there were 2 packages currently to fix still that block merging this

Why would they block merging this? The authors of the package GridLayoutBase v0.4.1 explicitly put @assert into their code to make it fail if certain conditions are met (the precompile statements stopped working). This PR fixes the broken behavior on master where the previous behavior of precompile was silently changed to always return true.

If you want packages to not use @assert for certain pieces of code I suggest doing PRs / opening issues against them to remove them.

As already been shown, the current behavior is a regression for anyone that relied on the previous behavior.
If you want to change it, we can perhaps do it in 2.0. The alternative is to reintroduce this in a non-breaking way (another method or an added keyword).

[timholy]

Some recommended changes, but let's do this.

[vtjnash]

The main reason I am opposed to the return value (other than breaking two packages), is I would like to eventually replace this code with something that isn't simultaneously doing two semi-unrelated operations. Right now, they happen to mostly overlap, but that causes this to work very sub-optimally. Even now, the boolean is already somewhat hard to compute, and does not correspond precisely to success/failure of the precompile.

[timholy]

How about we leave the boolean in place for now so that it's purely non-breaking, but allow Julia to issue the warning? Then when you reorganize this more extensively, we can deprecate older functionality (a natural time to point out the reservations about the return value).

[vtjnash]

Why is it so useful to add a return value here (which makes this PR a breaking change, since PkgEval fails now for 2 current packages, plus many old versions of packages now have incorrect version bounds)?