-
Notifications
You must be signed in to change notification settings - Fork 644
fix(pt-expt): preserve lower semantics in backend conversion #5975
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
Open
OutisLi
wants to merge
2
commits into
deepmodeling:master
Choose a base branch
from
OutisLi:pr/5973-dpa1-graph-export
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking. This makes
dp convert-backendfail outright for every SeZM/DPA4 PyTorch model.SeZMModel.export_lower_input_kind()returns"edge_vec"(deepmd/pt/model/model/sezm_model.py:3171-3183), and the class is registered as"SeZM"/"sezm"/"DPA4"/"dpa4"at lines 677-680, withSeZMNativeSpinModelinheriting it. So this line now stampslower_input_kind="edge_vec"on the interchange dict,convert_backendforwards an explicit value verbatim, anddeepmd/pt_expt/utils/serialization.py:1429rejects it --_SUPPORTED_LOWER_INPUT_KINDSat lines 55-57 is{"nlist", "graph", "dpa1_canonical", "dpa4c_canonical"}and does not contain"edge_vec". Targeting a backend that does not takelower_kindfails one step earlier, inconvert_backenditself.Before this PR the same conversion worked:
"auto"went into_resolve_lower_kind, which deserializes the model (deepmd/pt_expt/model/dpa4_model.py:35-41registers"sezm"/"SeZM"/"dpa4"/"DPA4", so the pt dict is deserializable there) and returns"graph"or"dpa4c_canonical"-- both supported. This is deterministic, not an edge case.It also contradicts this PR's own documentation.
doc/backend.mdgains the sentence "model families with a graph-native deployment ABI report their corresponding kind" --edge_vecis that kind, for that family, and it is now the only family that cannot be converted at all.The same trade-off has a milder second face worth deciding deliberately: because the base default is
"nlist", a plain.pthcan no longer be promoted tograph/dpa1_canonical/dpa4c_canonicalon conversion, and there is no--lower-kindon the CLI to ask for it. Pinning dense is the correct default given #5973, but the promotion path disappears entirely rather than becoming opt-in.Two directions, either is fine: add
"edge_vec"to the kinds pt_expt accepts and map it onto the schema it already implements (deepmd/pt/entrypoints/freeze_pt2.pyanddeepmd/pt_expt/infer/deep_eval.pyboth already branch on it, so the vocabulary gap is only in the new validator), or have the pt serializer report nothing for models whose ABI the target cannot consume, so they fall back toautoas before.Worth noting how this got through: the new tests do parametrize
"edge_vec"throughconvert_backend, but only against an inline stubOutputBackendthat records the kwarg instead of calling the real pt_expt deserializer, so the failure is invisible in CI. That is the same stub-shaped gap as the one we just closed on the conversion side -- whichever fix you pick, it needs a case that reaches the real target.