SDK - Components - Refactoring - _resolve_graph_task now creates tasks directly #3448
Conversation
…nd graph handlers This is a refactoring PR. The main goal of the PR is to make _components._container_task_constructor receive TaskSpec.execution_options, is_enabled and any options that might be added in the future. Otherwise, the customizations of tasks in a graph component are lost. This PR partially reverses the previous refactoring which switched away from TaskSpec usage. The interface for _components._container_task_constructor has changed from (component_spec, arguments, component_ref) to (task_spec, arguments). The reason is that task_spec has additional attribues (execution_options) that should be passed in. It looks weird to pass arguments separately (as task_spec can already hold arguments), but the reason for this is that the passed arguments may have types that are incompatible with TaskSpec.arguments. So the arguments are passed separately. The interface is private, so it's fine to make a breaking change here as we control all implementations.
…s directly This change is a pure refactoring. Previously _resolve_graph_task was creating task-like objects by materializing components and calling the resulting factory functions. One problem with this approach is that it prevented task options from reaching the _container_task_constructor. With this change, the _resolve_graph_task function directly calls the extracted _create_task_object_from_task_spec_and_arguments function and all task options are preserved.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
This change is |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
stale
bot
added
the
lifecycle/stale
Ark-kun
removed
the
lifecycle/stale
|
@Ark-kun: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
stale
bot
added
the
lifecycle/stale
|
This issue has been automatically closed because it has not had recent activity. Please comment "/reopen" to reopen it. |
This PR builds on top of #3447
This change is a pure refactoring.
Previously
_resolve_graph_taskwas creating task-like objects by materializing components and calling the resulting factory functions. One problem with this approach is that it prevented task options from reaching the_container_task_constructor. With this change, the_resolve_graph_taskfunction directly calls the extracted_create_task_object_from_task_spec_and_argumentsfunction and all task options are preserved.