-
Notifications
You must be signed in to change notification settings - Fork 64
Do not inform user during precompilation #226
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
base: master
Are you sure you want to change the base?
Do not inform user during precompilation #226
Conversation
84f14e7 to
04bb5f1
Compare
The current approach seems to have multiple disadvantages: - If the user only got `TensorOperatoins.jl` as a indirect dependency, the information seems to be completely useless - It's untypical to give information about a package during precompilation. That's what Readmes, docstrings or similar things are for - The user can't deactivate it. If I read the code correctly, even setting the preference to false would print the warning again So until a better solution is found, I think it makes sense to deactivate the precompile output.
04bb5f1 to
11e2ad2
Compare
Codecov Report✅ All modified and coverable lines are covered by tests.
... and 14 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
Hi Patrick, Thanks for taking the time to look into this. Before I share my opinions, allow me to maybe give you a bit of background first. What we really wanted was to simply enable the precompilation. The reason this message is here is because some users, typically on clusters, have reported that the precompilation process takes forever (30min+), and we (I) have not been able to identify the source of that at all. In these circumstances it is a bit annoying to have to disable precompilation, since the suggested workflow for that requires first loading TensorOperations, which means first waiting for the entire precompilation process to finish. We felt like the message during precompilation was a good balance between informing people about this, while not having to show the message every time the Julia runtime was restarted. Of course it is possible that we have overlooked specific workflows for which this would be harmful, in that case do feel free to share how this is affecting you. I agree that it could be nice to provide a way of disabling it, but I would prefer not to simply remove this. Note that even if you indirectly load TensorOperations, you benefit from the precompilation as long as any part is calling into a contraction. Given that the start-up times from TensorOperations can be somewhat substantial, it is hard to strike a balance here. Of course happy to discuss further! |
|
Hi Lukas, thanks a lot for the information, which makes it much clearer how we came to the current solution. Let me also give you some background to have an informed discussion. I noticed that Julia 1.12 has a worse user experience than 1.10 or 1.11, because on first start (e.g. after a minor update), a lot of packages are precompiled (this is normal) and then the terminal is flooded with warnings/infos (at least on two of my systems, obviously depending on the number of installed packages, but this is not normal). This looks unfinished and I think also a bit scary, at least initially for the more experienced Julia developer until they read multiple pages of output. The less experienced Julia developer probably either ignores it or gets the feeling of poor quality. Therefore, I went on a mission to submit PRs to every one of them to get back to an output-free precompilation of all installed packages (at least on my systems). I think I succeeded to submit a solution to all other packages. However, fixing the general problem in I understand why you choose to inform the user about the precompilation issue, because that is what the initial problem was about. However, I'd argue, that the way which the user is currently informed is untypical and that regular documentation is preferable. Note, that most packages precompile a (relatively speaking possibly infinitely small) subset of their method instances / specializations. This is because every method which accepts an abstract type or a UnionAll type can have a theoretically infinite or at least large number of method instances. When the usage pattern is not fully known by the package author (which typically is the case for such methods, because there normally is a reason why the method uses such a broad interface), the point where adding a gazillion of additional method instances to the package image quickly outweights the benefits of some random hits of these cache entries. Therefore, it's quite normal that users of your package (including packages which use your package) need to have some precompiles about methods from your package in their packages (or an environment-specific "precompilation package" if the users want to have precompilation benefits in non-package code). In addition, the precompilation info was really not actionable for me. I am used to scan through actual precompiles for my programs to try to fix them. This is often relatively easy, as with tracing of precompiles active, you know exactly which method instance is the problem and already get a copy-paste solution proposed. using Pkg
npackages(mode) = (io = IOBuffer(); Pkg.status(; mode, io); io) |> seekstart |> countlines |> x -> x-2
1 - npackages(PKGMODE_PROJECT) / npackages(PKGMODE_MANIFEST)On my current systen in my default environment, it's 80%. Therefore, I propose to move the "precompilation info" into the documentation. If the precompilation in If the precompilation problem is not worse than in other packages, I don't think that it should be mentioned so prominently as it is currently done or would be done with the above proposal. Then I think the already existing separate precompilation section in the manual is the right balance. The current PR implements this. |
The current approach seems to have multiple disadvantages:
TensorOperatoins.jlas a indirect dependency, the information seems to be completely uselessSo until a better solution is found, I think it makes sense to deactivate the precompile output.