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

Blue/executor split #365

Merged
merged 40 commits into from
May 21, 2021
Merged

Blue/executor split #365

merged 40 commits into from
May 21, 2021

Conversation

shadeofblue
Copy link
Contributor

No description provided.

@azawlocki azawlocki self-assigned this May 18, 2021
@shadeofblue shadeofblue marked this pull request as ready for review May 18, 2021 10:25
@shadeofblue shadeofblue requested review from a team, kmazurek and filipgolem May 18, 2021 10:25
@azawlocki azawlocki force-pushed the blue/executor-split branch from 19d0cc0 to 5dac94b Compare May 18, 2021 17:00
payload: Payload,
max_workers: Optional[int] = None,
timeout: Optional[timedelta] = None,
budget: Optional[Union[float, Decimal]] = None,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm... why specify budget per task execution? ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@azawlocki do we want to leave it here? or remove it?

Copy link
Contributor

@kmazurek kmazurek left a comment

Choose a reason for hiding this comment

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

Looks fine, the scope plus overall complexity is somewhat overwhelming for me, but I guess I'm just not that familiar with the codebase yet.

@@ -12,6 +12,7 @@
WorkContext,
windows_event_loop_fix,
)
from yapapi.executor import Golem
Copy link
Contributor

Choose a reason for hiding this comment

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

That's most likely not the scope of this PR but it would be best to make this from yapapi import Golem, either by actually moving the code or just importing it in the base package.

Copy link
Contributor Author

@shadeofblue shadeofblue May 20, 2021

Choose a reason for hiding this comment

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

I would say this should be in the scope of the changes to be frozen - but I'll let @azawlocki decide if he'd like to proceed with reshuffling of code between the files within the scope of this pull request...

"""
logger.debug("Creating Executor instance; parameters: %s", locals())
self._init_api(app_key)
Copy link
Contributor

Choose a reason for hiding this comment

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

_init_api is an internal one-liner which is used only here, I think it would be more readable if we just expanded it. Added bonus: it would make the fate of app_key slightly easier to follow (it's one less jump to make, the default env var substitution happens in Configuration#__init__).

Comment on lines 299 to 300
# TODO: add message
logger.debug("TODO", exc_info=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO

async def _handle_proposal(
self,
proposal: OfferProposal,
demand_builder: DemandBuilder,
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be an immutable representation of the demand rather than a builder object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

seems like a good idea. @azawlocki what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

yapapi/executor/__init__.py Outdated Show resolved Hide resolved
Comment on lines 1160 to 1161
# TODO: add message
logger.debug("TODO", exc_info=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO

@azawlocki azawlocki force-pushed the blue/executor-split branch 3 times, most recently from c91830c to b5dade5 Compare May 21, 2021 10:49
@azawlocki azawlocki force-pushed the blue/executor-split branch from b5dade5 to 1973569 Compare May 21, 2021 10:55
@azawlocki azawlocki merged commit b70ab8e into master May 21, 2021
@azawlocki azawlocki deleted the blue/executor-split branch May 21, 2021 11:54
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