feat: implement the lazy-deploy mode #539
Conversation
|
@wvmcastro @jhonasiv I strongly recommend to first do a review commit-by-commit. I refactored Engine to extract the runtime adaptation bits, which is a good thing, but made the mistake to not spend the effort to have a clean PR with just this. Looking commit-by-commit would allow you to see what actually is being changed on that part of the code between the original code and the one that is "ready" for lazy deployt |
| # Verifies that all tasks have orocos_name set | ||
| # | ||
| # @param [ResolutionErrorHandler | RaiseErrorHandler] error_handler | ||
| def verify_all_tasks_lazily_deployed(error_handler: RaiseErrorHandler.new) | ||
| self.class.verify_all_tasks_lazily_deployed( | ||
| plan, default_deployment_group, error_handler: error_handler | ||
| ) | ||
| end | ||
|
|
||
| # @see #verify_all_tasks_lazily_deployed | ||
| # | ||
| # @param [Component=>DeploymentGroup] deployment_groups which | ||
| # deployment groups has been used for which task. This is used | ||
| # to generate the error messages when needed. | ||
| def self.verify_all_tasks_lazily_deployed( |
There was a problem hiding this comment.
I dont see the use of this methods. the changes you made on verify_all-tasks_deployed is enough IMO
| ) | ||
|
|
||
| deployer.deploy( | ||
| @used_deployments, = deployer.deploy( |
There was a problem hiding this comment.
Why not forward the used deployments up until the calling places of apply_system_network_to_plan (the resolve method and async AFAIK)? It feels cleaner even if I cant find another behavior problem that comes from the way you implemented it
There was a problem hiding this comment.
The amount of things we would need to "remove" the state from Engine itself is ... big actually.
I think we'll need sometimes to either do all of it or none of it, but right now it is 80% inside the task and 20% as argument... I don't think there's a "winner" w.r.t. being consistent.
wvmcastro
left a comment
wvmcastro
left a comment
There was a problem hiding this comment.
I think you could reuse the computed used_deployments between #compute_system_network and #compute_deployed_network at #resolve_system_network when early deploy is on.
| if lazy | ||
| query.find_all { !_1.orocos_name } | ||
| else | ||
| query.find_all { !_1.execution_agent } | ||
| end |
There was a problem hiding this comment.
Registering what we've talked in person: Create a #deployed? predicate to abstract what being deployed means
doudou
force-pushed
the
lazy_deploy
branch
3 times, most recently
from
158ef1d to
2eb16fa
Compare
The 'lazy-deploy' mode only sets the orocos_name argument during network generation, to avoid unnecessary creation of deployment and deployed task instances on networks that are being modified. This commit is the first of the overall implementation. It modifies the network generation itself, but is not functional yet.
…gine It will have to be significantly modified for the lazy-deploy mode, so best have the current known-to-work version in a cleanly separated class.
The lazy deployments are only settings orocos_name. The objective of the execution_agent test is to actually check that the task's deployment has been chosen and is the same (even if the instance is different), which can be done by comparing orocos_name Modify it. This fixes merging of tasks with lazy-deploy enabled
A requirement for lazy deployment
… as well In the end, instead of creating a brand new copy of RuntimeNetworkAdaptation, it seemed viable to do small adaptations to make it work in both eager and lazy deploy cases
…Engine and Generator
…s in DataflowDynamics
The relation is here to make sure we don't execute two instances of a task context at the same time. This *definitely* cannot be removed automatically
The non-reusable but not finished tasks are handled normally through the adaptation mechanisms.
…to be nil/non-nil
In that case, we need to 'exfiltrate' the used_deployments hash from the network generation step to pass to runtime network adaptation.
They are actual components, not deployments, and are needed for dataflow analysis
I did not change the !( ... <= ... ) into ( ... > ... ) as even if Ruby somehow pretends the order is total, it really is not. That is, I would expect A < B and A > B to both return false (or even nil) for unrelated classes.
The current network generation + adaptation algorithm currently:
With early deploy, it means instantiating more deployments and deployed tasks than the final number of task contexts (because early deploy happens before merge), to then - in practice - replace most of them by the tasks from the plan (assuming we don't change the whole system all the time).
The late deploy mode tries to avoid all that wasted energy. Deployment in system network generation is now only setting the
orocos_namefield. We generate new deployed tasks only during the plan adaptation, and only when needed.