WIP: Implement new manifest format with top-level metadata #2557

IanButterworth · 2021-05-09T05:29:49Z

A WIP go at implementing a new manifest format that:

Adds julia_version and project_hash metadata fields
Moves the packages from top-level to a deps field

Dependent on:

Add support for new Manifest.toml format during code load julia#40765 - CI has been limited to linux 64, and that PR's binary is used (ignore appveyor)
If the project_hash is going to be implemented, the hash method needs fixing Fix two Base.hash methods and add tests #2556

Example manifest:

# This file is machine-generated - editing it directly is not advised

julia_version = "1.7.0-DEV.1077"
project_hash = "nothing"

[[deps.Base64]]
uuid = "2a0f44e3-6c83-55bd-87e4-b1978d98bd5f"

[[deps.Distributed]]
deps = ["LinearAlgebra", "Random", "Serialization", "Sockets"]
uuid = "8ba89e20-285c-5b6f-9357-94700520ee1b"

Notes:

Backward compatible with the old manifest format. When an old manifest format is detected, the julia_version is set to nothing
Needs new format manifest tests added
The hash checking isn't implemented properly yet. Needs some thought about when the project is hashed and saved to the manifest. Also if/when the user should be warned
Should version warnings be just for major+minor julia version mismatches?
Tests passing - Ignore appveyor

IanButterworth · 2021-05-09T14:52:02Z

src/API.jl

@@ -1024,8 +1024,8 @@ function precompile(ctx::Context; internal_call::Bool=false, strict::Bool=false,
        for (name, uuid) in ctx.env.project.deps if !Base.in_sysimage(Base.PkgId(uuid, name))
    ]

-    man = Pkg.Types.read_manifest(ctx.env.manifest_file)
-    deps_pair_or_nothing = Iterators.map(man) do dep
+    man = ctx.env.manifest


I don't see why the manifest needed re-reading here. I may have just been unaware of the manifest cache when writing it. This change should just speed things up

DilumAluthge · 2021-05-09T23:53:46Z

It looks like the structure in this PR is this:

top-level
- julia_version
- project_hash
- deps
  - dependency_1
  - dependency_2
  - dependency_3

How about doing this instead?

top-level
- metadata
  - julia_version
  - project_hash
- deps
  - dependency_1
  - dependency_2
  - dependency_3

That way, if we add more metadata in the future, we don't need to add them as top-level keys.

fredrikekre · 2021-05-10T07:09:50Z

Why not have a toplevel key that can never be a name of a Julia package? For example manifest-metadata or .metadata? That would not require any changes to code loading, and would not be breaking (I think?).

DilumAluthge · 2021-05-10T08:16:36Z

Why not have a toplevel key that can never be a name of a Julia package? For example manifest-metadata or .metadata? That would not require any changes to code loading, and would not be breaking (I think?).

This would still be breaking.

For example, this manifest instantiates just fine on Julia master:

[[Example]]
git-tree-sha1 = "46e44e869b4d90b96bd8ed1fdcf32244fddfb6cc"
uuid = "7876af07-990d-54b4-ab0e-23690620f79a"
version = "0.5.3"

However, if I take this manifest...

[manifest-metadata]
julia_version = "123"

[[Example]]
git-tree-sha1 = "46e44e869b4d90b96bd8ed1fdcf32244fddfb6cc"
uuid = "7876af07-990d-54b4-ab0e-23690620f79a"
version = "0.5.3"

... and I try to instantiate it, I get this error:

julia> Pkg.instantiate()
┌ Error: Could not parse entry for `manifest-metadata`
└ @ Pkg.Types ~/dev/repos/JuliaLang/julia/usr/share/julia/stdlib/v1.7/Pkg/src/manifest.jl:153
ERROR: MethodError: no method matching get(::Pair{String, Any}, ::String, ::Nothing)
Closest candidates are:
  get(::IdDict{K, V}, ::Any, ::Any) where {K, V} at iddict.jl:101
  get(::IOContext, ::Any, ::Any) at show.jl:343
  get(::Dict{K, V}, ::Any, ::Any) where {K, V} at dict.jl:504
  ...
