Fix is_local for LocalKubernetesExecutor #28288
Conversation
boring-cyborg
bot
added
the
area:Scheduler
There was a problem hiding this comment.
Approving. Though note: a discussion is ongoing for the final approach to be taken, so this solution may change, please see here: #28276 (comment)
| @@ -43,6 +43,8 @@ class LocalKubernetesExecutor(LoggingMixin): | |||
|
|
|||
| KUBERNETES_QUEUE = conf.get("local_kubernetes_executor", "kubernetes_queue") | |||
|
|
|||
| is_local: bool = False | |||
There was a problem hiding this comment.
This is tricky, since this executor is sometimes local, but I think it's best to go with False unless all executors are local.
There was a problem hiding this comment.
Yep I thought the same. This is the conservative choice, and equivalent to what was implemented before
There was a problem hiding this comment.
Yeah i mean, the flag itself is problematic. Because, with these composite executors, it's not yes or no.
Is it local? well, yes and no -- depends what you need, depends why you are asking. So this flag should probably be replaced with something more specific to what it's actually concerned about. Iike, the specific behavior that is required / investigated and not whether the executor is "a local executor".
There was a problem hiding this comment.
I thinkg better question is - what is the use and semantic of the "local" flag - why it is needed and what we want to achieve by it. I.e. what different happens when executor is local vs. non-local - and can we name it differently in this case to better reflect the meaning.
There was a problem hiding this comment.
that's basically what i'm saying. is_local doesn't make sense when applied to these executors, it's probably a proxy for some behavior that .... prior to the composite executors made sense but no longer does.
pierrejeambrun
deleted the
fix-is-local-for-local-kurbernetes-executor
branch
pierrejeambrun
added
the
type:misc/internal
LocalKurbernetesExecutordoes not extends theBaseExecutorclass, and therefore does not inheritis_localattribute.See #28276 general discussion on how to handle mixed executor API.
cc: @dstandish @o-nikolas