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

Exceptions in workload shouldn't prevent compilation #42

Open
aplavin opened this issue Sep 24, 2024 · 4 comments
Open

Exceptions in workload shouldn't prevent compilation #42

aplavin opened this issue Sep 24, 2024 · 4 comments

Comments

@aplavin
Copy link

aplavin commented Sep 24, 2024

Currently, whenever the compile_workload throws an exception, the package becomes effectively uninstallable.
That's especially bad for packages with comprehensive precompilation workloads: they become impossible to install and use in any capacity even when a tiny part of their functionality is broken by Julia / dependencies update.

Compile workload errors should just be turned into a small warning, like "precompilation workload failed, not all functionality may be available".

@timholy
Copy link
Member

timholy commented Sep 24, 2024

xref #21. There hasn't been a chorus of such requests, so I have to ask: how often does this happen? Can you just put try/catch in the workload if you're worried about this?

I recognize that not being able to use a package due to a failing precompile workload sucks. But changing this to a warning opens us up to real problems:

  • developer turns off precompilation for their local development
  • developer modifies a currently-working precompile workload but introduces a bug
  • developer commits and pushes up to github
  • CI works because it's only a warning, and the developer doesn't check the logs because CI passed
  • developer merges and tags a new release

Presto, now everyone using the package isn't getting effective precompilation. In contrast, if it's an error, CI fails and the release never gets made until the buggy workload is fixed.

@aplavin
Copy link
Author

aplavin commented Sep 24, 2024

I only searched over open issues, and was surprised to see this wasn't reported before :) Now I see that it was indeed.

There hasn't been a chorus of such requests

IMO, even two issues for such a package (not user-facing, nontrivial to understand PrecompileTools is the issue) are an indication that people do encounter this.

how often does this happen?

Of course, not too often in general. But I've definitely seen this a couple of times when either trying some pre-release Julia version, or upgrading packages individually (to preserve current versions as much as possible).
Both with my packages and in others, so "just put try/catch in the workload" is not a solution. Unless everyone does that, but then it's just a worse version of PrecompileTools giving warnings.

It's very frustrating to deal with such an issue, where one cannot even install a package. When encountering this, I just gave up and didn't upgrade julia/packages.

Re potential issues for development workflow: an obvious solution is to keep it an error in CI and/or in ]test – but turn into warning for regular usage. Both CI and tests already have different execution semantics than regular Julia, so this is a reasonable change.
More generally, promoting stuff like Revise and TestItems would help to avoid the need to disable precompilation workloads locally.

@timholy
Copy link
Member

timholy commented Sep 24, 2024

people do encounter this

I completely agree. The question is whether fixing it could make things worse in other ways.

an obvious solution is to keep it an error in CI and/or in ]test

Error only on CI has a fatal flaw: that's only assessed by ENV variables which are transient (they don't get hard-coded into the .so file). You could set CI to false, precompile, set CI to true, and you wouldn't trigger the error. Even worse, downstream packages could compile expecting to link against compiled code that isn't in the cache file. That's a linker error, which is far harder to diagnose. This is why developers' ability to disable precompilation is a Preferences.jl setting rather than an ENV variable---it forces recompilation when you change it.

What we'd need is some variable whose setting gets incorporated into the precompile "stale cache file" check. That means one of these: https://github.com/JuliaLang/julia/blob/a06a80162bb9bdf6f7e91dc18e7ccf5c12673ca4/src/staticdata_utils.c#L610-L614. At least on my machine, the only one changed by Pkg.test (default settings) is check-bounds, and of course in principle even that is subject to change. I'm not sure that repurposing that makes sense.

I really only see two options:

  • add another $ flag specifically for this purpose (see julia --help if you don't know what I mean by $ flag)
  • modify julia-actions/julia-runtest to set up a compile-time preference (via Preferences.jl), e.g., precompile_workload_error_is_fatal = true, before building any of the packages.

I lean towards the latter. But people who use, e.g., GitLab are left behind. Yuck. former, but it's a fair amount of work and requires changes to Julia itself.

More generally, promoting stuff like Revise and TestItems would help to avoid the need to disable precompilation workloads locally

No, developers really do need to turn off local precompilation in many cases. I had half a dozen people ask for that within a week of merging pkgimages (and I'm sure all of them knew about Revise), whereas this issue took more than a year to reach two requests.

@aplavin
Copy link
Author

aplavin commented Sep 30, 2024

Thanks, I understand it's nontrivial to fix while keeping other parts of user experience.
Still, I believe making packages installable is important, and will only become more important in the future.

Assuming PrecompileTools gets more and more traction, soon we'll start seeing unmaintained packages that use it. And they will naturally start losing compatibility with more recent Julia/packages versions (even those matching semver constraints). Without precompilation workloads, I typically can still install and load those packages, and then either rely on still-working functionality or monkey-patch some method in the tree.
But if those packages use PrecompileTools with reasonably complete workloads, they become uninstallable – end of story. Basically, it turns "something is broken" into "everything is broken".

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

No branches or pull requests

2 participants