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

[onnx] Reconstruct torch model from onnx doc_string #511

Open
wants to merge 48 commits into
base: master
Choose a base branch
from

Conversation

take-cheeze
Copy link
Contributor

@take-cheeze take-cheeze commented Feb 8, 2022

  • Also output constant nodes with original node
  • Always convert onnx identity node as onnx::Identity node for missing doc_string
  • Rename default unique named values like v{v.unique()} to keep names same both in torch graph and onnx graph
  • Add check_reconstruct option to onnx test utilities
  • Fix parameter name destruction like https://ci.preferred.jp/pytorch-pfn-extras.torch110-win/112649/#L1415

@@ -0,0 +1,97 @@
import marko
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To parse doc_string's markdown marko will be used

@@ -421,7 +434,9 @@ def handle_if(self, g: torch._C.Graph, n: torch._C.Node) -> None:
# Generated onnx node doc string should be added later since DCE isn't completed yet
doc_str: str = f"""
## Original node
```
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will be quoted for easier markdown parsing

@@ -384,7 +384,7 @@ def _dump_upgraders_map() -> Dict[str, str]: ...
def _test_only_populate_upgraders(content: Dict[str, str]) -> None: ...
def _test_only_remove_upgraders(content: Dict[str, str]) -> None: ...
def merge_type_from_type_comment(decl: Decl, type_annotation_decl: Decl, is_method: _bool) -> Decl: ...
def parse_ir(input: str, parse_tensor_constants: _bool) -> Graph: ...
def parse_ir(input: str, parse_tensor_constants: _bool = False) -> Graph: ...
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kmaehashi kmaehashi removed this from the v0.6.6 milestone Apr 6, 2023
@kmaehashi kmaehashi assigned linshokaku and unassigned kmaehashi Apr 6, 2023
@linshokaku
Copy link
Member

/test

@take-cheeze
Copy link
Contributor Author

Hmm, cpu test seems to be running with torch-2.0. Let's fix that first

@kmaehashi
Copy link
Member

Pre-review test seems still failing?

@take-cheeze
Copy link
Contributor Author

/test

@take-cheeze
Copy link
Contributor Author

https://ci.preferred.jp/pytorch-pfn-extras.torch113-linux/128554/261346#L1514 segv occured in pytest but seems to be a flaky behaviour

@take-cheeze
Copy link
Contributor Author

/test

linshokaku
linshokaku previously approved these changes Apr 26, 2023
kmaehashi
kmaehashi previously approved these changes May 23, 2023
@take-cheeze take-cheeze dismissed stale reviews from kmaehashi and linshokaku via 3ad7f41 May 23, 2023 06:00
@take-cheeze
Copy link
Contributor Author

/test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants