Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Updates to build and test
nx-cugraph
wheel as part of CI and nightly workflows #3852Updates to build and test
nx-cugraph
wheel as part of CI and nightly workflows #3852Changes from 19 commits
985e921
9ba4595
d63eb6e
89e9dc9
f3ffacc
64a3d8c
b3c595f
e473c5b
e67a3ee
de14d8d
9412d20
52ffb67
f79f231
0fbddeb
a1d079d
b3faa5d
0e9c754
e084d5d
a662ec7
8d7546e
5adefe8
9f81547
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
There was a case where the wildcard did not match the .whl files on disk and 'echo' simply echo'd the wildcard pattern, which made
pip
generate a misleading error message. Using 'ls' will result in the script erroring out with a clear message about the missing/mis-named files.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.
Odd, but I'm fine with the change if it works.
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 agree with this change, I see the problem with shell wildcard expansion.
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 guess this change is necessary for nightlies? Were nightly tests for arm not running the tests that required datasets?
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 shouldn't remove this, but might need to modify it to only be skipped in ARM PR jobs. In ARM PR jobs, we can only run "smoke tests" due to limited resource capacity.
cugraph/ci/test_wheel.sh
Line 16 in 5c34d3d
Downloading the datasets is time-intensive and should not be performed for "smoke tests" in ARM PR jobs because it wastes a lot of time on the ARM GPU node.
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'm terribly sorry. I read this backwards. This removed downloading datasets for ALL jobs (all arches, nightly/PR). Are we not dependent on datasets for tests to pass anymore?
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.
The
get_test_data.sh
script will download supplemental datasets that are too big to commit to our github repo. Those are currently only used by C++ tests. The python tests use the smaller .csv datasets committed to the repo here, so it's safe to skip downloading when only testing python code.I only removed it here since it was in the proximity of the changes for the wheel builds, but I think an audit of what runs require downloading the supplemental datasets would be good for a separate PR.
Prior to the recent commits to the PR which triggered CI running now, we had everything passing with the download step removed.
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 @rlratzel, I'll approve now.