Refactor disable_invalidation to force_compiletime_default

[JamesWrigley]

This makes the argument always return the default value to avoid invalidating the compilation cache.

See #90.

[codecov]

Codecov Report

❌ Patch coverage is 85.71429% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.88%. Comparing base (98fa66a) to head (6493c30).
⚠️ Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
src/Preferences.jl	85.71%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #92      +/-   ##
==========================================
+ Coverage   93.82%   93.88%   +0.06%     
==========================================
  Files           2        2              
  Lines         178      180       +2     
==========================================
+ Hits          167      169       +2     
  Misses         11       11              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[topolarity]

Can you explain a bit about how this works for your use case @JamesWrigley ?

My understanding of JuliaLang/IJulia.jl#1215 is that Preferences allow users to specify the IJulia path when the default is non-functional. Doesn't this change mean that those users will fail to pre-compile IJulia, since the preference will not report the user-provided path when you run notebook() during precompilation?

[JamesWrigley]

Nope, we can't run the command at all during precompilation because it's not guaranteed that Jupyter is installed anyway (it's also a bit awkward to launch a non-terminating command during precompilation). I'll probably add a special case that exits run_subcommand() during precompilation before calling launch(): https://github.com/JuliaLang/IJulia.jl/blob/8e17d35ec29c89a7b51b75fd649021a94c647c3c/src/jupyter.jl#L92

The actual path doesn't matter for the purposes of precompilation, it could also be the path to true.

[topolarity]

That makes some sense, but why is IJulia querying the path if it's not going to use it?

[JamesWrigley]

It's purely for the sake of precompilation to reduce TTFX; because IJulia is used so widely I try to reduce TTFX as much possible. Most of my work on that so far has been to reduce TTFX for the kernel itself, but I can't think of many more ways to reduce the kernel TTFX so I'm tackling the notebook()/jupyterlab() commands next.

[topolarity]

What are you trying to precompile, the load_preference function or downstream code that uses the String from it?

I'm having trouble seeing the value in calling load_preference at pre-compile time. It seems like you could just as well use the default value directly at pre-compile time (if that's acceptable) and have the same result.

[JamesWrigley]

Ah no, it's the downstream code. I'm not sure what you mean by using the default value, do you mean adding an argument like path=@load_preference("foo", "bar") and explicitly passing path during precompilation?

[topolarity]

Yeah, that would be one way to do it.

The basic argument is: Since you're already adding special cases to exit before launch() in pre-compilation, why not add a special case to skip the load_preference and just use the default path?

[JamesWrigley]

One special case is better than two 😛 But more generally I think having code being amenable to precompilation without refactoring is really useful. In the case of notebook() I agree that the advantage of force_compiletime_default is less clear-cut because it needs some refactoring anyway to be precompiled, but in general for preferences where the value doesn't matter for precompilation (and Preferences is used for the sake of user-friendliness) I would argue it's better that that special case logic goes inside Preferences.

[topolarity]

Sorry, but I don't agree. Users are welcome to provide this functionality manually using something like:

path = default
if !Base.generating_output()
    path = load_preference(...)
end

but I don't think it makes sense to provide this logic in the standard API. Preferences should assume that, by default, the values it returns do matter, including at pre-compile time.

One special case is better than two 😛

Not by very much IMHO - Users are already having to customize their code to handle the fact that Preferences doesn't return the "right" thing here - I don't think the API should paper over that. It should make it obvious, and expect the user to implement their special case pre-compile logic, even if that means editing multiple places.

[JamesWrigley]

Preferences should assume that, by default, the values it returns do matter, including at pre-compile time.

Agreed, and this PR doesn't change that. It only adds an optional (and I think fairly obvious) way to opt-out, which is something users will have to do one way or another. That's why I think it makes sense to have in the package.

I understand your reasoning though, and I don't think I have much more to add. If you and the other maintainers agree then I can make a PR to revert #90.

[Keno]

I think the kwarg is fine and a reasonable way to express semantic intent

[staticfloat]

Preferences should assume that, by default, the values it returns do matter, including at pre-compile time.

I agree that this should be the default, but providing a way to disable this here makes sense to me. As a code organization decision, I think having a way to prevent a preference from invalidating you belongs with the preferences code rather than in the caller's code. In this specific instance the difference in lines of code is negligible, but I think it is ever so slightly cleaner to have things like Base.generating_output() in Preferences.jl where we have other code of that ilk, rather than in the caller's code.