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 support for "package extensions" to code loading #47695

Merged
merged 7 commits into from
Dec 7, 2022
Merged

Conversation

KristofferC
Copy link
Member

@KristofferC KristofferC commented Nov 24, 2022

This is an implementation of the proposal in #43119 (with some tweaks) that aims to replace Requires.jl for loading code based on the presence of other packages. It is a different implementation to #47040 and should work better in for example big environments where you expect to only load a small subset of packages.

In spirit, it is very similar to Requires.jl in that given a set of packages and a file, you load the file when those packages have been loaded in the julia session. The differences to Requires.jl are:

  • The file with the conditional code gets precompiled in the same way as a normal package would. Requires.jl instead directly evaluates the code into the package.
  • It is possible to use Pkg to set compatibility on the packages that "activates" the conditional code. This is a new feature in Pkg and could in theory also be used together with Requires.jl.
  • It is "declarative" in the sense that all information requires id stored in the Project.toml and Manifest.toml file so it avoids having to run any arbitrary Julia code in e.g. __init__ (which is done when using Requries.jl).

The added docs in code loading as well as the added docs in the accompanying Pkg PR (JuliaLang/Pkg.jl#3264) can be used to get a more detail description of its usage.

A lot of the diff in this PR is due to the code loading being pretty awkward and would have been easier to be built on top of #46690 but I don't want to couple those PRs.

For reviewers: going through all the uuid lookups etc in the code loading is probably fairly uninteresting since it is mostly mechanical, the interesting parts are those around line 1080 - 1200 in loading.jl which deals with the insertion of gluepkgs "callbacks" and the loading of them.

An example of a real package moving from Requires.jl to using this system can be seen in https://github.com/KristofferC/PGFPlotsX.jl/compare/kc/glue. It should be pretty easy to be backwards compatible with Requires. You just put the @require inside a version check and include the file for the conditional file in the @require block.

One TODO that is left is that Pkg should precompile these "glue packages" during Pkg.precompile (which is often run automatically) in the case of them having a chance to be loaded in the current environment.

@KristofferC KristofferC force-pushed the kc/glue branch 2 times, most recently from 53d34f2 to a2b2714 Compare November 25, 2022 10:32
@KristofferC
Copy link
Member Author

Pushed a bump to Pkg here to include JuliaLang/Pkg.jl#3264 to run CI of these together.

@Seelengrab
Copy link
Contributor

Does this have downsides/a tradeoff compared to #47040?

@KristofferC
Copy link
Member Author

Some are discussed in #47040 (comment)

@Seelengrab
Copy link
Contributor

Gotcha, I just thought there may be some other ones. Thinking about the description of this in the Pkg PR, what happens when there's glue needed between glue, e.g. when you have weak dependencies A and B and want different behavior for when only A, only B or both A and B are loaded? Would this need GlueA, GlueB and GlueAB modules..?

@KristofferC
Copy link
Member Author

when you have weak dependencies A and B and want different behavior for when only A, only B or both A and B are loaded? Would this need GlueA, GlueB and GlueAB modules..?

Yes.

Another question is, what if you want to activate extra functionality inside a glue package when some other package is loaded. Basically glue^2. The reason I think about this is that I saw this line:

https://github.com/JuliaPlots/Plots.jl/blob/7bb8f6aee1c93d4dd478cc0d596a804abec8a723/src/backends/pgfplotsx.jl#L1042-L1046

in Plots, where a @requires block is used inside another requires block. In theory, this could be specified like:

[gluedeps]
PGFPlotsX = "..."
Measurements = "..."

[gluepkgs]
PGFPlotsGlue = "PGFPlotsX"
PGFPlotsMeasurementGlue = ["PGFPlotsGlue", "Measurements"]

And PGFPlotsMeasurementGlue would load when both PGFPlotsGlue and Measurements are loaded.

@Seelengrab
Copy link
Contributor

Another question is, what if you want to activate extra functionality inside a glue package when some other package is loaded. Basically glue^2.

And that's exactly the reason why I'm more in favor of the other proposal :D No risk of accidentally going quadratic in terms of complexity, at the cost of penalizing larger environments a bit (which probably should just use a large sysimage anyway?).

@KristofferC
Copy link
Member Author

@nanosoldier runtests()

@KristofferC
Copy link
Member Author

And that's exactly the reason why I'm more in favor of the other proposal :D No risk of accidentally going quadratic in terms of complexity, at the cost of penalizing larger environments a bit (which probably should just use a large sysimage anyway?).

The weak deps idea has some advantages and is more "powerful" since you have access to all information during precompile time, yes. But after thinking about this for quite some time I do think that the suggestion here is what should be tried first. Some reasons:

  • It is conceptually similar to Requires which people seem happy to use (with the exception of its problems which are more side effects of the implementation). This also makes it quite easy to be backward compatible with Requires (which the Pkg PR has docs for how to be).
  • When the "conditional features" (or glue packages) are separate modules that are loaded, the extra stuff is more or less "pure" additions to what the base package offers. I think that would mean that it would be easier to test each thing separately.
  • I think the amount of recompilation that would be required with the weak deps thing is pretty bad. If you remove a package, you might have to recompile your whole environment. With glue deps, adding a package will at most cause you to compile new glue packages (which are small), and removing a package will never require new compilation.
  • I think this will on average give users a smaller load time. Weak dependencies really punish you if you don't have super clean environment usage and a lot of people just don't use Julia like that. We can say "oh but people should make sysimages or have smaller envs etc" but if that is just not how the majority of people use Julia, that should be taken into account.'

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

@KristofferC
Copy link
Member Author

PkgEval looks good.

NEWS.md Outdated Show resolved Hide resolved
@@ -348,7 +348,47 @@ The subscripted `rootsᵢ`, `graphᵢ` and `pathsᵢ` variables correspond to th
2. Packages in non-primary environments can end up using incompatible versions of their dependencies even if their own environments are entirely compatible. This can happen when one of their dependencies is shadowed by a version in an earlier environment in the stack (either by graph or path, or both).

Since the primary environment is typically the environment of a project you're working on, while environments later in the stack contain additional tools, this is the right trade-off: it's better to break your development tools but keep the project working. When such incompatibilities occur, you'll typically want to upgrade your dev tools to versions that are compatible with the main project.
### "Glue" packages and dependencies
Copy link
Member

Choose a reason for hiding this comment

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

Obviously there are more changes to like this, but just to mark it as I think this is where the discussion settled

Suggested change
### "Glue" packages and dependencies
### "Weak" dependencies and "Glue" packages

doc/src/manual/code-loading.md Outdated Show resolved Hide resolved
doc/src/manual/code-loading.md Outdated Show resolved Hide resolved
doc/src/manual/code-loading.md Outdated Show resolved Hide resolved
@KristofferC
Copy link
Member Author

For those following along, it is likely that "glue packages" will be renamed to "package extensions".

Copy link
Member

@IanButterworth IanButterworth left a comment

Choose a reason for hiding this comment

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

Awesome.

@KristofferC KristofferC merged commit 495a004 into master Dec 7, 2022
@KristofferC KristofferC deleted the kc/glue branch December 7, 2022 15:01
@KristofferC KristofferC added the backport 1.9 Change should be backported to release-1.9 label Dec 8, 2022
KristofferC added a commit that referenced this pull request Dec 8, 2022
* Add support for "glue packages" to code loading

This allows packages to define "glue packages" which
are modules that are automatically loaded when
a set of other packages are loaded into the Julia
session.

(cherry picked from commit 495a004)
else
require(extid.id)
@debug "Extension $(extid.id.name) of $(extid.parentid.name) loaded"
end
Copy link
Member

@johnnychen94 johnnychen94 Dec 8, 2022

Choose a reason for hiding this comment

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

Not sure if this is a deliberate design so wanna ask it here. Does it sound like a good idea to make Plotting.ContourExt a "normal" submodule? And if not loaded, Plotting.ContourExt keeps to be nothing.

This makes it easy and intuitive to get objects inside the glue module, for instance:

Base.get_extension(Plotting, :ContourExt).Contour

becomes

Plotting.ContourExt.Contour

One hack/workaround is to insert

module ContourExt

using Plotting

function __init__()
    setproperty!(Plotting, :ContourExt, Base.get_extension(Plotting, :ContourExt))
end
end

Copy link
Member Author

Choose a reason for hiding this comment

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

The extensions should in my opinion be "private" and their behavior should almost only be visible to users by new dispatch rules being defined. You shouldn't really need to reach into the extension module itself. The get_extension is thus mostly for debugging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packages Package management and loading
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants