Skip to content
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

feat: add get_model classmethod to BaseModel #4002

Merged
merged 1 commit into from
Jul 24, 2024

Conversation

njzjz
Copy link
Member

@njzjz njzjz commented Jul 22, 2024

Fix #3968. External and new models can implement this method (if different from default) without changing the old get_model methods (which cannot be done by a plugin).

Note: I don't modify old get_model methods in this PR.

Summary by CodeRabbit

  • New Features

    • Introduced a new method for model instantiation that enhances flexibility in parameter configuration.
    • Improved the model retrieval process to support dynamic model selection based on specified types.
  • Bug Fixes

    • Enhanced control flow to ensure correct model type selection, addressing potential issues with model retrieval.
  • Refactor

    • Updated existing model retrieval functions to streamline logic and improve maintainability.

Fix deepmodeling#3968. External and new models can implement this method (if different from default) without changing the old `get_model` methods (which cannot be done by a plugin).

Note: I don't modify old `get_model` methods in this PR.
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
@njzjz njzjz requested review from iProzd and wanghan-iapcm July 22, 2024 21:01
Copy link
Contributor

coderabbitai bot commented Jul 22, 2024

Walkthrough

Walkthrough

The recent changes introduce a standardized method for model instantiation across various classes in the DeepMD framework. The new get_model method in BaseBaseModel allows for the flexible creation of model instances using parameter dictionaries, while preserving original parameters for reference. Modifications in related files enhance the model selection logic, facilitating the retrieval of different model types dynamically, thus improving overall functionality and extensibility of the model architecture.

Changes

Files Change Summary
deepmd/dpmodel/model/base_model.py Added get_model method in BaseBaseModel for instantiating models from parameter dictionaries while preserving original parameters.
deepmd/dpmodel/model/model.py Modified get_model function logic to retrieve model type from input data, enhancing model selection process based on specified types.
deepmd/pt/model/model/__init__.py Updated get_model function to incorporate a new model_type variable, allowing dynamic model retrieval based on type, improving flexibility in handling various model configurations.

Assessment against linked issues

Objective Addressed Explanation
PT: get_model should be moved into the BaseModel class (#3968)

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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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 as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@njzjz njzjz linked an issue Jul 22, 2024 that may be closed by this pull request
Copy link
Contributor

@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: 3

deepmd/dpmodel/model/model.py Show resolved Hide resolved
deepmd/pt/model/model/__init__.py Show resolved Hide resolved
deepmd/dpmodel/model/base_model.py Show resolved Hide resolved
Copy link

codecov bot commented Jul 22, 2024

Codecov Report

Attention: Patch coverage is 62.50000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 82.82%. Comparing base (8116297) to head (0db9747).
Report is 107 commits behind head on devel.

Files with missing lines Patch % Lines
deepmd/dpmodel/model/base_model.py 33.33% 6 Missing ⚠️
deepmd/dpmodel/model/model.py 71.42% 2 Missing ⚠️
deepmd/pt/model/model/__init__.py 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            devel    #4002      +/-   ##
==========================================
- Coverage   82.83%   82.82%   -0.02%     
==========================================
  Files         522      522              
  Lines       50901    50917      +16     
  Branches     3015     3015              
==========================================
+ Hits        42165    42171       +6     
- Misses       7799     7807       +8     
- Partials      937      939       +2     

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

@iProzd iProzd added this pull request to the merge queue Jul 23, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 23, 2024
@wanghan-iapcm wanghan-iapcm added this pull request to the merge queue Jul 23, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 23, 2024
@njzjz
Copy link
Member Author

njzjz commented Jul 23, 2024

.47   × No solution found when resolving dependencies:
#10 28.47   ╰─▶ Because only deepmd-kit-cu11[torch]==3.0.0b2.dev15+g91357071 is
#10 28.47       available and deepmd-kit-cu11[torch]==3.0.0b2.dev15+g91357071 depends
#10 28.47       on torch>=2.4.0.dev0,<2.4.1.dev0, we can conclude that all versions of
#10 28.47       deepmd-kit-cu11[torch] depend on torch>=2.4.0.dev0,<2.4.1.dev0.
#10 28.47       And because only the following versions of torch are available:
#10 28.47           torch<2.4.0.dev0
#10 28.47           torch==2.4.0+cu118
#10 28.47           torch>=2.4.1.dev0
#10 28.47       and torch==2.4.0+cu118 depends on nvidia-cudnn-cu11{platform_system
#10 28.47       == 'Linux' and platform_machine == 'x86_64'}==9.1.0.70, we can
#10 28.47       conclude that all versions of deepmd-kit-cu11[torch] depend on
#10 28.47       nvidia-cudnn-cu11==9.1.0.70.
#10 28.47       And because deepmd-kit-cu11[cu11]==3.0.0b2.dev15+g91357071
#10 28.47       depends on nvidia-cudnn-cu11<9 and only
#10 28.47       deepmd-kit-cu11[cu11]==3.0.0b2.dev15+g91357071 is available, we can
#10 28.47       conclude that all versions of deepmd-kit-cu11[cu11] and all versions of
#10 28.47       deepmd-kit-cu11[torch] are incompatible.
#10 28.47       And because you require deepmd-kit-cu11[cu11] and
#10 28.47       deepmd-kit-cu11[torch], we can conclude that the requirements are
#10 28.47       unsatisfiable
28.47       conclude that all versions of deepmd-kit-cu11[cu11] and all versions of
28.47       deepmd-kit-cu11[torch] are incompatible.
28.47       And because you require deepmd-kit-cu11[cu11] and
28.47       deepmd-kit-cu11[torch], we can conclude that the requirements are
28.47       unsatisfiable.
28.47 
28.47       hint: torch was requested with a pre-release marker (e.g., any of:
28.47           torch>=2.4.0.dev0,<2.4.0+cu118
28.47           torch>2.4.0+cu118,<2.4.1.dev0
28.47       ), but pre-releases weren't enabled (try: `--prerelease=allow`)
``‘

@njzjz njzjz added this pull request to the merge queue Jul 24, 2024
Merged via the queue into deepmodeling:devel with commit 269ed3e Jul 24, 2024
60 checks passed
@njzjz njzjz deleted the get_model branch July 24, 2024 04:03
mtaillefumier pushed a commit to mtaillefumier/deepmd-kit that referenced this pull request Sep 18, 2024
Fix deepmodeling#3968. External and new models can implement this method (if
different from default) without changing the old `get_model` methods
(which cannot be done by a plugin).

Note: I don't modify old `get_model` methods in this PR.

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

- **New Features**
- Introduced a new method for model instantiation that enhances
flexibility in parameter configuration.
- Improved the model retrieval process to support dynamic model
selection based on specified types.
  
- **Bug Fixes**
- Enhanced control flow to ensure correct model type selection,
addressing potential issues with model retrieval.

- **Refactor**
- Updated existing model retrieval functions to streamline logic and
improve maintainability.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
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.

PT: get_model should be moved into the BaseModel class
3 participants