[CI] Enable Java test in CI workflow #805
Conversation
|
@rhdong could you please put this PR into draft until you're ready for reviews? That'd reduce the notifications reviewers are getting, and help them understand when it's time to come review. |
Thanks for the reminder! I’ve marked the PR as draft now. |
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
/ok to test |
|
/ok to test |
|
/ok to test |
@chatman thank you! Done the patch! |
|
/ok to test 0f90041 |
There was a problem hiding this comment.
I left some suggestions for your consideration. There are enough of them that I'm leaving a blocking review... please @ me when this is ready for another review.
If you'd prefer that I just push these changes directly to the PR instead, please let me know and I'd be happy to.
Also (not blocking this PR), I think it would be very helpful to reduce the duplication of the Java version across these builds. The assumption of Java 22 is hard-coded in many places right now, and it'd be easy to accidentally miss mentions like this during updates:
If you agree with me about that, that could be a followup PR.
Sure, thank you so much for your suggestions, which are super helpful! For this time, I will submit by myself to avoid duplicate commits, and pls feel free to push directly in next round. |
|
/ok to test 88165ad |
Hi @chatman , per James’s comment regarding the Java version, do you have any suggestions or guidance on what the best practice typically is in this case? Thanks! |
ci/build_java.sh
Outdated
| export CMAKE_GENERATOR=Ninja | ||
|
|
There was a problem hiding this comment.
| export CMAKE_GENERATOR=Ninja |
This can probably be removed here as well, as it was in ci/test_java.sh as part of #805 (comment).
And I see another issue... the change to add libraft and NCCL was not also applied to the test_java group in dependency-file-generator: #805 (comment).
These types of issues are direct result of the high amount of duplication here... I think we should consolidate things.
test_java.sh only differs from build_java.sh in 2 ways:
- passes
--run-java-teststojava/build.sh - uses the
test_javadependency group forrapids-dependency-file-generator(which would be identical to thejavagroup there iflibraftand NCCL were correctly added)
I think we should do the following:
- remove
test_javagroup independencies.yaml - consolidate all of the duplicated stuff in
build_java.shandtest_java.sh- for example, by having
build_java.shaccept a--run-java-testsargument - that would also ensure
build_java.shis getting tested on PRs (currently it'd only run on nightly builds)
- for example, by having
Could I push changes like that to this branch?
There was a problem hiding this comment.
I've pushed these changes in 1303422
Let's see how CI goes.
|
/ok to test 1303422 |
|
/ok to test 53f5002 |
There was a problem hiding this comment.
From my perspective, this is ready to merge! The stuff I mentioned about reducing hard-coding of the Java version could come in a later PR (or not at all, if you choose not to pursue it).
Just check the conda-java-tests logs to be sure the tests are running after the most recent fix, otherwise I am ok with merging any time.
|
/ok to test e69d623 |
|
/merge |
…only modify update-version.sh (#875) While reviewing #805, I found an issue with this project's `update-version.sh`... it refers to a script `examples/cmake/thirdparty/fetch_rapids.cmake` which no long exists (removed in #824). This proposes the following: * removing that reference from `update-version.sh` * skipping most CI on PRs that only modify `update-version.sh` ## Notes for Reviewers `ci/release/update-version.sh` is standardized (same filepath, same usage) across almost all RAPIDS repos. I cannot think of a situation where a PR that only changes that file would need to have any CI re-run (other than linting, e.g. for `shellcheck`). If folks agree, I'll roll out a change like that more broadly across RAPIDS. Authors: - James Lamb (https://github.com/jameslamb) - Ben Frederickson (https://github.com/benfred) Approvers: - Ben Frederickson (https://github.com/benfred) - Corey J. Nolet (https://github.com/cjnolet) - Gil Forsyth (https://github.com/gforsyth) URL: #875
This PR adds changes for Java CI.
Some scripts modified here also appear in PR #831. Once 831 is merged, I’ll rebase and make sure everything stays consistent.