Repository navigation
Added DeePMD-kit PyTorch backend integration - #604
rahulumrao wants to merge 7 commits into
Conversation
| # phono3py>=4.0.0: some older 3.x sdists (e.g. 3.2.0) fail to build and break uv resolution. | ||
| mattersim = ["mattersim>=1.2.5", "phono3py>=4.0.0"] | ||
| metatomic = ["metatomic-torchsim>=0.1.1", "metatomic-ase>=0.1.0", "upet>=0.2.0"] | ||
| deepmd = ["deepmd_torchsim>=0.1.1"] |
There was a problem hiding this comment.
make sure to capture any deps clashes below.
There was a problem hiding this comment.
Verified dependency compatibility, the added deepmd = ["deepmd_torchsim>=0.1.1"] doesn't introduce any conflicts. Because deepmd_torchsim is a thin wrapper that places no direct constraints on deepmd-kit or torch, it won't clash with the other extras.
I intentionally leave deepmd-kit out of the direct dependencies since its PyTorch backend requires specific, torch versions. Offloading those constraints to deepmd_torchsim own deepmd extra keeps torch-sim's dependency tree clean and decoupled.
There was a problem hiding this comment.
Please pin a minimum version that works such that all someone would need to do if importing from top level is install the right option. Otherwise there's no benefit to having the ease of use first party import now we adopted an external model posture
In terms of clashes please check once you've pinned a minimum working set which models deepmd can solve the deps alongside and mark the uv clashes in the pyproject toml. This is also important for UX who use TS for interfacing to many models.
There was a problem hiding this comment.
Done! torch-sim[deepmd] now installs deepmd-kit==3.1.3 with torch==2.10.0. All model extras work with deepmd except fairchem, which has a PyTorch version conflict. Added fairchem to the conflicts list.
There was a problem hiding this comment.
We have standard testing approach for models and their interfaces. Using that infrastructure is a prerequisite to merging here.
There was a problem hiding this comment.
Rewritten on the standard infrastructure and now pushed into GitHub. Test used the DPA-3.1-3M universal model, and passed all 22/22 test locally.
f9ad0b3 to
822d41f
Compare
00cc824 to
4d42bcd
Compare
# Conflicts: # pyproject.toml
|
@rahulumrao Thanks for your interest in contributing! I'm joining the review late, so apologies if this has already been discussed. As far as I understand, @CompRhys If so, I think it would be better for maintenance to have this integration upstreamed into deepmd-kit. What do you think? |
|
I have been sitting on this PR unsure if it made sense to merge as something that might not have longer term support. My true preference is that we will eventually drop all the 1P wrappers here to reduce maintenance burdens. I agree that upstreaming into deepmd-kit is a better idea and then if they were to open an equivalent CL backed by long term support from deepmodelling then we might consider that. |
|
Thanks for the review, @CompRhys and @lil-lon. I've opened an issue to discuss long-term support with the DeePMD-kit maintainers: deepmodeling/deepmd-kit#6125 |
Summary
Checklist
Before a pull request can be merged, the following items must be checked: