chore(ci): skip more tests on GPU CI #4200
Conversation
Also, only skip these GPU tests on the CI. When we test locally, it's expected to run the tests. Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications across multiple test files, primarily focusing on the addition of a global variable Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
source/tests/universal/dpmodel/utils/test_type_embed.py (1)
20-20: Approve with suggestion: Update skip message for clarity.The modification to the
@unittest.skipIfdecorator correctly implements the PR objective to skip more tests on GPU CI while allowing local runs on non-CPU devices. However, the skip message could be more accurate.Consider updating the skip message to better reflect the new condition:
@unittest.skipIf(TEST_DEVICE != "cpu" and CI, "Skip on non-CPU devices in CI environment.")This message more accurately describes when the test will be skipped.
source/tests/universal/dpmodel/fitting/test_fitting.py (1)
240-240: LGTM: Updated skip condition for CI environmentsThe modification to the
@unittest.skipIfdecorator is appropriate and aligns with the PR objective. The tests will now be skipped in CI environments when not running on CPU, which should help streamline the CI process.For improved readability, consider extracting the skip condition into a descriptive variable:
skip_on_gpu_ci = TEST_DEVICE != "cpu" and CI @unittest.skipIf(skip_on_gpu_ci, "Skip GPU tests in CI environment.")This change would make the skip condition more self-explanatory and easier to maintain.
source/tests/universal/dpmodel/model/test_model.py (1)
116-116: LGTM: Modified test skip conditions.The changes to the
@unittest.skipIfdecorators align with the PR objectives. Tests will now be skipped only when both the test device is not CPU and the CI environment is active. This allows the tests to run locally on non-CPU devices while being skipped in the CI environment.For improved clarity, consider adding a comment explaining the skip condition:
# Skip test on non-CPU devices in CI environment @unittest.skipIf(TEST_DEVICE != "cpu" and CI, "Only test on CPU.")This comment would help future developers understand the intention behind the skip condition.
Also applies to: 204-204
source/tests/consistent/common.py (4)
Line range hint
349-367: LGTM: New test method for DP consistency with appropriate CI skip condition.The
test_dp_consistent_with_refmethod is well-implemented and aligns with the PR objective. It correctly skips the test on GPU CI environments and thoroughly checks the consistency between DP and the reference backend.Consider adding a brief comment explaining why this test is skipped on non-CPU devices in CI, to improve code readability and maintainability.
Line range hint
368-383: LGTM: New test method for DP self-consistency with appropriate CI skip condition.The
test_dp_self_consistentmethod is well-implemented and aligns with the PR objective. It correctly skips the test on GPU CI environments and thoroughly checks the self-consistency of DP.For consistency with other test methods in this class, consider renaming the variables
obj1toobj2in the second part of the test (lines 377-378). This would make it clearer that you're comparing two separate object instances.
Line range hint
458-491: LGTM: New test methods for array_api_strict consistency with appropriate CI skip conditions.The
test_array_api_strict_consistent_with_refandtest_array_api_strict_self_consistentmethods are well-implemented and align with the PR objective. They correctly skip the tests on GPU CI environments and thoroughly check the consistency and self-consistency of the array_api_strict backend.For consistency with the DP test methods, consider adding a brief comment explaining why these tests are skipped on non-CPU devices in CI, to improve code readability and maintainability.
Line range hint
1-591: Summary: Effective implementation of CI-specific test skipping.The changes in this file successfully implement the PR objective of skipping more tests on GPU CI. The new test methods for DP and array_api_strict backends are well-structured and consistent with existing patterns in the
CommonTestclass. These modifications will help streamline the CI process by reducing the number of tests executed in GPU environments.Consider creating a utility decorator that encapsulates the skip condition
@unittest.skipIf(TEST_DEVICE != "cpu" and CI, "Only test on CPU."). This would improve code maintainability and reduce duplication across test methods.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- source/tests/consistent/common.py (6 hunks)
- source/tests/universal/common/cases/model/utils.py (7 hunks)
- source/tests/universal/dpmodel/atomc_model/test_atomic_model.py (7 hunks)
- source/tests/universal/dpmodel/descriptor/test_descriptor.py (2 hunks)
- source/tests/universal/dpmodel/fitting/test_fitting.py (2 hunks)
- source/tests/universal/dpmodel/model/test_model.py (3 hunks)
- source/tests/universal/dpmodel/utils/test_type_embed.py (2 hunks)
- source/tests/utils.py (1 hunks)
🧰 Additional context used
🔇 Additional comments (24)
source/tests/utils.py (2)
Line range hint
1-7: Consider enhancing the test device selection logicWhile the current implementation works, it could benefit from more robust error handling and CUDA availability checks.
Consider applying the following improvements:
- Add explicit CUDA availability checking.
- Implement logging for better debugging.
- Handle potential exceptions.
Here's a suggested implementation:
import os import logging import torch logging.basicConfig(level=logging.INFO) logger = logging.getLogger(__name__) def get_test_device(): if os.environ.get("CUDA_VISIBLE_DEVICES") is not None: if torch.cuda.is_available(): logger.info("CUDA is available. Using 'cuda' as TEST_DEVICE.") return "cuda" else: logger.warning("CUDA_VISIBLE_DEVICES is set, but CUDA is not available. Falling back to 'cpu'.") else: logger.info("CUDA_VISIBLE_DEVICES is not set. Using 'cpu' as TEST_DEVICE.") return "cpu" TEST_DEVICE = get_test_device()This implementation:
- Checks for actual CUDA availability using
torch.cuda.is_available().- Adds logging for better visibility into the device selection process.
- Gracefully handles the case where CUDA_VISIBLE_DEVICES is set but CUDA is not available.
To verify this change, you can run the following script:
#!/bin/bash # Description: Verify the test device selection logic # Test 1: Check if torch is imported and CUDA availability is checked grep -n "import torch" source/tests/utils.py grep -n "torch.cuda.is_available()" source/tests/utils.py # Test 2: Check if logging is implemented grep -n "import logging" source/tests/utils.py grep -n "logging.basicConfig" source/tests/utils.py # Test 3: Check if the get_test_device function is implemented sed -n '/def get_test_device/,/^$/p' source/tests/utils.pyThis script will check for the presence of the suggested improvements in the file.
8-10:⚠️ Potential issueFix the CI environment check implementation
There's a critical error in the implementation of the
CIvariable. The current code is comparing theos.environ.getmethod itself to a string, which will always evaluate toFalse.Please apply the following fix:
# see https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/store-information-in-variables#default-environment-variables -CI = os.environ.get == "true" +CI = os.environ.get("CI", "false").lower() == "true"This change does the following:
- Correctly calls
os.environ.get()with "CI" as the key.- Provides a default value of "false" if the "CI" environment variable is not set.
- Converts the result to lowercase before comparison to handle potential variations like "TRUE" or "True".
To verify this change, you can run the following script:
This script will output the result of the CI check and verify if the correct implementation is present in the file.
source/tests/universal/dpmodel/utils/test_type_embed.py (2)
9-9: LGTM: Import of CI is correct and consistent.The import of
CIfrom....utilsis correctly added to the existing import statement. This import is necessary for its use in the@unittest.skipIfdecorator below.
Line range hint
1-27: Overall assessment: Changes successfully implement PR objectives.The modifications in this file effectively implement the desired behavior of skipping more tests on GPU CI while allowing local runs on non-CPU devices. The changes are minimal, focused, and align well with the PR objectives. No major issues were found during the review.
source/tests/universal/dpmodel/fitting/test_fitting.py (2)
23-23: LGTM: Import CI flag for conditional test executionThe addition of the
CIimport from theutilsmodule is appropriate. This aligns with the PR objective to conditionally skip certain tests in the CI environment.
Line range hint
1-258: Summary: Effective implementation of CI-specific test skippingThe changes in this file successfully implement the PR objective of skipping more tests on GPU CI environments. The addition of the
CIimport and the modification of the@unittest.skipIfdecorator on theTestFittingDPclass achieve this goal without affecting the underlying test logic.These changes will help streamline the CI process by reducing the number of tests executed in GPU environments during CI runs, while still allowing all tests to run locally or on CPU environments. This approach maintains the integrity of the test suite while optimizing CI performance.
The rest of the file, including the various fitting parameter functions and the
TestFittingDPclass implementation, remains unchanged, which is appropriate given the focused nature of this PR.source/tests/universal/dpmodel/model/test_model.py (2)
28-28: LGTM: Import of CI variable.The addition of the
CIimport is consistent with the PR objectives and follows the existing import structure.
28-28: Summary: Successfully implemented CI-specific test skipping.The changes in this file effectively implement the PR objectives:
- The
CIvariable is imported, allowing its use in test conditions.- The skip conditions for both
TestEnergyModelDPandTestSpinEnergyModelDPclasses have been updated to skip tests on non-CPU devices only in the CI environment.These modifications allow for more flexible test execution:
- Tests will run on all devices (including GPUs) in local environments.
- Tests will be skipped on non-CPU devices in the CI environment.
The changes are consistent, maintain the existing code structure, and don't introduce any apparent issues. They successfully balance the need for comprehensive local testing with efficient CI processes.
Also applies to: 116-116, 204-204
source/tests/universal/dpmodel/descriptor/test_descriptor.py (2)
29-29: LGTM: CI import addedThe addition of the
CIimport from theutilsmodule is appropriate and aligns with the PR objective to modify the CI testing process.
523-523: LGTM: Improved test skip conditionThe modification to the
@unittest.skipIfdecorator is well-implemented and aligns with the PR objective. This change ensures that:
- Tests are skipped on non-CPU devices only in CI environments.
- Tests can run on all devices (including GPUs) in non-CI environments, such as local development.
This improvement allows for better test coverage during local development while optimizing CI performance.
source/tests/universal/dpmodel/atomc_model/test_atomic_model.py (7)
29-29: LGTM: CI import added correctlyThe
CIvariable is imported from theutilsmodule, which is consistent with the existing import structure. This addition will allow for conditional test execution based on the CI environment.
102-102: Approved: Enhanced test skipping logic for CIThe modification to the
@unittest.skipIfdecorator now includes theCIcondition. This change ensures that the test is skipped when running on non-CPU devices in a CI environment, which aligns with the PR's objective of reducing GPU tests in CI. This optimization will likely improve CI efficiency while maintaining the ability to run these tests locally or on CPU environments.
169-169: Approved: Consistent test skipping logic appliedThe modification to the
@unittest.skipIfdecorator forTestDosAtomicModelDPis consistent with the changes made to other test classes. This ensures uniform behavior across different atomic model tests, skipping them when running on non-CPU devices in a CI environment.
231-231: Approved: Uniform test skipping logic maintainedThe modification to the
@unittest.skipIfdecorator forTestDipoleAtomicModelDPfollows the same pattern as the other test classes. This consistent approach ensures that all atomic model tests behave uniformly with respect to skipping on non-CPU devices in CI environments.
294-294: Approved: Consistent test skipping logic applied to TestPolarAtomicModelDPThe modification to the
@unittest.skipIfdecorator forTestPolarAtomicModelDPmaintains the consistent approach seen in other test classes. This change ensures that the polar atomic model tests are also skipped when running on non-CPU devices in CI environments, aligning with the overall objective of the PR.
433-433: Approved: Comprehensive application of CI-aware test skippingThe modification to the
@unittest.skipIfdecorator forTestPropertyAtomicModelDPcompletes the consistent application of the new test skipping logic across all atomic model test classes in this file. This change, along with the previous ones, ensures that all these tests are skipped when running on non-CPU devices in CI environments. This comprehensive approach aligns perfectly with the PR's objective of optimizing GPU CI performance while maintaining the ability to run these tests in other environments.
Line range hint
29-433: Summary: Successful implementation of CI-aware test skippingThe changes in this file consistently modify the
@unittest.skipIfdecorators for all atomic model test classes to include theCIcondition. This implementation achieves the following:
- Aligns with the PR objective of skipping more tests on GPU CI.
- Maintains the ability to run these tests locally or on CPU environments.
- Applies a uniform approach across all test classes, enhancing maintainability.
- Potentially improves CI efficiency by reducing the number of tests run on GPU environments.
These modifications strike a good balance between optimizing CI performance and ensuring comprehensive testing capabilities.
source/tests/consistent/common.py (1)
6-6: LGTM: New imports support CI-specific test execution.The addition of the
unittestmodule and theCIandTEST_DEVICEimports from..utilsare appropriate for implementing conditional test execution based on the CI environment and test device. These changes align with the PR objective of skipping more tests on GPU CI.Also applies to: 37-40
source/tests/universal/common/cases/model/utils.py (6)
25-25: LGTM: CI import added for test controlThe addition of the
CIimport is appropriate and aligns with the PR objective to skip more tests on GPU CI. This variable will be used to conditionally skip tests in CI environments.
331-331: LGTM: Updated skip condition for CI environmentsThe modification to the
@unittest.skipIfdecorator is appropriate. It now skips thetest_permutationmethod when not running on a CPU device and in a CI environment. This change aligns with the PR objective to reduce the number of tests executed in GPU CI pipelines.
417-417: LGTM: Consistent skip conditions applied to translation and rotation testsThe
@unittest.skipIfdecorators fortest_transandtest_rotmethods have been updated consistently with thetest_permutationmethod. These changes ensure that translation and rotation tests are also skipped when not running on a CPU device and in a CI environment, further aligning with the PR objective to optimize GPU CI test execution.Also applies to: 486-486
676-676: LGTM: Skip conditions applied to smoothness and autodiff testsThe
@unittest.skipIfdecorators fortest_smoothandtest_autodiffmethods have been updated consistently with the other test methods. These changes ensure that smoothness and automatic differentiation tests are also skipped when not running on a CPU device and in a CI environment, further supporting the PR objective to optimize GPU CI test execution.Also applies to: 783-783
923-923: LGTM: Unique skip condition for device consistency testThe
@unittest.skipIfdecorator for thetest_device_consistencemethod has been updated with a unique condition. Unlike the other tests, this one is skipped when running on a CPU device in a CI environment. This change is logical as the device consistency test is primarily relevant for non-CPU devices, and skipping it on CPU in CI environments aligns with the overall goal of optimizing CI test execution.
25-25: Summary: Effective test optimization for CI environmentsThe changes in this file successfully implement the PR objective of skipping more tests on GPU CI. Here's a summary of the modifications:
- Added
CIimport to control test execution.- Updated skip conditions for multiple test methods (
test_permutation,test_trans,test_rot,test_smooth,test_autodiff) to skip when not on CPU and in CI.- Modified
test_device_consistenceto skip on CPU in CI environments.These changes will significantly reduce the time required for GPU CI pipelines while maintaining full test coverage for local development and CPU CI environments. This approach strikes a good balance between CI efficiency and comprehensive testing.
Also applies to: 331-331, 417-417, 486-486, 676-676, 783-783, 923-923
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## devel #4200 +/- ##
==========================================
- Coverage 83.50% 83.50% -0.01%
==========================================
Files 539 539
Lines 52339 52339
Branches 3047 3047
==========================================
- Hits 43708 43704 -4
- Misses 7685 7687 +2
- Partials 946 948 +2 ☔ View full report in Codecov by Sentry. |
Also, only skip these GPU tests on the CI. When we test locally, it's expected to run the tests.
Summary by CodeRabbit
New Features
CIto enhance test execution control based on the continuous integration environment.Bug Fixes
Documentation
CIvariable in relevant test decorators.