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

Add code loading support for Preferences package #37595

Merged
merged 1 commit into from
Sep 28, 2020
Merged

Conversation

staticfloat
Copy link
Member

@staticfloat staticfloat commented Sep 15, 2020

EDIT: This PR has now changed to add only the code-loading portion.

This commit adds the Preferences standard library; a way to store a
TOML-serializable dictionary into top-level Project.toml files, then
force recompilation of child projects when the preferences are modified.

This pull request adds the Preferences standard library, which does
the actual writing to Project.toml files, as well as modifies the
loading code to check whether the preferences have changed.

base/loading.jl Outdated
# check that project preferences match by first loading the Project.toml
active_project_file = Base.active_project()
if isfile(active_project_file)
preferences = get(parsed_toml(cache, active_project_file), "preferences", Dict{String,Any}())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry if I missed earlier discussion on this. But wouldn't this behave a bit confusingly when you stack environments?

Suppose I have LOAD_PATH = ["@", "@v#.#", "@stdlib"] and some preferences for SomeREPLUtilities.jl in ~/.julia/environments/v1.6/Project.toml. If I activate $PWD/Project.toml and then evaluate using SomeREPLUtilities, does it mean the preferences in ~/.julia/environments/v1.6/Project.toml would be ignored? How about looking at all project files in Base.load_path()?

Copy link
Member

@fredrikekre fredrikekre Sep 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should only check the project file where SomeREPLUtilities is found I guess. If you need local preferences for that package you should probably add the package to the local project.

Edit: In fact, perhaps we should explicitly disallow adding preferences for packages not in the project?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes, good point. I agree.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's purposeful that it only checks the top-level project file. We decided against any kind of merging for a few reasons:

  1. Reproducibility; if a Project.toml somewhere high up in a chain is effecting things, then simply saving the Project.toml and Manifest.toml that you normally do would not be enough to guarantee identical results later on; we want all the configuration stored in one place.

  2. Performance; we don't want to have to touch a bunch of files. This can be ameliorated with caches, of course, but it's nice that we only have to read in one file.

  3. Merging complexity; since we're storing full-on Dict's here, what happens if a higher-up and lower-down Projects have conflicts around key mappings?

I'm torn on adding preferences for packages not in the project; it makes sense to disallow it, but it's also harmless.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I favor disallowing the inclusion of preferences for packages not in the project simply because allowing it allows a[two] non-involved package[s] to carry preferences that may conflict were that[those] package[s] to appear in the project at a future time. It works against clear communication in collaboration.

Copy link
Member

@fredrikekre fredrikekre Sep 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @tkf: If you need preferences on B in Y you can just add B such that it exists in the Project.toml file (#37595 (comment)).

I agree that it will be difficult to get a clean env though. But perhaps it is simple enough to play around with the LOAD_PATH then? I for one quite often use export JULIA_LOAD_PATH=$(mktemp) when I need to play around with a clean env.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, it does become a bit messy; when you load A and thus B is loaded implicitly, should you really go look for deps for B lower in the load path in a project that might have a different version of B?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this manner, the behavior of using B is already changing based on transitive dependencies.

Yeah, I agree that's an issue. But isn't it an issue somewhat orthogonal to preferences? I think it'd be better to only support the case where the manifests are compatible and explicitly state that otherwise, the behavior is undefined. I think it's better to solve the issue with incompatible manifests with something like #32906.

But I can see that combining two manifests can change the version of a package in a manner incompatible with the stored preferences.

If we can change how the package resolution works in a stacked environment, how about

  1. When looking for a package to load, first try stacked Project.toml files. This way, the package version explicitly added by the user takes precedence over the indirect dependencies in the environments above.
  2. Allow preferences to be set only when the package is added by the user (i.e., in deps).
  3. For a given package, use the first Project.toml that contains the package as a dependency. If there are no preferences, use the default. (This is my implementation in Add code loading support for Preferences package #37595 (comment))

I think the combination of these three ingredients ensure that preferences and package version are compatible while allowing the scenario I described in #37595 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any update on this? I think falling back to the lower environment stack is a key feature. I think one of the important use-cases for CompilePreferences.jl is to capture machine-specific configuration such as system libraries and hardware properties. For this, ~/.julia/environments/v1.6/Project.toml sounds like a good place to store such information. It doesn't make sense to share Project.toml with machine-specific values.

I think I need this for PyPreferences.jl JuliaPy/PyCall.jl#835. If it is decided that it is not appropriate to add this to CompilePreferences.jl, I think I'll create SharedPreferences.jl to handle this use-case (e.g., by storing the preferences in ~/.julia/sharedpreferences/$UUID/v$major.toml and call include_dependency on it). I'll look up per-project preferences first via CompilePreferences.jl and then fallback to SharedPreferences.jl. The fact that it is implementable outside Base could mean that not adding this in Base/CompilePreferences.jl is a good idea. But having many ways to store preferences may be confusing. I'm not sure what's the best approach here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened an issue for this discussion #37791

@staticfloat
Copy link
Member Author

There's another small design point I want to bring up; right now, all preferences stored will force recompilation. That may not be desirable in all cases, perhaps a package wants to store something that is only used at runtime (and hence does not need recompilation) but still wants it bundled into the Project.toml. I'm not sure what the best API for this is.

One possibility is that we store all the compilation-time preferences under the build_preferences key and the non-compilation-time preferences under the preferences key in the TOML, then loading.jl only pays attention to build_preferences. We would then, in Preferences, add a keyword argument to all the getters and setters that switches whether you're saving a build-time preference or not. The part of this that is slightly awkward to me is that it feels strange to have two separate dictionaries that you have to manage, but perhaps that's not that awkward and is fine. It's certainly better (from an implementation perspective) than tagging every key in a dictionary that is marked as require-recompilation, which is one of the the single-dict options.

@fredrikekre
Copy link
Member

IMO some system like that seem unnecessary complicated -- is it really so bad to reprecompile even though it will not change the result?

@JeffreySarnoff
Copy link
Contributor

Only if there is an undesired cost to that reprecompilation .. and the run-time only preferences are changing at a rate that exacerbates the disutility of that cost. Is that something that is likely to occur now and again?

@tkf
Copy link
Member

tkf commented Sep 15, 2020

How about adding an interface like @get_preference(key, default) (and recommend using it over @load_preferences)? This way, we can get a list of keys used during precompilation time. Maybe this is enough for optimizing unnecessary precompilation in the future?

@staticfloat
Copy link
Member Author

is it really so bad to reprecompile even though it will not change the result?

Well, I can imagine there are users that want to do things like store ML model hyperparameters, or plotting backend (I'm assuming changing that can be done completely at runtime), or whatever. There are all sorts of useful things to store, (and even ones that you might want to communicate to others with your Project.toml) and right now this is the closest thing to what a user might use to store preferences in their packages.

An alternative approach would be to write a second package that just gives a nice interface to TOML and Scratch to store non-compile-time preferences in a Scratch space.

@staticfloat
Copy link
Member Author

staticfloat commented Sep 16, 2020

I just got another question from a user that wanted to know if they should use Preferences to store things like whether a package had been run before, so they could give a one-time welcome message. I think it's clear users want a key-value store and that they're not thinking about the fundamental capabilities of this package properly, because the name is too general and inviting. :)

I suggest re-naming this to CompiletimePreferences, and we can create a separate Preferences package that is not a stdlib, that just stores preferences to a TOML file in a Scratch space. How does that sound to everybody?

@vchuravy
Copy link
Member

Maybe instead of CompiletimePreferences, ProjectPreferences? E.g. noting where the preferences live? I don't like the moniker Compiletime since it is a very "in-group" name, and most users shouldn't worry about compiletime vs runtime. Other ideas are Settings or CachedPreferences.

@tkf
Copy link
Member

tkf commented Sep 16, 2020

For storing software states like a welcome message, I think it's not ideal to contaminate user's Project.toml file. Isn't Scratch.jl the best way to do it?

But I think I agree separating compile-time and run-time key-value store is a clean design.

I suggest re-naming this to CompiletimePreferences

Brain-storming other names: CompileOptions/CompileConfigs.

@fredrikekre
Copy link
Member

Some other ideas: PkgConfig, PackageConfiguration, ProjectConfig[uration], ProjectSettings

@staticfloat
Copy link
Member Author

I don't like the moniker Compiletime since it is a very "in-group" name, and most users shouldn't worry about compiletime vs runtime.

I argue the only reason anyone should ever use this stdlib is because they (as a package developer) need to worry about compiletime vs runtime. Anyone who doesn't care about the difference between runtime and compiletime either should use a different package (like TOML) or actually does need to care about the difference.

I think keeping the term Compile in there or maybe Build still communicates clearly that it's got a specific purpose.

  • BuildPreferences/BuildConfig/BuildOptions
  • CompilePreferences/CompileConfig/CompileOptions

I'm not so hot on Package* because that again feels a little too general.

@tkf
Copy link
Member

tkf commented Sep 16, 2020

I think Compile* is better than Build* unless we can call @load_preferences in deps/build.jl.

@KristofferC
Copy link
Member

KristofferC commented Sep 16, 2020

unless we can call @load_preferences in deps/build.jl.

We don't talk about that anymore. In fact, I don't even know what that is. Looks like some random file in a random non-special directory. ( ;) )

@KristofferC
Copy link
Member

I also like to focus this package on compile-time preferences. However, some questions: For non-compile-time preferences, how does a user pass the preferences to the package? It's not via the project file so does the package have to write such an API then? Also, perhaps there are non-compile time preferences that you want to persist when you move your project to another machine. How would those be transferred?

@tkf
Copy link
Member

tkf commented Sep 16, 2020

Sorry to mention the file-must-not-be-named :)

If we don't want to talk about it, I think Compile* is a better prefix.

For non-compile-time preferences, how does a user pass the preferences to the package? It's not via the project file so does the package have to write such an API then?

I thought Project.toml would have two tables for two kinds of preferences as in

[deps]
PkgA = "342fba16-3e17-4664-b1bb-a60ccdbe268d"

[compile-preferences.342fba16-3e17-4664-b1bb-a60ccdbe268d]
compile_time_preference = "value"

[runtime-preferences.342fba16-3e17-4664-b1bb-a60ccdbe268d]
runtime_preference = "value"

@staticfloat staticfloat force-pushed the sf/preferences branch 5 times, most recently from 9bc5740 to b0f5abe Compare September 16, 2020 21:53
@staticfloat
Copy link
Member Author

unless we can call @load_preferences in **************.

I thought Project.toml would have two tables for two kinds of preferences as in

Yes, I suggested this idea earlier and I think it's still my favorite idea. I've moved this PR to store things under the key compile-preferences, and then Preferences can just be a normal package that stores things in Project.toml and that will not trigger recompilation.

@staticfloat staticfloat force-pushed the sf/preferences branch 2 times, most recently from 466867b to e9e4d60 Compare September 17, 2020 21:43
@staticfloat
Copy link
Member Author

Alright, I think I've successfully implemented the desired functionality here; the preferences are now loaded from the first environment that knows what the given UUID is, and saving preferences goes to that same location, defaulting to the currently-active project in the event that you're using a UUID that you've never add'ed before. This seemed safer to me than throwing an error, as it means that if users start add'ing/removing packages they're less likely to get strange errors, and it enables the usage of things like preferences with @requires, on the off-chance that someone sets a preference in a package before checking to see if it's actually a part of their project.

@fredrikekre fredrikekre changed the title Add Preferences standard library Add CompilePreferences standard library Sep 17, 2020
@musm
Copy link
Contributor

musm commented Sep 18, 2020

Wouldn't a name like PkgPreferences or PkgConfig be a lot more appropriate? CompilePreferences sounds to me like it's related to static compilation or sorts. I think including Pkg is desirable as this is tied with the Pkg manager.

@JeffreySarnoff
Copy link
Contributor

Wouldn't a name like PkgPreferences ..

Yes. Re-reading this thread, ProjectPreferences is even more accurate; although people are more familiar with Packages than Projects in Julia.

@tkf
Copy link
Member

tkf commented Sep 18, 2020

There will be another package named Preferences or PkgPreferences or ProjectPreferences or something else. As discussed above, we need two packages. One of which can be used during precompilation so I think it's very natural to have "Compile" in its name.

Even if the name CompilePreferences is not intuitive, I don't think that's a big problem. Presumably, people would then just use Preferences. If they use Preferences when they should be using CompilePreferences, it's pretty easy to detect this and emit a friendly warning to nudge the users to switch to CompilePreferences.

@tkf
Copy link
Member

tkf commented Sep 25, 2020

I started playing with this branch to see if I can do what I had in mind for PyCall.jl. So far I think it's working as expected:
JuliaPy/PyCall.jl#835
https://github.com/tkf/PyPreferences.jl

Edit: I noticed that TOML file has strange formatting JuliaPy/PyCall.jl#835 (comment). It's not super important ATM but would be nice to fix it before 1.6 is out:

# <blank line>
[compile-preferences.cc9521c6-0242-4dda-8d66-c47a9d9eec02]
python = "python3.8"
[deps]
PyCall = "438e738f-606a-5dbb-bf0a-cddfbfd45ab0"
PyPreferences = "cc9521c6-0242-4dda-8d66-c47a9d9eec02"

i.e., a blank line before [compile-preferences....] and no blank line before deps

@KristofferC
Copy link
Member

There will be another package named Preferences or PkgPreferences or ProjectPreferences or something else. As discussed above, we need two packages.

