Skip to content

Conversation

@HydrogenSulfate
Copy link
Collaborator

@HydrogenSulfate HydrogenSulfate commented Dec 22, 2025

This pull request refactors several test files to update import paths for the expand_sys_str utility and related modules, reflecting a migration away from the deepmd.tf namespace. Additionally, it temporarily skips a test due to compatibility issues with numpy 2.4 and improves file cleanup logic in one test. The main themes are import path updates and test maintenance.

Import path updates:

  • Updated imports of expand_sys_str in multiple test files (test_descriptor.py, test_embedding_net.py, test_model.py, test_saveload_dpa1.py, test_saveload_se_e2_a.py, test_sampler.py) to use deepmd.common instead of deepmd.tf.common. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11]
  • Changed import of DeepmdDataSystem in test_sampler.py from deepmd.tf.utils.data_system to deepmd.utils.data_system.

Test maintenance and compatibility:

  • Temporarily skipped the test_sampler_debug_info test in test_sampler.py due to issues with numpy 2.4.
  • Enhanced file cleanup in test_training.py by checking for file existence before attempting to remove files or directories.

Summary by CodeRabbit

  • Tests

    • Improved test robustness by adding existence checks before file cleanup operations to prevent errors.
    • Temporarily skipped one test due to numpy compatibility issues.
  • Chores

    • Reorganized internal module imports for improved code structure.

✏️ Tip: You can customize this high-level summary in your review settings.

Related PR: PaddlePaddle/Paddle#77013

Copilot AI review requested due to automatic review settings December 22, 2025 03:37
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 22, 2025

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

📝 Walkthrough

Walkthrough

The changes relocate the import of expand_sys_str from deepmd.tf.common to deepmd.common across multiple test files, update the import source of DeepmdDataSystem in one test file, add a skip decorator to a test method, and introduce existence checks before file deletion operations in test cleanup.

Changes

Cohort / File(s) Summary
Import path relocation for expand_sys_str
source/tests/pd/model/test_descriptor.py, source/tests/pd/model/test_embedding_net.py, source/tests/pd/model/test_model.py, source/tests/pd/model/test_saveload_dpa1.py, source/tests/pd/model/test_saveload_se_e2_a.py
Moved import of expand_sys_str from deepmd.tf.common to deepmd.common across all files. No functional logic changes.
Multiple import changes and test skip
source/tests/pd/test_sampler.py
Relocated expand_sys_str import from deepmd.tf.common to deepmd.common; updated DeepmdDataSystem import from tf-related module to deepmd.utils.data_system; added unconditional skip decorator to test_sampler_debug_info test method.
File deletion guard checks
source/tests/pd/test_training.py
Added os.path.exists() checks in tearDown before removal operations on model files, lcurve.out, and stat files to prevent FileNotFoundError.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Verify that deepmd.common correctly exports expand_sys_str and that the import relocation is valid across all affected test files
  • Confirm that deepmd.utils.data_system provides DeepmdDataSystem and that the new import path is correct
  • Review the rationale for unconditionally skipping test_sampler_debug_info (related to numpy changes mentioned in comments)

Suggested labels

Python

Suggested reviewers

  • njzjz
  • wanghan-iapcm
  • iProzd

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title check ❓ Inconclusive The title 'fix(pd): fix unitest' is vague and generic, using non-descriptive language that doesn't convey the actual changes (import path migration, test maintenance). Revise to be more specific and descriptive, such as 'fix(pd): migrate test imports from tf.common to common' or 'fix(pd): update test imports and improve test maintenance'.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors test files to migrate import paths from the legacy deepmd.tf namespace to the common deepmd.common and deepmd.utils namespaces. It also includes test maintenance improvements, including a temporary test skip for numpy 2.4 compatibility and enhanced file cleanup logic.

Key changes:

  • Updated expand_sys_str imports from deepmd.tf.common to deepmd.common across 6 test files
  • Updated DeepmdDataSystem import from deepmd.tf.utils.data_system to deepmd.utils.data_system
  • Added existence checks before file removal operations in test teardown to prevent potential errors

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
source/tests/pd/test_training.py Enhanced tearDown method with existence checks before removing files/directories
source/tests/pd/test_sampler.py Updated imports to use common namespace and temporarily skipped test for numpy 2.4 compatibility
source/tests/pd/model/test_saveload_se_e2_a.py Migrated expand_sys_str import from deepmd.tf.common to deepmd.common
source/tests/pd/model/test_saveload_dpa1.py Migrated expand_sys_str import from deepmd.tf.common to deepmd.common
source/tests/pd/model/test_model.py Migrated expand_sys_str import from deepmd.tf.common to deepmd.common
source/tests/pd/model/test_embedding_net.py Migrated expand_sys_str import from deepmd.tf.common to deepmd.common
source/tests/pd/model/test_descriptor.py Migrated expand_sys_str import from deepmd.tf.common to deepmd.common

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codecov
Copy link

codecov bot commented Dec 22, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.15%. Comparing base (188dae3) to head (1904746).
⚠️ Report is 1 commits behind head on devel.

Additional details and impacted files
@@           Coverage Diff           @@
##            devel    #5113   +/-   ##
=======================================
  Coverage   82.15%   82.15%           
=======================================
  Files         709      709           
  Lines       72470    72469    -1     
  Branches     3615     3616    +1     
=======================================
  Hits        59536    59536           
+ Misses      11771    11770    -1     
  Partials     1163     1163           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@njzjz njzjz added this pull request to the merge queue Dec 22, 2025
Merged via the queue into deepmodeling:devel with commit d567700 Dec 22, 2025
64 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants