Missing docstrings for public classes and methods #416
Conversation
azawlocki
force-pushed
the
az/service-api-docstrings
branch
from
9eb53f6 to
a7889be
Compare
| @@ -124,6 +128,8 @@ def unpack_work_item(item: WorkItem) -> Tuple[Work, ExecOptions]: | |||
|
|
|||
|
|
|||
| class Golem(AsyncContextManager): | |||
| """Base execution engine containing functions common to all modes of operation.""" | |||
There was a problem hiding this comment.
Since this has become our main entry point class for the API I'd consider including a bit more information about it here (e.g. the fact it is considered a main entry point, mention it's usually a good idea to have a single instance in the agent code, how and why it can be used as a context manager etc.). This is just a suggestion though, not a change request.
There was a problem hiding this comment.
yeah, it would be a good idea to include a wider description and an example usage perhaps but indeed, it definitely could be a separate pull request
There was a problem hiding this comment.
I agree but decided to leave it for a separate PR, as suggested by @shadeofblue
There was a problem hiding this comment.
A follow-up issue: #425
yapapi/executor/__init__.py
Outdated
| :param worker: an async generator that takes a `WorkContext` object and a sequence | ||
| of tasks, and generates as sequence of work items to be executed on providers in order | ||
| to compute given tasks | ||
| :param data: an iterator of `Task` objects to be computed on providers |
There was a problem hiding this comment.
| :param data: an iterator of `Task` objects to be computed on providers | |
| :param data: an iterator or generator of `Task` objects to be computed on providers |
There was a problem hiding this comment.
fixed in 761f820
@shadeofblue btw, we should probably rename this parameter to tasks, data is weird
yapapi/executor/__init__.py
Outdated
| """Run a number of instances of a service represented by a given `Service` subclass. | ||
|
|
||
| :param service_class: a subclass of `Service` that represents the service to be run | ||
| :param num_instances: the number of service instances to run |
There was a problem hiding this comment.
| :param num_instances: the number of service instances to run | |
| :param num_instances: optional number of service instances to run, defaults to a single instance if not given |
yapapi/executor/__init__.py
Outdated
| :param num_instances: the number of service instances to run | ||
| :param payload: optional runtime definition for the service; if not provided, the | ||
| payload specified by the `get_payload()` method of `service_class` is used | ||
| :param expiration:optional expiration date for the service |
There was a problem hiding this comment.
| :param expiration:optional expiration date for the service | |
| :param expiration: optional expiration datetime for the service |
| @@ -907,29 +969,34 @@ def __init__( | |||
|
|
|||
| @property | |||
| def driver(self) -> str: | |||
| """Return the payment driver used for this `Executor`'s engine.""" | |||
There was a problem hiding this comment.
just a thought - is this still required here? (apart from backwards-compatibility) ? ... maybe we should add @deprecated here?
There was a problem hiding this comment.
It's still used in blender-deprecated.py, a version of our Blender example that uses the Executor interface.
We probably don't need to deprecate it separately from the whole Executor class.
| return self._engine.driver | ||
|
|
||
| @property | ||
| def network(self) -> str: | ||
| """Return the payment network used for this `Executor`'s engine.""" |
There was a problem hiding this comment.
just a thought - is this still required here? (apart from backwards-compatibility) ? ... maybe we should add @deprecated here?
There was a problem hiding this comment.
It's still used in blender-deprecated.py, a version of our Blender example that uses the Executor interface.
We probably don't need to deprecate it separately from the whole Executor class.
yapapi/executor/services.py
Outdated
|
|
||
| @property | ||
| def id(self): | ||
| """Return the id of the work context associated with this service instance.""" |
There was a problem hiding this comment.
well, I'm unsure - my thought here was that it would be "an identifier of an instance" and it would an internal implementation detail that it's - effectively - equal to the id of the respective work context ...
Co-authored-by: Kuba Mazurek <[email protected]> Co-authored-by: shadeofblue <[email protected]>
Resolves #410
Resolves golemfactory/yagna-triage#109