I didn't really understand why two packages are needed. In my opinion (and as was discussed on the triage call), all preference stuff can live in the same package, it doesn't have to be a stdlib, code loading just know that it has to recompile if things under [compile-preferences] change. It is the same way as how Pkg doesn't technically need to be a stdlib, or that other things that write project and manifests don't have to be a stdlib. They communicate via a "serialized format" (a file) and the file format has docs. [compile-preferences] is just one extra thing.

@tkf
Copy link
Member

tkf commented Sep 25, 2020

I think many preference setup currently done with environment variables can be handled by "runtime-Preferences.jl" package. I don't think it's a good idea to re-compile packages when changing such runtime preferences. Grepping julia, some candidates are

JULIA_ERROR_COLOR
JULIA_WARN_COLOR
JULIA_INFO_COLOR
JULIA_STACKTRACE_EXPAND_BASEPATHS
JULIA_STACKTRACE_CONTRACT_HOMEDIR
JULIA_STACKTRACE_LINEBREAKS
JULIA_WORKER_TIMEOUT
JULIA_PKG_SERVER
JULIA_PKG_OFFLINE
JULIA_PKG_DEVDIR
JULIA_PKGRESOLVE_ACCURACY
JULIA_PKG_CONCURRENCY
JULIA_PKG_PRECOMPILE_AUTO
JULIA_NUM_PRECOMPILE_TASKS

(But depending on how the merging of runtime-Preferences.jl work, some of them may not be appropriate.)

A potential benefit of using runtime-Preferences.jl is that we can provide much better UI for configuring these values (e.g., using TerminalMenus.jl).

They communicate via a "serialized format" (a file) and the file format has docs.

I agree that CompilePreferences package itself does not have to be in stdlib.

@KristofferC
Copy link
Member

KristofferC commented Sep 25, 2020

I don't think it's a good idea to re-compile packages when changing such runtime preferences.

Totally, agree but why does that require two packages?

Regarding your examples, I also don't think those really belong in a project file. They are more system-wide than that.

Also, #2716 is highly relevant.

@tkf
Copy link
Member

tkf commented Sep 25, 2020

Ah, I think I see what you are getting at. Yeah, it should be fine as long as you can determine whether a given preference in the project is for compile-time or not. It's more about toml file schema than the fronted package(s).

@KristofferC
Copy link
Member

The only slight annoyance is that the user has to know whether the preference is a compile-time or not. It would be nice if that is up to the package.

@fredrikekre
Copy link
Member

The only slight annoyance is that the user has to know whether the preference is a compile-time or not. It would be nice if that is up to the package.

Isn't the idea that you use API functions for setting the variables? E.g. the examples here: https://github.com/JuliaLang/julia/pull/37595/files#diff-153066f1b4b0aa6caf26959fc7f1ce57

@JeffreySarnoff
Copy link
Contributor

JeffreySarnoff commented Sep 25, 2020

keep it simple [while powerful and etc], so its easier for many to use

as a transitive dependency) will be the one that is searched in for preferences.
"""
function load_preferences(uuid::UUID)
prefs = Dict{String,Any}()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This variable seems to be unused.

@tkf
Copy link
Member

tkf commented Sep 25, 2020

Isn't the idea that you use API functions for setting the variables?

Yeah, I think the API functions would be the primary way to tweak the preferences. If you have something like

[compile-preferences.342fba16-3e17-4664-b1bb-a60ccdbe268d]
compile_time_preference = "value"

figuring out the UUID by hand is tedious.

@KristofferC
Copy link
Member

KristofferC commented Sep 25, 2020

Being super lazy and not checking myself, are "global" preferences also available, for example

[compile-preferences]
debug = true

which is then a shared namespace among the packages.

@staticfloat
Copy link
Member Author

are "global" preferences also available, for example

I originally had those stored within a "global" UUID of UUID(UInt128(0)), and we could make that a convention. You don't want to be able to get a top-level dict, as then you'll get the preferences for your sibling preferences as keys within the global dict.... easy to screw something up then.

This adds the calculation, serialization and verification of preferences
hashes at code loading time.  Preferences, as stored by the forthcoming
`Preferences.jl` package within a top-level `Project.toml` file, are
parsed by the `dump.c` and `loading.jl` code loading machinery and used
to provide a compile-time preferences machinery.
@staticfloat staticfloat changed the title Add CompilePreferences standard library Add code loading support for Preferences package Sep 28, 2020
@staticfloat
Copy link
Member Author

I've modified this PR to only include the Project.toml parsing and hashing parts in loading.jl and dump.c; the user-facing API is now in the new Preferences.jl package, let's move all API discussion to issues and pull requests to that package.

As a quick taste, it has two sets of bindings, one through Preferences and one through Preferences.CompileTime.

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

Successfully merging this pull request may close these issues.

9 participants