Stacktrace:
 [1] Dict{Base.UUID, Pkg.Types.PackageEntry}(raw::Dict{String, Any})
   @ Pkg.Types ~/dev/repos/JuliaLang/julia/usr/share/julia/stdlib/v1.7/Pkg/src/manifest.jl:141
 [2] read_manifest(f_or_io::String)
   @ Pkg.Types ~/dev/repos/JuliaLang/julia/usr/share/julia/stdlib/v1.7/Pkg/src/manifest.jl:177
 [3] Pkg.Types.EnvCache(env::Nothing)
   @ Pkg.Types ~/dev/repos/JuliaLang/julia/usr/share/julia/stdlib/v1.7/Pkg/src/Types.jl:306
 [4] EnvCache
   @ ~/dev/repos/JuliaLang/julia/usr/share/julia/stdlib/v1.7/Pkg/src/Types.jl:285 [inlined]
 [5] Pkg.Types.Context()
   @ Pkg.Types ./util.jl:471
 [6] #instantiate#278
   @ ~/dev/repos/JuliaLang/julia/usr/share/julia/stdlib/v1.7/Pkg/src/API.jl:1399 [inlined]
 [7] instantiate()
   @ Pkg.API ~/dev/repos/JuliaLang/julia/usr/share/julia/stdlib/v1.7/Pkg/src/API.jl:1399
 [8] top-level scope
   @ REPL[2]:1

Similarly, if I take this manifest...

[".metadata"]
julia_version = "123"

[[Example]]
git-tree-sha1 = "46e44e869b4d90b96bd8ed1fdcf32244fddfb6cc"
uuid = "7876af07-990d-54b4-ab0e-23690620f79a"
version = "0.5.3"

... and I try to instantiate it, I get the same error:

julia> Pkg.instantiate()
┌ Error: Could not parse entry for `.metadata`
└ @ Pkg.Types ~/dev/repos/JuliaLang/julia/usr/share/julia/stdlib/v1.7/Pkg/src/manifest.jl:153
ERROR: MethodError: no method matching get(::Pair{String, Any}, ::String, ::Nothing)
Closest candidates are:
  get(::IdDict{K, V}, ::Any, ::Any) where {K, V} at iddict.jl:101
  get(::IOContext, ::Any, ::Any) at show.jl:343
  get(::Dict{K, V}, ::Any, ::Any) where {K, V} at dict.jl:504
  ...
Stacktrace:
 [1] Dict{Base.UUID, Pkg.Types.PackageEntry}(raw::Dict{String, Any})
   @ Pkg.Types ~/dev/repos/JuliaLang/julia/usr/share/julia/stdlib/v1.7/Pkg/src/manifest.jl:141
 [2] read_manifest(f_or_io::String)
   @ Pkg.Types ~/dev/repos/JuliaLang/julia/usr/share/julia/stdlib/v1.7/Pkg/src/manifest.jl:177
 [3] Pkg.Types.EnvCache(env::Nothing)
   @ Pkg.Types ~/dev/repos/JuliaLang/julia/usr/share/julia/stdlib/v1.7/Pkg/src/Types.jl:306
 [4] EnvCache
   @ ~/dev/repos/JuliaLang/julia/usr/share/julia/stdlib/v1.7/Pkg/src/Types.jl:285 [inlined]
 [5] Pkg.Types.Context()
   @ Pkg.Types ./util.jl:471
 [6] #instantiate#278
   @ ~/dev/repos/JuliaLang/julia/usr/share/julia/stdlib/v1.7/Pkg/src/API.jl:1399 [inlined]
 [7] instantiate()
   @ Pkg.API ~/dev/repos/JuliaLang/julia/usr/share/julia/stdlib/v1.7/Pkg/src/API.jl:1399
 [8] top-level scope
   @ REPL[2]:1

I am pretty sure that all three of those manifests are valid TOML. All three parse just fine using the TOML stdlib, and all three validate when pasted into an online TOML validator.

But the second and third manifests throw errors when you try to Pkg.instantiate() them on Julia master.

IanButterworth · 2021-05-11T02:36:07Z

