Skip to content

Conversation

@o-nikolas
Copy link
Contributor

@o-nikolas o-nikolas commented Feb 20, 2025

In preparation of deprecating support in Airflow core for the old hardcoded hybrid executors in V3, mark them as deprecated and emit warnings when they are loaded. Also mark multi exec as stable to give users somewhere to migrate towards.

To broadcast the message of their deprecation this PR:
- Adds a deprecation warning when these old hybrid executors are loaded
- Documentation has been updated to note that they are deprecated and provide steps for migration.
- A news fragment was added as well.

The warning message:
Screenshot from 2025-02-20 11-57-47

Rendered docs:
Screenshot from 2025-02-20 13-54-20


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

Multiple Executor Configuration (aka hybrid executors) has been released
for over half a year and can be marked stable. This gives users an
option to migrate to from the old hardcoded hybrid executors (e.g
CeleryKubernetesExecutor)
These executors will be removed in Airflow 3.0. Mark them as deprecated
so users are aware and have time to migrate.

- A deprecation warning will be printed when the executors are loaded
- Documentation has been updated to note that they are deprecated
- A news fragment added as well.
@o-nikolas
Copy link
Contributor Author

CC: @jedcunningham @jscheffl @amoghrajesh

PR to mark the static executors as deprecated in Airflow core and mark multi-exec as stable. Re: https://lists.apache.org/thread/qmdf5dx71lzcvf4fd7qyc9gq0z0j2jqr

Copy link
Contributor

@jscheffl jscheffl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good in my view

o-nikolas and others added 3 commits February 20, 2025 14:58
Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
@o-nikolas
Copy link
Contributor Author

Getting the tests to ignore the deprecation warning at the right time has proven very annoying/tricky.

The last stubborn one is related to one instances of the warning not being thrown, but it seems that some underlying failure is actually happening, but I'm not sure why it's happening on this branch that should be otherwise stable. And I also cannot produce it locally. From the test failure logs:

                with pytest.warns(RemovedInAirflow3Warning):
>                   file_handler.read(ti)
E                   Failed: DID NOT WARN. No warnings of type (<class 'airflow.exceptions.RemovedInAirflow3Warning'>,) were emitted.
E                    Emitted warnings: [].

tests/utils/test_log_handlers.py:289: Failed
---------------------------- Captured stdout setup -----------------------------
[2025-02-21T01:46:21.370+0000] {dagbag.py:588} INFO - Filling up the DagBag from /dev/null
----------------------------- Captured stdout call -----------------------------
[2025-02-21T01:46:21.386+0000] {dag.py:3239} INFO - Sync 1 DAGs
[2025-02-21T01:46:21.391+0000] {dag.py:3262} INFO - Creating ORM DAG for dag_for_testing_multiple_executors
[2025-02-21T01:46:21.397+0000] {dag.py:4180} INFO - Setting next_dagrun for dag_for_testing_multiple_executors to 2016-01-01T00:00:00+00:00, run_after=2016-01-02T00:00:00+00:00
[2025-02-21T01:46:21.436+0000] {file_task_handler.py:623} ERROR - Could not read served logs
Traceback (most recent call last):
  File "/opt/airflow/airflow/utils/log/file_task_handler.py", line 599, in _read_from_logs_server
    log_type = LogType.TRIGGER if ti.triggerer_job else LogType.WORKER
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/ext/associationproxy.py", line 193, in __get__
    return inst.get(obj)
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/ext/associationproxy.py", line 575, in get
    target = getattr(obj, self.target_collection)
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/orm/attributes.py", line 487, in __get__
    return self.impl.get(state, dict_)
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/orm/attributes.py", line 959, in get
    value = self._fire_loader_callables(state, key, passive)
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/orm/attributes.py", line 995, in _fire_loader_callables
    return self.callable_(state, passive)
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/orm/strategies.py", line 863, in _load_for_state
    raise orm_exc.DetachedInstanceError(
sqlalchemy.orm.exc.DetachedInstanceError: Parent instance <TaskInstance at 0x7fe810c4d6d0> is not bound to a Session; lazy load operation of attribute 'trigger' cannot proceed (Background on this error at: https://sqlalche.me/e/14/bhk3)

Copy link
Contributor

@amoghrajesh amoghrajesh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

We already have coverage for this behaviour with the other executor
@eladkal
Copy link
Contributor

eladkal commented Feb 24, 2025

Mark the hardcoded hybrid executors

We do need to actually deprecate them in the providers code don't we?

@@ -0,0 +1,3 @@
Statically-coded executors are now deprecated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need here also announcement that hybrid executor feature is now stable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. I've updated the newsfragment to make that more clear!

@o-nikolas
Copy link
Contributor Author

Mark the hardcoded hybrid executors

We do need to actually deprecate them in the providers code don't we?

That one is less of a AF3 time crunch as it can be done at any time, and I actually also think it's fine to allow the provider to improve and support those executors into the near future. Since many people will continue to use them on AF2 for a long time to come. But yes, eventually they will need to be deprecated there as well as the user base shrinks.

@jscheffl
Copy link
Contributor

FYI I assume the failed CI run goes away on re-base... or as it is on v2-10-test you might need to cherry-pick #46980

Note I just realize that the PR target is v2-10-stable, I think this should rather be directed to v2-10-test!

@o-nikolas o-nikolas merged commit 12a124a into apache:v2-10-stable Feb 24, 2025
53 of 57 checks passed
@o-nikolas o-nikolas deleted the onikolas/v2-10-stable/deprecate-static-hybrid-execs branch February 24, 2025 22:04
@o-nikolas
Copy link
Contributor Author

FYI I assume the failed CI run goes away on re-base... or as it is on v2-10-test you might need to cherry-pick #46980

Yupp, we should cherry-pick your change into v2-10-test

Note I just realize that the PR target is v2-10-stable, I think this should rather be directed to v2-10-test!

Also, yup, I accidentally targeted the wrong branch 🤦 I just chatted with @jedcunningham about it today, he's going to handle the mismatch during the next release.

@o-nikolas
Copy link
Contributor Author

@jscheffl I added the backport-to-v2-10-test label to your PR to have it cherry picked back

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Executors-core LocalExecutor & SequentialExecutor kind:documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants