-
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
Make improvements in pandas-test reporting #15485
Make improvements in pandas-test reporting #15485
Conversation
.github/workflows/test.yaml
Outdated
@@ -130,7 +130,7 @@ jobs: | |||
secrets: inherit | |||
uses: rapidsai/shared-workflows/.github/workflows/[email protected] | |||
with: | |||
matrix_filter: map(select(.ARCH == "amd64")) | group_by(.CUDA_VER|split(".")|map(tonumber)|.[0]) | map(max_by([(.PY_VER|split(".")|map(tonumber)), (.CUDA_VER|split(".")|map(tonumber))])) | |||
matrix_filter: map(select(.ARCH == "amd64")) | group_by(.CUDA_VER|split(".")|map(tonumber)|.[0]) | min(max_by([(.PY_VER|split(".")|map(tonumber)), (.CUDA_VER|split(".")|map(tonumber))])) |
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.
We have the same job in pr.yaml
and here. One is run on nightly and one on pr's, both have different matrix combinations: https://github.com/rapidsai/shared-workflows/blob/13e008c746e30a830c4ebe83a6786862858dfcb8/.github/workflows/wheels-test.yaml#L75-L94
To achieve a consistent matrix for comparison, I changed this to min
so that we compare with cudf-12.2.2
and python 3.9 versions for both jobs.
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.
This is a coincidence that we have the same minimum between the PR matrix and the nightly matrix. Can we find another way to get a common configuration, like using the “latest” image?
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.
Do you think it's okay to hard-code here instead of map(select..
? Again, can we do this as a follow-up?
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.
Sure, feel free to defer on this. Just know that if this workflow breaks when the matrix changes, I’ll come poke you to fix it. 😉
@@ -10,12 +10,13 @@ | |||
GH_JOB_NAME="pandas-tests-diff / build" | |||
rapids-logger "Github job name: ${GH_JOB_NAME}" | |||
|
|||
MAIN_ARTIFACT=$(rapids-s3-path)cuda12_$(arch)_py310.main-results.json | |||
PR_ARTIFACT=$(rapids-s3-path)cuda12_$(arch)_py39.pr-results.json | |||
PY_VER="39" |
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.
same reason as here: https://github.com/rapidsai/cudf/pull/15485/files#r1556345903
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.
We don’t want to hardcode any values here. Our matrix will change over time and break this. Try using ${RAPIDS_PY_VERSION}
.
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.
Can I open a follow-up for this one? I will need to do some testing here after this PR merges and nightlies are uploaded.
…/cudf into pandas-reporting-2
Co-authored-by: Ray Douglass <[email protected]>
/merge |
Description
This PR fixes an issue where
listJobsForWorkflowRun
returns only 30 jobs details by default and we need to paginate and load the rest all of the job details to be able to filter jobs.This PR also address review comments in #15369
Checklist