Remove AIP-44 configuration from the code #44454
Conversation
potiuk
requested review from
XD-DENG,
ashb,
bbovenzi,
bolkedebruin,
hussein-awala,
jscheffl,
o-nikolas,
pierrejeambrun and
ryanahamilton
as code owners
boring-cyborg
bot
added
area:CLI
area:core-operators
area:dev-tools
area:Executors-core
potiuk
force-pushed
the
remove-aip-44-configuration
branch
from
75c243a to
a071c24
Compare
This change removes all configuration that controls AIP-44 behaviour. It does not yet remove all the related code, this will be a follow up but it removes all the controls that determine if AIP-44 is enabled or not and removes all the Traceback Session/Disabling of DB session modifications that were used in "database isolation" mode. The "database isolation" mode has been disabled in apache#44441 so there was no easy way to enable it anyoway - this change removes the capability to use database isolation mode completely. Part of apache#44436
potiuk
force-pushed
the
remove-aip-44-configuration
branch
from
a071c24 to
0819cf3
Compare
|
K8S tests failure unrelated (work in progress to fix them). Merging. |
Ruff team is using Airflow as one of the "ecosystem" benchmarks for new rules they developed and reported the issues to us in the apache#44455 discussion. This PR addresses those issues that are still there (some of the reported tests were removed earlier today when we removed AIP-44 code in apache#44454
| os.environ["AIRFLOW_ENABLE_AIP_44"] = "True" | ||
| os.environ["AIRFLOW__CORE__INTERNAL_API_URL"] = api_url | ||
| InternalApiConfig.set_use_internal_api("edge-worker") |
There was a problem hiding this comment.
Args. This was a bit tooo much removal ... because if this removed the start now fails in airflow/executors/executor_loader.py:validate_database_executor_compatibility() with error: cannot use SQLite with the...
There was a problem hiding this comment.
But should not edge worker be independent of database used?
There was a problem hiding this comment.
Yes, it should. Theproblem is that in Airflow 2.10 there is a bit of validation at point of CLI parsing which impliocitly invokes ExecutorLoader and this indirectly checks for which executor is used and this is compatible in regards of SQLite. This patching was needed to work-around these validations.
As of the removal - which totally make sense in main/Airflow 3 - the entrypoint was broken in Airflow 2.10 which we do not clean or repair at this stage. Scratched my head 2 hours yesterday trying to make it / restore it such that it is working with Airflow 2.10 again.
I did not complete a deployment test with Airflow 2.10 and Edge Executor, really running a CLI worker in the CI together with the server... individually all Unit tests are working but you see how it is broken if you call breeze down && breeze release-management prepare-provider-packages --include-not-ready-providers edge && breeze start-airflow --python 3.12 --load-example-dags --backend postgres --executor EdgeExecutor --answer y --use-airflow-version 2.10.3 --use-packages-from-dist
There was a problem hiding this comment.
See #44494 (comment)
I think we are technically at the FORK time. This is the time when we know Edge executor in main will not work in 2.10. And if you need to make it works, forking from "just before we started to remove AIP-44" by Bosh team and backporting the main changes to edge executor to make it works for "2.10" version maintained by Bosh is the right thing to do.
There was a problem hiding this comment.
I hope we are not the only ones using... and I hope we don't need to fork. At least now with #44434 I made it working again. Added a layer to abstact the backend and hope that we can make it working with AIP-72 as well. I do not see much redundancy and atm I see it beneficial keeping one source w/o fork.
Anyway, will now pick some tasks and help cleaning the AIP-44 beach :-D
There was a problem hiding this comment.
Yeah. If we can fix it ... Cool :)
This change removes all configuration that controls AIP-44 behaviour. It does not yet remove all the related code, this will be a follow up but it removes all the controls that determine if AIP-44 is enabled or not and removes all the Traceback Session/Disabling of DB session modifications that were used in "database isolation" mode. The "database isolation" mode has been disabled in apache#44441 so there was no easy way to enable it anyoway - this change removes the capability to use database isolation mode completely. Part of apache#44436
…e#44460) Ruff team is using Airflow as one of the "ecosystem" benchmarks for new rules they developed and reported the issues to us in the apache#44455 discussion. This PR addresses those issues that are still there (some of the reported tests were removed earlier today when we removed AIP-44 code in apache#44454
5 participants
This change removes all configuration that controls AIP-44 behaviour. It does not yet remove all the related code, this will be a follow up but it removes all the controls that determine if AIP-44 is enabled or not and removes all the Traceback Session/Disabling of DB session modifications that were used in "database isolation" mode. The "database isolation" mode has been disabled in #44441 so there was no easy way to enable it anyoway - this change removes the capability to use database isolation mode completely.
Part of #44436
^ 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.rstor{issue_number}.significant.rst, in newsfragments.