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

CI: Validate images across platforms #130

Merged
merged 2 commits into from
Mar 21, 2023
Merged

Conversation

victorlin
Copy link
Member

@victorlin victorlin commented Feb 28, 2023

Description of proposed changes

Start off by checking software in which versions are known to be time-varying.

Related issue(s)

Testing

  • Checks pass

Tested locally:

  1. With a tag known to have differing Augur versions.

    Commands and output
    % ./devel/validate-platforms -r docker.io -t build-20230217T192833Z
    [linux/amd64] Pulling image...
    docker.io/nextstrain/base:build-20230217T192833Z
    [linux/amd64] Checking that the platform is expected...
    [linux/amd64] Generating report file: /var/folders/rk/s2p6nv1d13lcf0dynjkvxyl80000gp/T/validate-platforms-XXXXXX.WEPtnBnz/linux/amd64
    [linux/amd64] Determining software versions...
    [linux/arm64] Pulling image...
    docker.io/nextstrain/base:build-20230217T192833Z
    [linux/arm64] Checking that the platform is expected...
    [linux/arm64] Generating report file: /var/folders/rk/s2p6nv1d13lcf0dynjkvxyl80000gp/T/validate-platforms-XXXXXX.WEPtnBnz/linux/arm64
    [linux/arm64] Determining software versions...
    The report for linux/amd64 has the following contents:
    $ nextstrain --version
    nextstrain.cli 6.1.0.post1
    $ nextalign --version
    nextalign 2.11.0
    $ nextclade --version
    nextclade 2.11.0
    $ augur --version
    augur 21.0.0
    $ auspice --version
    2.43.0
    $ python3 -c "from importlib.metadata import version; print(version(\"evofr\"))"
    0.1.16
    $ datasets --version
    datasets version: 14.13.0
    $ dataformat version
    14.13.0
    $ python3 -c "from importlib.metadata import version; print(version(\"phylo-treetime\"))"
    0.9.5
    Comparing against other platforms...
    --- linux/amd64	2023-03-10 12:13:04.000000000 -0800
    +++ linux/arm64	2023-03-10 12:13:11.000000000 -0800
    @@ -7,3 +7,3 @@
     $ augur --version
    -augur 21.0.0
    +augur 21.0.1
     $ auspice --version
    Failure!
    % echo $?
    1
    
  2. With a tag known to have the same Augur version.

    Commands and output
    % ./devel/validate-platforms -r docker.io -t build-20230220T161818Z
    [linux/amd64] Pulling image...
    docker.io/nextstrain/base:build-20230220T161818Z
    [linux/amd64] Checking that the platform is expected...
    [linux/amd64] Generating report file: /var/folders/rk/s2p6nv1d13lcf0dynjkvxyl80000gp/T/validate-platforms-XXXXXX.OHXS3kXH/linux/amd64
    [linux/amd64] Determining software versions...
    [linux/arm64] Pulling image...
    docker.io/nextstrain/base:build-20230220T161818Z
    [linux/arm64] Checking that the platform is expected...
    [linux/arm64] Generating report file: /var/folders/rk/s2p6nv1d13lcf0dynjkvxyl80000gp/T/validate-platforms-XXXXXX.OHXS3kXH/linux/arm64
    [linux/arm64] Determining software versions...
    The report for linux/amd64 has the following contents:
    $ nextstrain --version
    nextstrain.cli 6.1.0.post1
    $ nextalign --version
    nextalign 2.11.0
    $ nextclade --version
    nextclade 2.11.0
    $ augur --version
    augur 21.0.1
    $ auspice --version
    2.43.0
    $ python3 -c "from importlib.metadata import version; print(version(\"evofr\"))"
    0.1.16
    $ datasets --version
    datasets version: 14.13.0
    $ dataformat version
    14.13.0
    $ python3 -c "from importlib.metadata import version; print(version(\"phylo-treetime\"))"
    0.9.5
    Comparing against other platforms...
    Success!  All versions the same.
    % echo $?
    0
    

Post-merge

@victorlin victorlin self-assigned this Feb 28, 2023
@victorlin victorlin force-pushed the victorlin/validate-platforms branch 4 times, most recently from fddedea to 2646dac Compare March 2, 2023 00:16
@victorlin victorlin marked this pull request as ready for review March 2, 2023 18:17
@victorlin victorlin requested a review from a team March 2, 2023 18:17
Copy link
Member

@tsibley tsibley left a comment

Choose a reason for hiding this comment

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

I think it'd be more useful to take a different approach when comparing the versions. Instead of constructing an array for each component and comparing the contents individually, how about constructing a single text file report for each platform and then diff-ing them? Combined with meaningful report file names, this would provide more readily interpretable output and also provide all version differences at once instead of stopping at the first mismatch. The code would also be quite a bit reduced and simpler.

Thoughts?

@victorlin
Copy link
Member Author

@tsibley nice, that sounds like a better approach! I'll update this PR with that.

@victorlin victorlin force-pushed the victorlin/validate-platforms branch from 2646dac to ae982d4 Compare March 6, 2023 19:25
@tsibley
Copy link
Member

tsibley commented Mar 6, 2023

@victorlin Sounds good. I tweaked what was there earlier this morning into this while tinkering with it:

diff --git a/devel/validate-platforms b/devel/validate-platforms
index 804434c..6fefc85 100755
--- a/devel/validate-platforms
+++ b/devel/validate-platforms
@@ -18,23 +18,18 @@ while getopts "r:t:" opt; do
 done
 
 IMAGE="$registry/nextstrain/base:$tag"
+PLATFORMS=(linux/amd64 linux/arm64)
 
 main() {
-    compare-versions
-}
-
-compare-versions() {
     # Check software where downloaded versions are not deterministic at
     # different points in time (e.g. if "latest" is downloaded).
+    local output
+    output="$(mktemp -dt "$(basename "$0")"-XXXXXX)"
 
-    nextclade_versions=()
-    augur_versions=()
-    auspice_versions=()
-    evofr_versions=()
-    datasets_versions=()
-    dataformat_versions=()
+    for platform in "${PLATFORMS[@]}"; do
+        # Ensure slashes in platforms are subdirs
+        mkdir -p "$(dirname "$output/$platform")"
 
-    for platform in linux/amd64 linux/arm64; do
         echo "[$platform] Pulling image..."
         docker pull -q --platform "$platform" "$IMAGE"
 
@@ -42,59 +37,27 @@ compare-versions() {
         check-platform "$platform"
 
         echo "[$platform] Determining software versions..."
-        nextclade_versions+=("$(run-command "$platform" nextclade --version)")
-        augur_versions+=("$(run-command "$platform" augur --version)")
-        auspice_versions+=("$(run-command "$platform" auspice --version)")
-        evofr_versions+=("$(run-command "$platform" pip show evofr)")
-        datasets_versions+=("$(run-command "$platform" datasets --version)")
-        dataformat_versions+=("$(run-command "$platform" dataformat version)")
+        run-command "$platform" bash -c '
+            PS4="\$ "
+            set -x
+
+            nextclade --version
+            augur --version
+            auspice --version
+            pip show evofr
+            datasets --version
+            dataformat version
+        ' >"$output/$platform" 2>&1
+        cat "$output/$platform"
     done
 
-    echo "Comparing Nextclade versions..."
-    if different-values "${nextclade_versions[@]}"; then
-        echo "Nextclade versions differ!" 1>&2; exit 1
+    echo "Diffing versions..."
+    if cd "$output" && diff --unified --from-file="${PLATFORMS[0]}" "${PLATFORMS[@]:1}"; then
+        echo "Success!  All versions the same." >&2
+    else
+        echo "Failure!" >&2
+        exit 1
     fi
-
-    echo "Comparing Augur versions..."
-    if different-values "${augur_versions[@]}"; then
-        echo "Augur versions differ!" 1>&2; exit 1
-    fi
-
-    echo "Comparing Auspice versions..."
-    if different-values "${auspice_versions[@]}"; then
-        echo "Auspice versions differ!" 1>&2; exit 1
-    fi
-
-    echo "Comparing evofr versions..."
-    if different-values "${evofr_versions[@]}"; then
-        echo "evofr versions differ!" 1>&2; exit 1
-    fi
-
-    echo "Comparing NCBI Datasets (datasets) versions..."
-    if different-values "${datasets_versions[@]}"; then
-        echo "NCBI Datasets (datasets) versions differ!" 1>&2; exit 1
-    fi
-
-    echo "Comparing NCBI Datasets (dataformat) versions..."
-    if different-values "${dataformat_versions[@]}"; then
-        echo "NCBI Datasets (dataformat) versions differ!" 1>&2; exit 1
-    fi
-
-    echo "Success!"
-}
-
-different-values() {
-    local values=("$@")
-
-    first_value="${values[0]}"
-    for value in "${values[@]}"; do
-        if [[ "$first_value" != "$value" ]]; then
-            echo "Values \"$first_value\" and \"$value\" are different." 1>&2
-            return 0 # true
-        fi
-    done
-    echo "Values are all \"$first_value\"."
-    return 1 # false
 }
 
 check-platform() {

@victorlin
Copy link
Member Author

victorlin commented Mar 6, 2023

@tsibley great, I'm going to use some of that.

Also, I just noticed that the OS I'm using (macOS Ventura) comes with a version of diff that doesn't support --from-file:

% diff --version
Apple diff (based on FreeBSD diff)
% diff --from-file
diff: unrecognized option `--from-file'

Apparently this is new as of macOS Ventura. Bummer.

EDIT: I installed GNU diff via Homebrew's diffutils, much better.

@victorlin victorlin force-pushed the victorlin/validate-platforms branch from ae982d4 to 468cb91 Compare March 6, 2023 20:25
@corneliusroemer
Copy link
Member

corneliusroemer commented Mar 6, 2023

EDIT: I installed GNU diff via Homebrew's diffutils, much better.

Ha, I must have already installed gnu diff a while ago since I never got that error. The way to go would be to run the script in a docker container 🙃 Though I'm not sure it's possible to run docker in docker.

@victorlin
Copy link
Member Author

@corneliusroemer I imagine this will mostly be run on the GitHub Actions runner, which should continue to have GNU diff. However, it would be good to consider local dev on common OS's (latest macOS included), so for now I added a note (rather than automatically supporting 2 versions of diff):

# NOTE: if running on macOS ≥13, you may need to install GNU diff for the
# --from-file option.

devel/validate-platforms Show resolved Hide resolved
devel/validate-platforms Outdated Show resolved Hide resolved
@corneliusroemer
Copy link
Member

I imagine this will mostly be run on the GitHub Actions runner, which should continue to have GNU diff. However, it would be good to consider local dev on common OS's (latest macOS included), so for now I added a note (rather than automatically supporting 2 versions of diff):

Great! Re local dev experience, that's why I'd suggest changing default to point at public docker registry. It should work out of the box. Could be useful for debugging etc. Do you have a docker registry set up locally or why use localhost:5000 as default for the registry?

devel/validate-platforms Show resolved Hide resolved
devel/validate-platforms Show resolved Hide resolved
devel/validate-platforms Outdated Show resolved Hide resolved
devel/validate-platforms Outdated Show resolved Hide resolved
devel/validate-platforms Outdated Show resolved Hide resolved
@tsibley
Copy link
Member

tsibley commented Mar 6, 2023

Apparently this is new as of macOS Ventura. Bummer.

Ha. Burned by OS versions, because I'd actually checked that diff on blab-mac had --from-file and was surprised to find it was GNU (but from 2002, presumably due to licensing issues like bash).

@corneliusroemer
Copy link
Member

Let's merge this sooner than later? I don't see it doing any harm merged as is. If it fails, we fix it.

@victorlin victorlin force-pushed the victorlin/validate-platforms branch from c70aa6c to 7d8e948 Compare March 10, 2023 20:14
devel/validate-platforms Outdated Show resolved Hide resolved
devel/validate-platforms Outdated Show resolved Hide resolved
@victorlin victorlin force-pushed the victorlin/validate-platforms branch from 7d8e948 to b8135db Compare March 20, 2023 20:24
victorlin and others added 2 commits March 21, 2023 11:51
Start off by checking software in which versions are known to be
time-varying.

Co-authored-by: Thomas Sibley <[email protected]>
This allows errors such as "command not found" to terminate the script.

Other options which were considered:

1. Set BASH_XTRACEFD to separate lines from `set -x`. Did not use this
   because the option isn't available on bash <4.0, which is where the
   default bash version of macOS is tied to indefinitely.
2. Use trap but without the intermediate function. This gets a bit ugly
   when used inside the literal string due to the need to escape quotes.
@victorlin victorlin force-pushed the victorlin/validate-platforms branch from b8135db to 1856139 Compare March 21, 2023 18:51
@victorlin victorlin merged commit b8ce22b into master Mar 21, 2023
@victorlin victorlin deleted the victorlin/validate-platforms branch March 21, 2023 20:37
@victorlin
Copy link
Member Author

Noting here that the script did it's job in this CI run where NCBI datasets 15.6.0 was released between the time it was fetched in the linux/amd64 build and the time it was fetched in the linux/arm64 build.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

3 participants