Use HEAD versions in presubmit CI #782

olupton · 2024-05-01T14:37:27Z

Following #770:

This is in preparation for the next stage of work where we will stop pinning versions and go back to running the presubmit on the head of everything.

Flip the switch so that presubmit CIs run bump.sh. Because now the URLREF_XXX build argument default values encode the upstream repository names + default branches, there is no need to duplicate that information in the manifest.yaml committed to this repository.

Leave the workflow for generating patches for Rosetta builds untouched for now, even though there is still some duplication.

The logic for tracking commits of pip-vcs transitive dependencies moves to pip-finalize.sh, which writes the commits used to a new /opt/manifest.d/pip-finalize.yaml. Commits pinned in the "input" manifest file are still respected.

Note that the Dockerfile changes included in #770 cause a new manifest to be written inside the containers, for example here is the file contained in the upstream-pax container from today's nightly (post-#770, pre-this-PR):

# cat /opt/manifest.d/git-clone.yaml
jax:
  url: https://github.com/google/jax.git
  tracking_ref: cbee909a3249fcf6ef9ff2f6708365ea72de29de
  latest_verified_commit: cbee909a3249fcf6ef9ff2f6708365ea72de29de
  mode: git-clone
xla:
  url: https://github.com/openxla/xla.git
  tracking_ref: cba9952b6c91b71adc18ad368ab801d3021ff49f
  latest_verified_commit: cba9952b6c91b71adc18ad368ab801d3021ff49f
  mode: git-clone
flax:
  url: https://github.com/google/flax.git
  tracking_ref: ae5d66d3ad7b54d7bd9544baade1551d4412044a
  latest_verified_commit: ae5d66d3ad7b54d7bd9544baade1551d4412044a
  mode: git-clone
transformer-engine:
  url: https://github.com/NVIDIA/TransformerEngine.git
  tracking_ref: a81786845c6d76c647d690f1148b31d207bffcc2
  latest_verified_commit: a81786845c6d76c647d690f1148b31d207bffcc2
  mode: git-clone
paxml:
  url: https://github.com/google/paxml.git
  tracking_ref: 9e25f0bd23783def8a665ebdb6805d530125b579
  latest_verified_commit: 9e25f0bd23783def8a665ebdb6805d530125b579
  mode: git-clone
praxis:
  url: https://github.com/google/praxis.git
  tracking_ref: 963189fa26647d5d216bb506317fae3d75429996
  latest_verified_commit: 963189fa26647d5d216bb506317fae3d75429996
  mode: git-clone

which can be fed back into the machinery if an old build needs to be reproduced.

…now lives in default values for URLREF_* build arguments

…specified

… the workflow is explicitly launched with it set to false.

ko3n1g

Overall clear to me, was not able to spot issues. Also ran a quick test in a separate branch testing the logic to pin commits for reproducing older builds. LGTM.

I just think - as mentioned below - that some more documentation about manifest.yaml would be helpful. Especially now where it became more sparse it is difficult to learn by examples.

ko3n1g · 2024-05-02T06:17:01Z

.github/workflows/ci.yaml

