Skip to content

Call Pkg.resolve() before hashing Manifest #519

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

Merged
merged 4 commits into from
Sep 11, 2024
Merged

Call Pkg.resolve() before hashing Manifest #519

merged 4 commits into from
Sep 11, 2024

Conversation

penelopeysm
Copy link
Member

@penelopeysm penelopeysm commented Sep 9, 2024

Fixes #518.

TLDR;

  • The Manifest file specified Julia 1.10.4.
  • When Julia 1.10.5 was released, CI runs started using that, which meant that the version of the Manifest in the repository didn't match the version of the Manifest used for generating docs. This led to two problems:
    • The GHA cache key uses a hash of the Manifest file, so this caused a lot of GHA cache misses (lots of 2 hr builds for no reason).
    • Potential inconsistency between generated docs and repo.

The edits to preview.yml and publish.yml stop the cache misses.

The new workflow (resolve_manifest.yml) stops the inconsistency (or, at least, lets us know when there is one).

Copy link
Contributor

github-actions bot commented Sep 9, 2024

Preview the changes: https://turinglang.org/docs/pr-previews/519
Please avoid using the search feature and navigation bar in PR previews!

- name: Instantiate Julia environment
run: julia --project=. -e 'using Pkg; Pkg.instantiate()'
run: julia --project=. -e 'using Pkg; Pkg.instantiate(); Pkg.resolve()'
Copy link
Member

@yebai yebai Sep 10, 2024

Choose a reason for hiding this comment

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

On the downside, updating the Manifest.toml file for every PR might cause issues for docs/tutorials that are not changed by PRs. On the positive side, such a policy would ensure tutorials and docs are more actively maintained for new Julia package versions.

Is this a desirable policy that we wish to enforce?

cc @devmotion @mhauru

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, Pkg.resolve() doesn't update packages though; that would be Pkg.update().

I'm sure I'm missing some weird edge case of an edge case 😄 but the only situation I could think of where resolve() actually changes anything is the already edge case where CI runs with a Julia version different from that in the Manifest.

Copy link
Member Author

Choose a reason for hiding this comment

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

(Or if the Manifest is inconsistent with Project.toml, but we have a check for that 🙂)

Copy link
Member

@yebai yebai Sep 11, 2024

Choose a reason for hiding this comment

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

@penelopeysm, you might want to ask in the Julia language workspace to clarify the difference between resolve and update. The docstrings are not super clear in this case:

(@v1.10) pkg> ?resolve
  resolve

  Resolve the project i.e. run package resolution and update the Manifest. This is useful in case the dependencies of
  developed packages have changed causing the current Manifest to be out of sync.

(@v1.10) pkg> ?update
  [up|update] [-p|--project]  [opts] pkg[=uuid] [@version] ...
  [up|update] [-m|--manifest] [opts] pkg[=uuid] [@version] ...

  opts: --major | --minor | --patch | --fixed
        --preserve=<all/direct/none>

  Update pkg within the constraints of the indicated version specifications. These specifications are of the form @1,
  @1.2 or @1.2.3, allowing any version with a prefix that matches, or ranges thereof, such as @1.2-3.4.5. In --project
  mode, package specifications only match project packages, while in --manifest mode they match any manifest package.
  Bound level options force the following packages to be upgraded only within the current major, minor, patch version;
  if the --fixed upgrade level is given, then the following packages will not be upgraded at all.

  After any package updates the project will be precompiled. For more information see pkg> ?precompile.

Copy link
Member Author

@penelopeysm penelopeysm Sep 11, 2024

Choose a reason for hiding this comment

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

Hmm, I found https://discourse.julialang.org/t/what-is-the-difference-between-pkg-resolve-and-pkg-instantiate/77643/2:

resolve will first check that the Manifest.toml is consistent with the Project.tomls of the current project and all its dependencies, updating it if necessary, then instantiate.

update will use the newest possible dependencies (subject to compatibility constraints)

But actually even better, I tested it locally :)

  • If Project.toml has a compat entry of "Foo = 0.5" and Manifest has [email protected], resolve() will fail with an error. We shouldn't run into this case because in this repository there only has one compat entry for Turing itself, and we already check that the Turing version is consistent

  • If Project.toml has no compat entry and Manifest has [email protected], resolve() doesn't do anything, even if there's a newer version available. This means that for all non-Turing deps the version in the Manifest will always be the one used.

Copy link
Member

@yebai yebai left a comment

Choose a reason for hiding this comment

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

Left a comment below on what policy we wish to enforce for docs deps. Otherwise, it looks like a good improvement!

Copy link
Member

@shravanngoswamii shravanngoswamii left a comment

Choose a reason for hiding this comment

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

This really looks good! Thanks, @penelopeysm! No queries from my side.

@yebai yebai merged commit 71a7a60 into master Sep 11, 2024
4 checks passed
@yebai yebai deleted the pysm/pkg-resolve branch September 11, 2024 16:16
github-actions bot added a commit that referenced this pull request Sep 11, 2024
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.

GHA cache key changes between restoring and saving
3 participants