Support doc_string for TorchBind custom classes #46576
Conversation
facebook-github-bot
added
the
oncall: jit
💊 CI failures summary and remediationsAs of commit fc9c761 (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group. This comment has been revised 28 times. |
There was a problem hiding this comment.
@gmagogsfm has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Done, updated test to include a multi-line string. Please take another look |
| @@ -110,6 +116,8 @@ struct BuiltinOpFunction : public Function { | |||
| std::function<void(Stack&)> callable_; | |||
|
|
|||
| c10::FunctionSchema schema_; | |||
|
|
|||
| std::string doc_string_; | |||
There was a problem hiding this comment.
just a bit curious here, why we added the doc_string_ inside BuiltinOpFunction instead of Function?
There was a problem hiding this comment.
Function seems to be purely an interface. If we put a member here, then it either 1) limits how subclasses implement doc_string, maybe they want to generate it on the fly 2) or the member is a waste of memory. Let me know what you think.
There was a problem hiding this comment.
discussed offline, let's keep doc_string only in builtinOpFunction as not all subclass of Function need this doc_string.
| @@ -25,6 +25,11 @@ TORCH_API void preoptimizeGraph(std::shared_ptr<Graph>& graph); | |||
| // execution of the function. Method is a wrapper around a | |||
| // underlying Function that also provides a `self` object. | |||
| struct TORCH_API Function { | |||
| virtual const std::string& doc_string() const { | |||
There was a problem hiding this comment.
I think we can add the doc_string here, make it a default "', and then remove the no_doc_string?
There was a problem hiding this comment.
@gmagogsfm has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
@gmagogsfm has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
@gmagogsfm has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Codecov Report
@@ Coverage Diff @@
## master #46576 +/- ##
==========================================
- Coverage 68.96% 68.96% -0.01%
==========================================
Files 434 434
Lines 55993 55993
==========================================
- Hits 38618 38617 -1
- Misses 17375 17376 +1 |
|
@gmagogsfm merged this pull request in f9b9430. |
With this PR, users can optionally provide a "doc_string" to describe a class or its method. doc_string for TorchBind classes and methods are stored as
doc_stringproperties inFunctionandScriptClass. Thesedos_stringproperties are then exposed in Python layer via PyBind for doc generation.Fixes #46047