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

Move Golem and Job from yapapi.executor.__init__ to yapapi.golem #431

Merged
merged 4 commits into from
Jun 7, 2021

Conversation

azawlocki
Copy link
Contributor

@azawlocki azawlocki commented Jun 2, 2021

Separating Golem and Executor was not straightforward, since:

  • Executor creates an instance of Golem in its __init__() method
  • Golem creates an instance of Executor in its execute_tasks() method

For this reason, the class Golem has been split into a mini-hierarchy:

  • new class Engine, including everything Golem included, except the high-level methods run_service() and execute_tasks()
  • its subclass Golem, containing just the two methods mentioned above

The class Engine (together with Job and some utility functions) is in module yapapi.engine, while Golem is in yapapi.golem. Executor needs to import only yapapi.engine, and Golem may import yapapi.executor without creating
circular depdendencies.

All this was done rather quickly, there's probably much to be done, like adding the docstrings in golem.py and fixing the docstrings and comments elsewhere.

Also, some modules should be removed from yapapi.executor package, as they contain code that is used by Engine and Golem rather than (or in addition to) Executor, for example agreements_pool.py.

UPDATE: Further changes:

  • rename class yapapi.engine.Engine to yapapi.engine._Engine, to indicate that it's not part of the public API
  • move strategy.py (a module with MarketStrategy class, belonging to the public API) from yapapi/executor/ to yapapi/ -- this will be done in a separate PR, along with some other modules that currently reside in yapapi/executor/ but should not

@shadeofblue
Copy link
Contributor

wow, good idea!

Base automatically changed from az/services-out-of-executor to master June 7, 2021 06:36
Copy link
Contributor

@shadeofblue shadeofblue left a comment

Choose a reason for hiding this comment

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

@azawlocki accepting :) though, one further thought: what if we move yapapi.engine to yapapi._engine to further draw attention to the fact that the Engine class is yapapi's internal part?

@azawlocki azawlocki self-assigned this Jun 7, 2021
@azawlocki
Copy link
Contributor Author

@azawlocki accepting :) though, one further thought: what if we move yapapi.engine to yapapi._engine to further draw attention to the fact that the Engine class is yapapi's internal part?

@shadeofblue yes, we should probably do something like this. If we decided to make the whole yapapi.engine module "private" we should first remove some items that should be part of the public API of the library: the exception NoPaymentAccountAvailable and the type WorkItem.

@azawlocki azawlocki merged commit fcb58bf into master Jun 7, 2021
@azawlocki azawlocki deleted the az/golem-module branch June 7, 2021 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants