AIP-51 - Misc. Compatibility Checks #28375

rkarish · 2022-12-15T06:25:30Z

o-nikolas

Thanks for the PR! I've left some comments and requests :)

o-nikolas · 2022-12-16T22:55:14Z

airflow/executors/celery_kubernetes_executor.py

@@ -38,6 +38,7 @@ class CeleryKubernetesExecutor(LoggingMixin):
    otherwise, CeleryExecutor is used.
    """

+    is_picklable: bool = True


You'll also need to set a value for supports_sentry in this class as well since it does not implement the BaseExecutor interface (see the discussion starting from here for more context). You'll need to ensure both new fields are set on all the executors which don't inherit from BaseExecutor (LocalKubernetesExecutor as well) and add the appropriate unit tests for those.

o-nikolas · 2022-12-16T22:57:28Z

airflow/executors/executor_loader.py

@@ -167,10 +167,3 @@ def __load_local_kubernetes_executor(cls) -> BaseExecutor:

        local_kubernetes_executor_cls = import_string(cls.executors[LOCAL_KUBERNETES_EXECUTOR])
        return local_kubernetes_executor_cls(local_executor, kubernetes_executor)
-
-
-UNPICKLEABLE_EXECUTORS = (


I wonder if we should actually keep this in for backwards compatibility (with a comment describing that it is deprecated) 🤔 i.e. someone in the community could be depending on this list existing. I wonder what others think about this.

I searched the repository for references and it looked like it was safe to delete, but I didn't think about other users in the community being dependent on this. I think that adding a comment here and waiting to clean this up at a later time is a safer approach.

Maybe add a # TODO: Remove in Airflow 3.0. We have a couple of those in the repo and will help to not miss this spot when releasing the next major.

o-nikolas · 2022-12-16T23:00:36Z

airflow/executors/base_executor.py


    job_id: None | int | str = None
    callback_sink: BaseCallbackSink | None = None

    is_local: bool = False
+    is_picklable: bool = False


I think we want the default to be True, no? The previous code assumed all executors were picklable unless it was present in the list of UNPICKLEABLE_EXECUTORS. So any executor that goes through that code should be assumed to be picklable unless it says otherwise. WDYT?

I agree that this makes more sense. For some reason I went with the opposite 🙃

o-nikolas · 2022-12-16T23:08:02Z

airflow/executors/local_kubernetes_executor.py

@@ -38,6 +38,7 @@ class LocalKubernetesExecutor(LoggingMixin):
    otherwise, LocalExecutor is used.
    """

+    is_picklable: bool = True


Do we actually want this to be True? The LocalExecutor is not picklable and so far we've gone with True for these only if all sub executors are True. WDYT?
There is some context in a PR comment here (from @pierrejeambrun's PR for is_local)

o-nikolas · 2022-12-16T23:08:53Z

airflow/sentry.py


            sentry_flask = FlaskIntegration()

            # LoggingIntegration is set by default.
            integrations = [sentry_flask]

-            if executor_name == "CeleryExecutor":
+            executor_class, _ = ExecutorLoader.import_executor_cls(conf.get("core", "EXECUTOR"))


You can use the helper method I added for this ExecutorLoader.import_default_executor_cls()

rkarish · 2022-12-17T00:00:44Z

Thanks for the feedback Niko! I will clean this up and let you know when I am finished.

rkarish · 2022-12-18T03:48:30Z

Hey Niko, I've submitted some more changes based on the feedback and requests. I reworked the pickling support attribute, switched to using your get executor class helper method, and implemented tests for pickling and sentry support for all of the executors. CI pipeline failed so I rebased the branch off main which fixed a failure in integration tests, but it's still failing static checks. Pre-commit was passing for me locally before rebasing, seems like something changed here. When I ran it locally after rebase it was changing several files so I didn't commit those changes.

Also, I didn't mean to remove all of the other reviewers, I clicked on request re-review and it did it automatically.

…y Executor attribute (apache#27930)

…ute (apache#27930)

o-nikolas · 2023-01-04T20:27:58Z

Hey Niko, I've submitted some more changes based on the feedback and requests. I reworked the pickling support attribute, switched to using your get executor class helper method, and implemented tests for pickling and sentry support for all of the executors. CI pipeline failed so I rebased the branch off main which fixed a failure in integration tests, but it's still failing static checks. Pre-commit was passing for me locally before rebasing, seems like something changed here. When I ran it locally after rebase it was changing several files so I didn't commit those changes.

Also, I didn't mean to remove all of the other reviewers, I clicked on request re-review and it did it automatically.

Thanks @rkarish! Sorry for the delayed response but I have been on year end/holiday vacation. I'm just trying to get caught up now. I'll have a look at this PR very soon 🙏

rkarish · 2023-01-04T20:31:41Z

You're welcome and Happy New Year! I figured you were on vacation, no rush on the review!

o-nikolas

The changes look great! Perhaps @pierrejeambrun can be the second reviewer/approver, he's been very involved in the executor decoupling work.

rkarish · 2023-01-04T20:39:54Z

That would be great to get another set of eyes on this. Congratulations on becoming a Committer by the way! Would love for you to do the honors on this one 😄 .

pierrejeambrun

Just one suggestion, otherwise LGTM :)

pierrejeambrun · 2023-01-04T20:49:26Z

airflow/executors/executor_loader.py

@@ -167,10 +167,3 @@ def __load_local_kubernetes_executor(cls) -> BaseExecutor:

        local_kubernetes_executor_cls = import_string(cls.executors[LOCAL_KUBERNETES_EXECUTOR])
        return local_kubernetes_executor_cls(local_executor, kubernetes_executor)
-
-
-UNPICKLEABLE_EXECUTORS = (


Maybe add a # TODO: Remove in Airflow 3.0. We have a couple of those in the repo and will help to not miss this spot when releasing the next major.

o-nikolas · 2023-01-05T18:34:43Z

That would be great to get another set of eyes on this. Congratulations on becoming a Committer by the way! Would love for you to do the honors on this one 😄.

Thanks @rkarish! And thanks to you for helping on AIP-51 😀

rkarish requested review from dstandish, jedcunningham, kaxil, ashb and XD-DENG as code owners December 15, 2022 06:25

boring-cyborg bot added provider:cncf-kubernetes Kubernetes provider related issues area:Scheduler including HA (high availability) scheduler labels Dec 15, 2022

rkarish changed the title ~~AIP-51 - Misc. Compatibility Checks #27930~~ AIP-51 - Misc. Compatibility Checks Dec 15, 2022

o-nikolas reviewed Dec 16, 2022

View reviewed changes

rkarish requested review from o-nikolas and removed request for ashb, kaxil, XD-DENG, dstandish and jedcunningham December 17, 2022 21:58

rkarish force-pushed the apache#27930 branch from 77ec0b9 to 76f9d98 Compare December 17, 2022 22:49

rkarish added 6 commits December 25, 2022 21:28

Remove executor coupling from backfill_job.py (apache#27930)

facdaee

Remove executor coupling from scheduler_job.py (apache#27930)

af10d41

Add tests (apache#27930)

6a0aeca

Remove executor coupling from sentry.py, add tests for supports_sentr…

894d213

…y Executor attribute (apache#27930)

Fix logic in backfill_job.py (apache#27930)

d9c84fa

Use default executor helper method and rework pickling support attrib…

e6b2451

…ute (apache#27930)

rkarish force-pushed the apache#27930 branch from 76f9d98 to e6b2451 Compare December 26, 2022 02:28

Merge branch 'main' into apache#27930

f19ed3e

o-nikolas approved these changes Jan 4, 2023

View reviewed changes

o-nikolas requested a review from pierrejeambrun January 4, 2023 20:37

pierrejeambrun reviewed Jan 4, 2023

View reviewed changes

Add TODO for UNPICKLEABLE_EXECUTORS (apache#27930)

131694f

pierrejeambrun approved these changes Jan 5, 2023

View reviewed changes

o-nikolas merged commit aa4858d into apache:main Jan 5, 2023

pierrejeambrun added this to the Airflow 2.6.0 milestone Jan 9, 2023

pierrejeambrun added the type:misc/internal Changelog: Misc changes that should appear in change log label Jan 9, 2023

CoyoteLeo mentioned this pull request May 2, 2023

DB migration job fails with circular import #30988

Closed

2 tasks

potiuk mentioned this pull request May 21, 2023

Adding UIAlert in local settings fails with Sentry enabled #31442

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AIP-51 - Misc. Compatibility Checks #28375

AIP-51 - Misc. Compatibility Checks #28375

rkarish commented Dec 15, 2022

o-nikolas left a comment

o-nikolas Dec 16, 2022

o-nikolas Dec 16, 2022

rkarish Dec 16, 2022

pierrejeambrun Jan 4, 2023

o-nikolas Dec 16, 2022

rkarish Dec 16, 2022

o-nikolas Dec 16, 2022

o-nikolas Dec 16, 2022

rkarish commented Dec 17, 2022

rkarish commented Dec 18, 2022 •

edited

Loading

o-nikolas commented Jan 4, 2023

rkarish commented Jan 4, 2023

o-nikolas left a comment

rkarish commented Jan 4, 2023

pierrejeambrun left a comment

pierrejeambrun Jan 4, 2023

o-nikolas commented Jan 5, 2023

AIP-51 - Misc. Compatibility Checks #28375

AIP-51 - Misc. Compatibility Checks #28375

Conversation

rkarish commented Dec 15, 2022

o-nikolas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkarish commented Dec 17, 2022

rkarish commented Dec 18, 2022 • edited Loading

o-nikolas commented Jan 4, 2023

rkarish commented Jan 4, 2023

o-nikolas left a comment

Choose a reason for hiding this comment

rkarish commented Jan 4, 2023

pierrejeambrun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

o-nikolas commented Jan 5, 2023

rkarish commented Dec 18, 2022 •

edited

Loading