@@ -155,15 +159,15 @@ jobs:
          # converts manifest yaml to a json object of {SOFTWARE_NAME: URL#REF, ...}
          urlrefs=$(
            cat .github/container/manifest.yaml |\
-            yq -o=json 'to_entries | .[] | select(.value.mode == "git-clone") | {( .key | upcase | sub("-", "_") ): .value.url + "#" + .value.latest_verified_commit}' |\
+            yq -o=json 'to_entries | .[] | select(.value.mode == "git-clone") | {( .key | upcase | sub("-", "_") ): .value.url + "#" + .value.commit}' |\


It was a bit suprising to me, that I had to add mode: git-clone in order to pull a non-HEAD commit until I found this line. Maybe some documentation about manifest.yaml would be helpful at some point but no high prio as most devs here are familiar with its mechanics I suppose

I'm not sure if the mode is still needed at all. But needing better documentation is an evergreen comment...

ko3n1g · 2024-05-02T06:46:58Z

.github/container/pip-finalize.sh

-  echo "${line}@${ref}"
+# Find the VCS installs, which are of the form
+# PACKAGE @ git+GIT_REPO_URL
+for line in $(sed -n -e 's/^\([^#].*\) @ git+\(.*\)$/\1=\2/p' requirements.pre); do


Why did the regex expression change? I do understand the logic afterwards (19:27) but just not how records in requirements.pre differ such that the regrex matcher needs to be changed.

Before: it was just extracting PACKAGE. Now: it's extracting the GIT_REPO_URL as well.
This is because in the case that the package is not listed in the manifest file, we need the URL to query the current HEAD.

It is still possible to manually launch the pipeline without the bump. This is following on from #770. Further simplification will be possible. #782 has become too large, this is the minimal change to make the presubmit CI behave like the nightlies.

ko3n1g

Sorry for the late notice, but there is a small issue around feeding URLREFs into the build-args. See down below for an example/explanation.

ko3n1g · 2024-05-02T21:54:47Z

.github/workflows/ci.yaml

          # SOURCE_OVERRIDES is a comma-separated list of package=urlref pairs
          IFS=, read -ra overrides <<< "${{ inputs.SOURCE_OVERRIDES }}"
          for override in "${overrides[@]}"; do
            PACKAGE=$(cut -d= -f 1 <<< "${override}" | tr '[:lower:]' '[:upper:]' | tr '-' '_')
            URLREF=$(cut -d= -f 2- <<< "${override}")
-            urlrefs=$(echo "$urlrefs" | jq -c ". + {\"$PACKAGE\": \"$URLREF\"}")
+            urlrefs=$(echo "$urlrefs" | jq -c ". + {\"$PACKAGE\": \"URLREF_${PACKAGE}=$URLREF\"}")


Hey @olupton , can you walk me through the idea of how manifest.yaml and SOURCE_OVERRIDES are meant to be used together?

If I want to install FLAX on an older commit, I have two options: I can change the manifest.yaml, or use SOURCE_OVERRIDES. When is it recommended to use what?

I think the idea here is that there are two modes.

First, when you're using this machinery to test some feature or fix, you don't particularly want to commit/push to JAX-Toolbox, so you can inject your feature/fix branch using SOURCE_OVERRIDES, URLREF_XXX, etc.

Second, when you are preparing a release, you gradually want to converge on a fixed and reproducible set of versions. In that case, you would take some seed manifest.yaml output (perhaps from a nightly) and commit it to a release branch of JAX-Toolbox, and then iterate from there.

Ultimately I don't see why you would want to mix the two modes, and the second one ought to be pedantic. In mode one, you default to taking the HEAD and recording what that was. In mode two, forgetting to specify a version of something should maybe be an error.

ko3n1g · 2024-05-02T21:56:49Z

.github/workflows/ci.yaml

          echo "SOURCE_URLREFS=${urlrefs}" >> $GITHUB_OUTPUT



urlrefs is currently just a mapping of $PACKAGE: $URL which we feed into the build-args. However, when we feed them into the build-args, the actual build-arg-key is missing. The mapping should be $PACKAGE: URLREF_$PACKAGE=$PACKAGE

DwarKapex · 2024-05-16T18:36:32Z

.github/container/manifest.yaml

@@ -1,32 +1,14 @@
-jax:


Correct me if I'm wrong, but you suggest to use default values for jax ref#commit from Dockerfile rather than from manifest file?
What is the scenario for NGC release? Update Dockerfiles accordingly?

Definitely yes for the normal CI: by default we take the Dockerfile defaults, which is basically "HEAD".

When preparing releases, one option (noted here: #782 (comment)) is to commit a manifest file to the release branch and use that. Your suggestion of changing the default values in the Docker files in the release branches would also work, I think, and would require less machinery (to translate manifest --> URLREF build arg) but might be a little more tedious to seed at the beginning of the release process.

cc: @terrykong @ko3n1g

DwarKapex · 2024-05-16T18:44:47Z

LGTM

nouiz · 2024-05-17T15:35:46Z

This is in preparation for the next stage of work where we will stop pinning versions and go back to running the presubmit on the head of everything.

Why this change? It was a request to have PR be tested with fixed dependency. Why change this now?

olupton · 2024-05-17T15:49:12Z

This is in preparation for the next stage of work where we will stop pinning versions and go back to running the presubmit on the head of everything.

Why this change? It was a request to have PR be tested with fixed dependency. Why change this now?

Because it didn't work. The previous architecture relied on the same commit of JAX-Toolbox being able to build both the nightlies and the "last known good" state of pinned versions. In reality, those two states of the world did not track closely enough to one another for that requirement to be viable.

It was annoying in other ways too, but I think the above is the fundamental point.

olupton added 11 commits May 1, 2024 16:27

presubmit: bump manifest

b73aaaf

manifest.yaml: remove patch-less git-clone entries; this information …

9e4557a

…now lives in default values for URLREF_* build arguments

pip-constraint: remove example of unsupported feature

ca30d1f

latest_verified_commit: remove values from repo, rename to commit

7556a3a

URLREF_X: do not set to empty string, overriding the default, if not …

3e8f56c

…specified

latest_verified_commit: missing one

b148ef8

remove mode=git-clone

f97c1ca

Fixup BUMP_MANIFEST logic; this should always be true apart from when…

98f4ea7

… the workflow is explicitly launched with it set to false.

fixup

f687a7f

pip-finalize

95532c2

drop dead code

8783894

terrykong requested a review from ko3n1g May 1, 2024 18:11

ko3n1g approved these changes May 2, 2024

View reviewed changes

olupton mentioned this pull request May 2, 2024

Bump manifest in presubmit CI. #791

Merged

ko3n1g requested changes May 2, 2024

View reviewed changes

fix: Extract URLREF from manifest.yaml

82d16c0

DwarKapex self-requested a review May 16, 2024 15:44

DwarKapex reviewed May 16, 2024

View reviewed changes

olupton closed this Oct 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use HEAD versions in presubmit CI #782

Use HEAD versions in presubmit CI #782

olupton commented May 1, 2024 •

edited

Loading

ko3n1g left a comment

ko3n1g May 2, 2024

olupton May 2, 2024

ko3n1g May 2, 2024

olupton May 2, 2024

ko3n1g left a comment

ko3n1g May 2, 2024

olupton May 15, 2024

ko3n1g May 2, 2024

DwarKapex May 16, 2024

olupton May 17, 2024

DwarKapex commented May 16, 2024

nouiz commented May 17, 2024

olupton commented May 17, 2024

Use HEAD versions in presubmit CI #782

Use HEAD versions in presubmit CI #782

Conversation

olupton commented May 1, 2024 • edited Loading

ko3n1g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ko3n1g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DwarKapex commented May 16, 2024

nouiz commented May 17, 2024

olupton commented May 17, 2024

olupton commented May 1, 2024 •

edited

Loading