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

Remove implicit_init from the WorkContext #647

Merged
merged 11 commits into from
Sep 15, 2021
Merged

Conversation

johny-b
Copy link
Contributor

@johny-b johny-b commented Sep 10, 2021

resolves #649

Also: fixes current problem with services without start() method.

@johny-b johny-b changed the title replace implicit init with an explicit init Remove implicit_init from the WorkContext Sep 13, 2021
@johny-b johny-b marked this pull request as ready for review September 13, 2021 13:08
@johny-b johny-b requested review from a team, shadeofblue and kmazurek September 13, 2021 13:08
yapapi/services.py Outdated Show resolved Hide resolved
yapapi/services.py Outdated Show resolved Hide resolved
@shadeofblue
Copy link
Contributor

added some suggestions, otherwise, I think it's okay

Comment on lines 191 to 193
if self._implicit_init:
work_context.deploy()
work_context.start()
Copy link
Contributor

Choose a reason for hiding this comment

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

These two calls add commands to the internal Script instance kept by a WorkContext. When the non-deprecated way of creating scripts is used (i.e. calling new_script on the WorkContext and adding steps to that Script) in the requestor application, these commands will end up never being called.
One option I can think of is to perform the implicit init in WorkContext#new_script and use an internal bool flag to check whether deploy/start should be added to the newly returned Script instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in bc86bae, hopefully.

Copy link
Contributor Author

@johny-b johny-b Sep 14, 2021

Choose a reason for hiding this comment

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

I've chosen other solution than proposed because now everything is still in Executor, without any implicit-init related things in WorkContext and I think there's some value in having this in a single place.

@shadeofblue
Copy link
Contributor

the issue I have is that we're adding overhead of sending two batches (one with the init, and one with commands coming from the client code) instead of just one, where the user script is prepended with the init

@johny-b
Copy link
Contributor Author

johny-b commented Sep 14, 2021

the issue I have is that we're adding overhead of sending two batches (one with the init, and one with commands coming from the client code) instead of just one, where the user script is prepended with the init

Do you know any reason why this should be harmful?
But well, I'd also prefer to avoid this, but not at a cost of putting this stuff in WorkContext.

Maybe a simple solution would be to merge those two batch generators into a single one? I tried that first, but this was too hard for me :/ But I could give it another try if you think this is important.

@shadeofblue
Copy link
Contributor

the issue I have is that we're adding overhead of sending two batches (one with the init, and one with commands coming from the client code) instead of just one, where the user script is prepended with the init

Do you know any reason why this should be harmful?
But well, I'd also prefer to avoid this, but not at a cost of putting this stuff in WorkContext.

Maybe a simple solution would be to merge those two batch generators into a single one? I tried that first, but this was too hard for me :/ But I could give it another try if you think this is important.

harmful might be too strong a word, rather potentially suboptimal performance-wise...

@johny-b
Copy link
Contributor Author

johny-b commented Sep 14, 2021

the issue I have is that we're adding overhead of sending two batches (one with the init, and one with commands coming from the client code) instead of just one, where the user script is prepended with the init

Do you know any reason why this should be harmful?
But well, I'd also prefer to avoid this, but not at a cost of putting this stuff in WorkContext.
Maybe a simple solution would be to merge those two batch generators into a single one? I tried that first, but this was too hard for me :/ But I could give it another try if you think this is important.

harmful might be too strong a word, rather potentially suboptimal performance-wise...

As far as I understand the Executor/Engine code, this makes ~~ 0 difference from the performance POV.

@shadeofblue
Copy link
Contributor

the issue I have is that we're adding overhead of sending two batches (one with the init, and one with commands coming from the client code) instead of just one, where the user script is prepended with the init

Do you know any reason why this should be harmful?
But well, I'd also prefer to avoid this, but not at a cost of putting this stuff in WorkContext.
Maybe a simple solution would be to merge those two batch generators into a single one? I tried that first, but this was too hard for me :/ But I could give it another try if you think this is important.

harmful might be too strong a word, rather potentially suboptimal performance-wise...

As far as I understand the Executor/Engine code, this makes ~~ 0 difference from the performance POV.

it's not about Executor/Engine - it's about provider <-> requestor communications....

@johny-b johny-b merged commit 4740fb1 into master Sep 15, 2021
@shadeofblue shadeofblue deleted the johny-b/remove-implicit-init branch September 16, 2021 11:24
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.

Move implicit_init to Executor only
3 participants