Skip to content

Conversation

@thangckt
Copy link

@thangckt thangckt commented Nov 27, 2025

This pull request primarily removes unused imports and cleans up type annotations across several modules in the dpgen2 codebase. These changes help streamline the code, reduce unnecessary dependencies, and improve readability. Additionally, some minor logic and formatting updates are included to modernize the code and ensure consistency.

Code cleanup and import removal

  • Removed unused imports such as os, json, logging, glob, pickle, and several type hints (Set, Union, etc.) from multiple files including main.py, showkey.py, status.py, submit.py, watch.py, workflow.py, prep_fp.py, run_fp.py, and vasp.py. This reduces clutter and potential confusion for future maintenance. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13]

Type annotation improvements

  • Updated function signatures to use modern type hinting, such as changing Optional[Dict] = {} to Optional[Dict] | None = None and ensuring default values are properly handled with wf_config = wf_config or {}. This makes the code safer and more in line with current Python standards. [1] [2]

Minor logic and formatting updates

  • Improved string formatting in logging statements and conditional checks for better readability and consistency. For example, switched to f-strings with consistent quotation marks and improved assertion error messages. [1] [2]

These changes collectively enhance code maintainability and readability without altering core functionality.This pull request primarily cleans up unused imports and improves code readability across several entrypoint modules in the dpgen2 package. It also introduces minor enhancements to type hints and default argument handling for better code clarity and robustness.

Codebase cleanup and readability:

  • Removed unused imports (such as os, glob, pickle, dpdata, etc.) from multiple files including main.py, showkey.py, status.py, submit.py, and workflow.py to streamline the codebase. [1] [2] [3] [4] [5]
  • Simplified assertion error messages and improved string formatting for logging statements in submit.py and watch.py. [1] [2]

Type hint and argument improvements:

  • Updated type hints for optional dictionary arguments in watch.py and workflow.py to use Optional[Dict] | None, and ensured proper default initialization using wf_config = wf_config or {}. [1] [2]
  • Minor formatting improvements for argument parsing and function calls for better readability in main.py. [1] [2]

General code style enhancements:

  • Removed unnecessary tuple and union type imports, and cleaned up type hints in submit.py and status.py. [1] [2] [3]

Summary by CodeRabbit

  • Refactor

    • Removed numerous unused imports across entry points and FP modules to reduce dependency surface and trim code.
    • Standardized logging/string formatting for consistency.
  • Bug Fixes

    • Changed workflow-config default from a mutable empty dict to None with internal initialization to avoid shared-state issues.
    • Corrected membership check used when computing finished-step diffs to fix status reporting.

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

thangckt and others added 4 commits June 18, 2024 17:28
Removed unused imports from several entrypoint modules to reduce clutter and improve maintainability. Updated type hints for optional dictionary arguments to use modern Python syntax and avoid mutable default argument values
@coderabbitai
Copy link

coderabbitai bot commented Nov 27, 2025

📝 Walkthrough

Walkthrough

Removed unused imports across multiple modules, replaced mutable dict defaults with None plus defensive initialization in watch and workflow, and inverted a conditional in watch.update_finished_steps.

Changes

Cohort / File(s) Change Summary
Entrypoint import cleanup
dpgen2/entrypoint/main.py, dpgen2/entrypoint/showkey.py, dpgen2/entrypoint/status.py, dpgen2/entrypoint/submit.py, dpgen2/entrypoint/workflow.py
Removed unused imports (e.g., os, glob, pickle, Path, dpdata, unused typing names, and dflow exports). Minor string formatting consistency edits. No public API signature removals except wf_config defaults noted separately.
Watch entrypoint changes
dpgen2/entrypoint/watch.py
Changed wf_config default from {} to None and added wf_config = wf_config or {} defensive init; replaced Optional[Dict] with Optional[dict]; flipped conditional in update_finished_steps from kk in finished_keys to kk not in finished_keys; minor logging formatting tweaks.
Workflow default fix
dpgen2/entrypoint/workflow.py
Changed execute_workflow_subcommand signature default wf_config from {} to None and added defensive wf_config = wf_config or {} before normalization.
FP modules import cleanup
dpgen2/fp/prep_fp.py, dpgen2/fp/run_fp.py, dpgen2/fp/vasp.py, dpgen2/fp/vasp_input.py
Removed unused imports (e.g., json, os, dpdata, and several typing aliases). No functional or signature changes.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Areas to inspect:
    • watch.update_finished_steps: confirm the inverted membership check is intentional and matches expected diffing behavior.
    • Call sites that pass mutable wf_config defaults — ensure changing default identity to None doesn't affect callers relying on shared dict identity.
    • Removal of dpdata and other imports where imports might be conditionally required at runtime.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
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.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main changes: removing unused imports and improving type hints across multiple modules.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

  • Provide your own instructions using the high_level_summary_instructions setting.
  • Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
  • Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

  1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
  2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
  3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
  4. 📊 Contributor Summary — Include a Markdown table showing contributions:
    | Contributor | Lines Added | Lines Removed | Files Changed |
  5. ✔️ Additional Notes — Add any extra reviewer context.
    Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.


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.

thangckt and others added 2 commits November 27, 2025 14:55
Removed unused imports from prep_fp.py, run_fp.py, vasp.py, and vasp_input.py to improve code clarity and reduce clutter. Also applied minor formatting changes for better readability, such as consolidating multi-line statements and docstrings.
@codecov
Copy link

codecov bot commented Nov 28, 2025

Codecov Report

❌ Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 84.17%. Comparing base (9b6f382) to head (02fce86).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
dpgen2/entrypoint/watch.py 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #324      +/-   ##
==========================================
- Coverage   84.23%   84.17%   -0.07%     
==========================================
  Files         104      104              
  Lines        6129     6111      -18     
==========================================
- Hits         5163     5144      -19     
- Misses        966      967       +1     

☔ 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.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
dpgen2/entrypoint/workflow.py (1)

35-42: Good change away from mutable default; consider a slightly clearer None check

Switching wf_config’s default from {} to None and initializing inside the function removes the mutable default pitfall; that’s the right direction.

To make the intent a bit clearer (and avoid re-allocating when an empty dict is intentionally passed), you could prefer an explicit None check over or {}:

 def execute_workflow_subcommand(
     command: str,
     wfid: str,
-    wf_config: Optional[dict] = None,
+    wf_config: Optional[dict] = None,
 ):
-    wf_config = wf_config or {}
+    if wf_config is None:
+        wf_config = {}
     wf_config = normalize_args(wf_config)

This keeps behavior for None the same while preserving any dict object explicitly provided by the caller.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ca40173 and 02fce86.

📒 Files selected for processing (2)
  • dpgen2/entrypoint/watch.py (1 hunks)
  • dpgen2/entrypoint/workflow.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • dpgen2/entrypoint/watch.py

@zjgemi zjgemi merged commit 687fb41 into deepmodeling:master Dec 1, 2025
9 checks passed
@thangckt thangckt deleted the cleanup branch December 11, 2025 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants