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

Fix docstring for sphinx autodoc #443

Merged
merged 2 commits into from
Jan 5, 2024
Merged

Fix docstring for sphinx autodoc #443

merged 2 commits into from
Jan 5, 2024

Conversation

tanmayv25
Copy link
Contributor

No description provided.

@@ -42,5 +43,6 @@ def __call__(self, request):
----------
request : Request
The request object.

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: extra space

@fpetrini15
Copy link
Contributor

fpetrini15 commented Nov 30, 2023

Leaving a bulk comment here because I cannot leave comments on unchaged lines:

Should return fields like this also be decorated?

# Original
InferResult
    "The object holding the result of the inference." 
# Suggestion
InferResult
   "A :py:class:`InferResult` object holding the result of the inference."

=====================================================================
There are a few remaining fields that I think also need to be decorated. E.g.,

"""A KeepAliveOptions object is used to encapsulate GRPC KeepAlive

=====================================================================
I found it useful to cd into src/python/library and execute the following command to find any undecorated stragglers:

# Name
grep -r --include='*.py' -nF '{OBJ_NAME}'

@@ -32,6 +32,7 @@
from tritonclient.utils import *

from .._plugin import InferenceServerClientPlugin
from .._request import Request
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to import this? Is this redundant?

@tanmayv25
Copy link
Contributor Author

Should return fields like this also be decorated?

@fpetrini15 The decoration is for cross-referencing. For the case that you described, we have link within the return type itself.

infer_result

Hence, adding more cross-reference seems redundant exercise. I will double-check if I missed something crucial.

Copy link
Contributor

@rmccorm4 rmccorm4 left a comment

Choose a reason for hiding this comment

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

LGTM at a glance. @tanmayv25 can you share link to rendered version for a quick visual look?

@tanmayv25 tanmayv25 merged commit 2867f07 into main Jan 5, 2024
3 checks passed
@tanmayv25 tanmayv25 deleted the tanmayv-autodoc branch January 5, 2024 22:32
Comment on lines +64 to +73
__all__ = [
"InferenceServerClientPlugin",
"Request",
"InferenceServerClient",
"InferInput",
"InferRequestedOutput",
"InferResult",
"KeepAliveOptions",
"InferenceServerException",
]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this may have broken usage of MAX_GRPC_MESSAGE_SIZE inside aio client as it's not an undefined variable

Copy link
Contributor

Choose a reason for hiding this comment

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

This affects the output of from tritonclient.grpc import * because now it is only importing these symbols defined by __all__ when called in aio here.

kthui added a commit that referenced this pull request Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants