-
Notifications
You must be signed in to change notification settings - Fork 37
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
Update MLI docstrings part 1 #692
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## mli-feature #692 +/- ##
==============================================
Coverage ? 69.50%
==============================================
Files ? 103
Lines ? 8750
Branches ? 0
==============================================
Hits ? 6082
Misses ? 2668
Partials ? 0
|
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.
Only a couple of very small comments. Thanks for fixing this up!
:param key: the key of the model to check for existence | ||
:returns: whether the model is available on the device | ||
:param key: The key of the model to check for existence | ||
:returns: Whether the model is available on the device | ||
""" | ||
return key in self._models | ||
|
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.
Does get
need docs?
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.
Oops! Fixed. (Definitely check this one.)
@@ -109,9 +114,9 @@ def _load_model_on_device( | |||
|
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.
I think the previous line would be more correct as:
```Load the model needed to execute a batch on the managed device.``
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.
Fixed!
|
||
the model needed to run the batch of requests is | ||
guaranteed to be available on the model | ||
The model needed to run the batch of requests is |
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.
should on the model
be on the device
?
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.
I think so! Fixed.
I added and updated the MLI docstrings so that everything renders correctly and they are more consistent. This means I needed to make some decisions on what "consistent" means, because there were many different combinations going on. Feel free to push back on these! I just went with what looked like the best/what we were doing the most.
:param:
,:returns:
, etc do not use punctuation to end their description line UNLESS the line needs multiple sentences, in which case, use punctuation. (I think this happens once or twice).example: