-
Notifications
You must be signed in to change notification settings - Fork 915
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
[REVIEW] Fix branch version in CI scripts #13029
Conversation
@@ -98,5 +98,5 @@ jobs: | |||
sha: ${{ inputs.sha }} | |||
package-name: dask_cudf | |||
# Test against latest dask/distributed/dask-cuda. | |||
test-before: "pip install git+https://github.com/dask/dask.git@main git+https://github.com/dask/distributed.git@main git+https://github.com/rapidsai/dask-cuda.git@branch-23.04" | |||
test-before: "pip install git+https://github.com/dask/dask.git@main git+https://github.com/dask/distributed.git@main git+https://github.com/rapidsai/dask-cuda.git@branch-23.06" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this automated correctly? It looks like line 104 of update-version.sh
should've fixed this. Here's raft's code, for comparison:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is automated correctly in cudf:
https://github.com/rapidsai/cudf/blob/branch-23.04/ci/release/update-version.sh#L101
I verified running it locally and the script did the job locally and should've upgraded this automatically in CI too but looks like somehow it didn't change this value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this is a BSD sed vs GNU sed issue. When I run this script during burndown, I use my Mac which uses BSD sed and it misses this change. When I updated to gsed
in the update-version.sh
script it worked as expected.
Let me think about how to handle this in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @raydouglass 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume (though cannot confirm since I don't use a Mac) that the issue with that expression lies within this string: branch-[^\"\s]\+
Therefore we can probably just change this:
for FILE in .github/workflows/*.yaml; do
sed_runner "s/dask-cuda.git@branch-[^\"\s]\+/dask-cuda.git@branch-${NEXT_SHORT_TAG}/g" ${FILE};
done
to this:
for FILE in .github/workflows/*.yaml; do
sed_runner "s/dask-cuda.git@branch-[0-9][0-9].[0-9][0-9]/dask-cuda.git@branch-${NEXT_SHORT_TAG}/g" ${FILE};
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/merge |
Description
This PR fixes missed version upgrades in scripts.
Checklist