-
Notifications
You must be signed in to change notification settings - Fork 82
test: use ParallelTestRunner #1650
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
Conversation
Summary of ChangesHello @avik-pal, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request undertakes a significant refactoring of the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request refactors the LuxCore test suite to use ParallelTestRunner. This is a great improvement that splits a large test file into smaller, more focused files, which should improve maintainability and allow for parallel test execution. During the review, I noticed a couple of pre-existing bugs in the test logic that were carried over into the new files. I've provided suggestions to fix these issues.
Benchmark Results (Julia v1.12)Time benchmarks
Memory benchmarks
|
5b00ad8 to
3db909e
Compare
9a6ddec to
26359fd
Compare
|
/gemini review |
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.
Code Review
This pull request refactors the testing infrastructure to use ParallelTestRunner across LuxCore, MLDataDevices, and WeightInitializers. This involves splitting large test files into smaller, more manageable ones, and updating the runtests.jl files to use the new runner. The CI configuration in Buildkite is also updated.
My main feedback is regarding the Buildkite pipeline configuration, where the updated path triggers seem to miss the test directories within the sub-packages, which could prevent CI from running when only tests are modified.
On a positive note, this refactoring also implicitly fixes a bug in the Chain2 test helper within LuxCore's tests.
dff9411 to
f5d89f8
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1650 +/- ##
==========================================
- Coverage 76.79% 75.71% -1.08%
==========================================
Files 174 174
Lines 7219 7211 -8
==========================================
- Hits 5544 5460 -84
- Misses 1675 1751 +76 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
b1f4723 to
4ed5a37
Compare
4ed5a37 to
d116e6c
Compare
4abe175 to
4bf9e68
Compare
No description provided.