Tests are passing now (ignore appveyor, only github actions are set to use JuliaLang/julia#40765)

On the format, I think this format makes most sense to me of the things brought up. There was some discussion on slack and I think it's fair to say that the conclusion was that there's no decent way to add the julia version and retain backwards compatibility, so the format might as well be redone to make it more expandable.

cc. @StefanKarpinski @KristofferC

DilumAluthge · 2021-05-11T03:50:12Z

I agree that we should redo the format in a breaking way. I.e. I don't think that there is any way to keep backwards-compatibility.

But I'm still having trouble understanding why julia_version and project_hash are top-level keys. Wouldn't it be a lot cleaner if the only top-level keys are deps and metadata? Then julia_version and project_hash would both go under metadata. And it leaves room in the future to add other pieces of information under metadata.

Is there a specific reason why julia_version and project_hash have to be top-level keys?

DilumAluthge · 2021-05-11T03:55:21Z

E.g. this is what I'm suggesting:

# This file is machine-generated - editing it directly is not advised

[metadata]
julia_version = "1.7.0-DEV.1077"
project_hash = "nothing"

[[deps.Base64]]
uuid = "2a0f44e3-6c83-55bd-87e4-b1978d98bd5f"

[[deps.Distributed]]
deps = ["LinearAlgebra", "Random", "Serialization", "Sockets"]
uuid = "8ba89e20-285c-5b6f-9357-94700520ee1b"

To me personally, it looks a lot cleaner to just have two top-level keys (metadata and deps).

DilumAluthge · 2021-05-11T04:02:39Z

If we really want to be concise, we can make the top-level keys deps and meta, for example:

# This file is machine-generated - editing it directly is not advised

[meta]
julia_version = "1.7.0-DEV.1077"
project_hash = "nothing"

[[deps.Base64]]
uuid = "2a0f44e3-6c83-55bd-87e4-b1978d98bd5f"

[[deps.Distributed]]
deps = ["LinearAlgebra", "Random", "Serialization", "Sockets"]
uuid = "8ba89e20-285c-5b6f-9357-94700520ee1b"

fredrikekre · 2021-05-11T08:27:22Z

But the second and third manifests throw errors when you try to Pkg.instantiate() them on Julia master.

But it wouldn't require changes to code loading

KristofferC · 2021-05-11T16:06:13Z

Why not have a toplevel key that can never be a name of a Julia package?

I personally don't like metadata that is "non-structural", for example by some naming convention. It makes writing code for it very awkward.

KristofferC · 2021-05-11T16:10:24Z

I don't see a reason to add an extra "metadata" key.

I think the first PR should only change things to have deps. section and not do anything extra (not add any extra keys, no warnings etc).

If this is the format we go with, we should probably backport the support for reading it to 1.6 (which is another reason for trying to keep it as minimal as possible).

DilumAluthge · 2021-05-11T23:37:37Z

I don't see a reason to add an extra "metadata" key.

Presumably, in the future, we want to be able to add more metadata, right? Won't it get messy if each new piece of metadata is a top-level key?

DilumAluthge · 2021-05-11T23:39:28Z

If this is the format we go with, we should probably backport the support for reading it to 1.6 (which is another reason for trying to keep it as minimal as possible).

Wouldn't having a top-level metadata key make it easier to backport support? I.e. if there are only two top-level keys (deps and metadata), when we backport support to 1.6, we can just tell 1.6 to completely ignore the metadata key, and just read the dependencies out of deps.

But in contrast, if we have each piece of metadata as a top-level key, then when we backport support to 1.6, we need to teach 1.6 about each individual top-level key and how to handle it.

Whereas if all metadata is under metadata, just tell earlier Julia versions to ignore the single top-level metadata key.

DilumAluthge · 2021-05-11T23:59:59Z

Also, what if in the future we want to add information to the manifest that is neither dependencies nor metadata? E.g. maybe (just for example, I'm not saying we should actually do this) we start putting information in the manifest about the specific versions of artifacts that were downloaded. That information would not go into the deps key. But also, that's not really metadata. It would better fit into its own artifacts top-level key.

If we have separate deps and metadata keys, it leave the door open in the future to add other top-level keys that represent meaningful groupings of data that are neither dependencies nor metadata.

But if we don't have a top-level metadata key, then we are basically saying that every top-level key that is not deps must be interpreted as metadata, and we make it harder for ourselves to make future non-breaking additions to the manifest format.

IanButterworth · 2021-05-12T02:56:00Z

@KristofferC

I think the first PR should only change things to have deps. section and not do anything extra (not add any extra keys, no warnings etc).
If this is the format we go with, we should probably backport the support for reading it to 1.6 (which is another reason for trying to keep it as minimal as possible).

I tried to keep it minimal in #2561

davidanthoff · 2021-05-28T12:28:51Z

This is great!

Has there been some discussion about the behavior if one tries to load a project with a non-matching Julia version? I'm curios because the stuff I've been doing over at https://github.com/JuliaLang/juliaup would make it relatively trivial to have a situation where the launcher from juliaup reads the manifest and then starts the exact (or not) Julia version that was specified in the manifest, see here. But at the moment it is unclear to me what a good design in that space would actually be. The simple solution could be to just always use exactly the Julia version that is recorded in the manifest. But maybe it would actually make more sense to specify a compat bound in Project.toml and then decide which Julia version to use based on that? Or something else?

I think that discussion is potentially orthogonal to the more narrow design of this PR, but at the same time clearly related. If you feel this would derail this PR, we could also discuss my question over at JuliaLang/juliaup#10.

davidanthoff · 2021-05-28T12:42:33Z

Another thought: might it make sense to add another field schemaversion? In case in the future someone wanted to change the format for Manifest.toml again, then there would be a way to record inside the Manifest.toml itself which exact version of the manifest schema a given file follows?

IanButterworth · 2021-05-31T01:12:57Z

This PR became a bit more of a playground, but given the general discussion is here, here's an update..

The actual PRs are:

#2561

#2580

IanButterworth marked this pull request as draft May 9, 2021 05:30

IanButterworth commented May 9, 2021

View reviewed changes

IanButterworth mentioned this pull request May 10, 2021

Add support for new Manifest.toml format during code load JuliaLang/julia#40765

Merged

1 task

IanButterworth force-pushed the ib/new_manifest branch 3 times, most recently from c589dfd to 29908fd Compare May 10, 2021 05:02

IanButterworth force-pushed the ib/new_manifest branch 3 times, most recently from 0f7f779 to 975af0d Compare May 11, 2021 02:01

IanButterworth added 3 commits May 10, 2021 22:18

Restructure manifest format to add top level metadata

a0c49c1

fix project hash func

60fddc3

temporarily limit to 1 CI run

bddd72c

IanButterworth force-pushed the ib/new_manifest branch from 975af0d to bddd72c Compare May 11, 2021 02:19

IanButterworth mentioned this pull request May 12, 2021

New Manifest.toml format: Pt 1. Teach Pkg new format, but default to old format #2561

Merged

3 tasks

DilumAluthge mentioned this pull request Jun 4, 2021

New Manifest.toml format: Pt 2. Default to new format, warn if old format is being maintained #2580

Merged

KristofferC closed this in #2580 Jun 7, 2021

IanButterworth deleted the ib/new_manifest branch December 21, 2022 01:15

Uh oh!

WIP: Implement new manifest format with top-level metadata #2557

WIP: Implement new manifest format with top-level metadata #2557

Uh oh!

Conversation

IanButterworth commented May 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IanButterworth May 9, 2021

Choose a reason for hiding this comment

Uh oh!

DilumAluthge commented May 9, 2021

Uh oh!

fredrikekre commented May 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DilumAluthge commented May 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IanButterworth commented May 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DilumAluthge commented May 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DilumAluthge commented May 11, 2021

Uh oh!

DilumAluthge commented May 11, 2021

Uh oh!

fredrikekre commented May 11, 2021

Uh oh!

KristofferC commented May 11, 2021

Uh oh!

KristofferC commented May 11, 2021

Uh oh!

DilumAluthge commented May 11, 2021

Uh oh!

DilumAluthge commented May 11, 2021

Uh oh!

DilumAluthge commented May 11, 2021

Uh oh!

IanButterworth commented May 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidanthoff commented May 28, 2021

Uh oh!

davidanthoff commented May 28, 2021

Uh oh!

IanButterworth commented May 31, 2021

Uh oh!

Uh oh!

IanButterworth commented May 9, 2021 •

edited

Loading

fredrikekre commented May 10, 2021 •

edited

Loading

DilumAluthge commented May 10, 2021 •

edited

Loading

IanButterworth commented May 11, 2021 •

edited

Loading

DilumAluthge commented May 11, 2021 •

edited

Loading

IanButterworth commented May 12, 2021 •

edited

Loading