Strip doc_string from exported ONNX models #18882
Conversation
facebook-github-bot
added
the
oncall: jit
ezyang
added
module: onnx
|
The patch looks reasonable. It is technically BC-breaking because the default behavior changes; maybe that's OK. It's a little weird that you flip the meaning of the boolean between the frontend and backend. |
|
@ezyang, I set the default to strip the doc string as it is more confidential, and the stack trace does not add much value to the generated model. The backend already uses "strip_doc_string" which I didn't want to change. From the front end side, it seemed to me that add_doc_string would make more sense from the user's point of view (especially if the default is generating the model without the doc_string), which is why I flip the meaning of the boolean. But I can change it to use strip_doc_string everywhere (or add_doc_string everywhere). |
|
@houseroad any update on this? |
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
houseroad
left a comment
houseroad
left a comment
There was a problem hiding this comment.
stripping doc by default should be fine. could you address my comments and resolve the conflicts?
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@houseroad, any other comments? (the lint error is not related to this PR) |
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
rebase and resolve the conflicts? |
|
@houseroad, I resolved the conflicts, could we merge this? |
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@houseroad merged this pull request in 9983c24. |
Summary: Strip the doc_string by default from the exported ONNX models (this string has the stack trace and information about the local repos and folders, which can be confidential). The users can still generate the doc_string by specifying add_doc_string=True in torch.onnx.export(). Pull Request resolved: pytorch#18882 Differential Revision: D14889684 Pulled By: houseroad fbshipit-source-id: 26d2c23c8dc3f484544aa854b507ada429adb9b8
4 participants
Strip the doc_string by default from the exported ONNX models (this string has the stack trace and information about the local repos and folders, which can be confidential).
The users can still generate the doc_string by specifying add_doc_string=True in torch.onnx.export().