-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
make failure to precompile a method return a value instead of a unconditionally warn #41447
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Kristoffer! :)
I lack meaningful feelings about the maxlog bit.
🤷 I don't Co-authored-by: Jameson Nash <[email protected]>
I noticed now that this always returns |
Yeah that was intentional, #39905 (comment), c0f9666#commitcomment-47798892. |
So we had a situation where users could choose based on the return value of |
@nanosoldier |
Your package evaluation job has completed - possible new issues were detected. A full report can be found here. cc @maleadt |
Doesn't look so bad? |
@timholy, do you have any thoughts about this. I think the current situation with 1) unconditional logging to |
I like the idea of still returning the status, independent of warning. It makes it easier to do automatic package analysis. |
So you are ok with the current iteration (effectively reverting to 1.6 behavior)? |
139e09e
to
f64aecd
Compare
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. |
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 Also, if we don't do this now, we are arguably stuck with the current bad behaviour since it then gets into a release. |
@timholy, any thoughts? |
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. |
It has been a regular cause of PkgEval issue. Breaking 2 packages is still unneeded breakage. |
If we have a list of packages, can we just remove the 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 |
And FWIW I like the idea of |
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 And it is not unneeded, it is to fix a regression in usability and ergonomics of
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 |
If needed I am willing to help get a GridLayoutBase 0.4.2 out the door that shouldn't be blocked by Pkg [compat]. |
Could we make the warning opt-in instead? |
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. |
In this vein, it's worth noting the pushback in timholy/SnoopCompile.jl#243 and JuliaGraphics/ColorTypes.jl#232. |
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 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. |
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. |
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 |
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. |
Why would they block merging this? The authors of the package GridLayoutBase v0.4.1 explicitly put If you want packages to not use As already been shown, the current behavior is a regression for anyone that relied on the previous behavior. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some recommended changes, but let's do this.
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. |
Co-authored-by: Tim Holy <[email protected]>
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). |
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)? |
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).