-
Notifications
You must be signed in to change notification settings - Fork 23
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
the docstring for Golem
#447
Conversation
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.
Nice, very clear! I have some questions and minor suggestions though.
yapapi/golem.py
Outdated
The first one - `execute_tasks` - instructs Golem to take a sequence of task fragments | ||
that the user wishes to compute on Golem and distributes those among the providers. | ||
|
||
The second one - `run_service` - instructs Golem to spawn a certain number of instances |
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 feel like there's some ambiguity in the use of the word Golem
here and in the two previous paragraphs. Is it Golem
the Python class? Is it Golem
as in local yagna
node? Is it Golem
as in the network?
yapapi/golem.py
Outdated
|
||
Whereas a task-based job exists for the purpose of computing the specific | ||
sequence of tasks and is done once the whole sequence has been processed, the | ||
service-based job is created for an indefinite period and the services spawned |
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.
potentially indefinite?
yapapi/golem.py
Outdated
shutdown. | ||
|
||
As `Golem`'s job includes tracking and executing payments for activities spawned by | ||
either mode of operation, it's usually good to have just one instance of `Golem`. |
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.
To have it where/when? I'd add some more context here, e.g. "at any given time" and/or "in a given program" or something like that.
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.
okay, going with "at any given time"
Additionally, the service interface provides a way to easily define handlers for | ||
certain, discrete phases of a lifetime of each service instance - startup, running and | ||
shutdown. |
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.
Can we somehow link to the Service
class here? It's docstring should contain more information about the lifecycle of service instances.
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'm unsure how to link to another place in the docs, plus, I'd make it a separate PR
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 like the style but had some minor comments regarding the content.
yapapi/golem.py
Outdated
""" | ||
The main entrypoint of the Golem's high-level API. |
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.
""" | |
The main entrypoint of the Golem's high-level API. | |
"""The main entrypoint of the Golem's high-level API. |
Flake 8 docstrings plugin, if we ever enable it, would require us to change that. That would be also consistent with other multi-line docstrings e.g. in yapapi.executor
.
Co-authored-by: Kuba Mazurek <jakub.mazurek@golem.network>
…marks by @zakaprov and @azawlocki
updated the code, please re-review, @zakaprov @azawlocki |
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.
LGTM!
No description provided.