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

[BUG] Enable k8s array plugin and aws batch plugin simultaneously #2205

Closed
2 tasks done
hamersaw opened this issue Feb 23, 2022 · 2 comments · Fixed by flyteorg/flytepropeller#405
Closed
2 tasks done
Assignees
Labels
bug Something isn't working

Comments

@hamersaw
Copy link
Contributor

Describe the bug

The k8s array plugin and aws batch plugin both declare themselves as the default plugin for the task type "container_array". Therefore, a FlytePropeller instance is unable to enable both of these plugins simultaneously.

Expected behavior

A Flyte deployment should be able to support the k8s array plugin and AWS batch plugin simultaneously.

Additional context to reproduce

The following FlytePropeller task configuration:

tasks:
  task-plugins:
    enabled-plugins:
      - aws_array
      - K8S-ARRAY
    default-for-task-types:
      - container_array: K8S-ARRAY

causes this error:
Error: Error running FlytePropeller.: TaskType [container_array] has multiple default handlers specified: [aws_array] and [k8s-array]

Screenshots

No response

Are you sure this issue hasn't been raised already?

  • Yes

Have you read the Code of Conduct?

  • Yes
@hamersaw hamersaw added bug Something isn't working untriaged This issues has not yet been looked at by the Maintainers and removed untriaged This issues has not yet been looked at by the Maintainers labels Feb 23, 2022
@hamersaw hamersaw self-assigned this Feb 23, 2022
@hamersaw hamersaw added this to the 0.19.3 - Feb 2021 milestone Feb 23, 2022
@hamersaw
Copy link
Contributor Author

@EngHabu @katrogan wanted to get a quick opinion here based on the previous isuse.

To breakdown what is happening in this case, because this is fairly bizarre. We have the ability to define a plugin as being the default for a task type in two place, namely the flytepropeller DefaultForTaskTypes configuration and the plugin entries DefaultForTaskType field. If there is an entry (or multiple) for a plugin defined in the flytepropeller configuration we replace the plugins default task types during the registration process here for a core plugin and here for a k8s plugin.

In this example, the aws_batch plugin declares itself as the default plugin for the container_array task type in it's plugin entry, and then the k8s-array plugin declares as the default for the container_array task type in the flytepropeller configuration. This introduces the obvious conflict.

It looks like there was some previous discussion here and here regarding the DefaultForTaskType plugin field. I think we may have missed a corner case in the implementation.

That being said, I don't see any reason why we need the DefaultForTaskType field defined in the plugin registry entry. I think it makes sense to register plugin types as follows:
(1) if there is only one plugin registered for a task type it is automatically the default
(2) if more than one plugin registered for a task type - require setting the default in the flytepropeller configuration
Implementing this will be trivial, really just removing some functionality that is unnecessary.

It was difficult to track the origins of the plugin configuration, but it seems to predate the flytepropeller configuration. Can we deprecate the DefaultForTaskType plugin entry field?

@katrogan
Copy link
Contributor

katrogan commented Mar 1, 2022

That being said, I don't see any reason why we need the DefaultForTaskType field defined in the plugin registry entry. I think it makes sense to register plugin types as follows:
(1) if there is only one plugin registered for a task type it is automatically the default
(2) if more than one plugin registered for a task type - require setting the default in the flytepropeller configuration
Implementing this will be trivial, really just removing some functionality that is unnecessary.

This all sounds super reasonable! I know Haytham was looking into the plugins' specific defaults config so I'll let him chime in, but centralizing the config in propeller sounds 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants