Skip to content

Conversation

@kaxil
Copy link
Member

@kaxil kaxil commented Apr 8, 2025

Not everyone will want OL installed.


^ 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 airflow-core/newsfragments.

@kaxil kaxil requested review from ashb, eladkal and potiuk April 8, 2025 14:59
Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

I guess we should add newsfragment about it - it changes the behaviour of pip install apache-airflow

@kaxil
Copy link
Member Author

kaxil commented Apr 8, 2025

I guess we should add newsfragment about it - it changes the behaviour of pip install apache-airflow

OL wasn't installed in 2.10.x though

@potiuk
Copy link
Member

potiuk commented Apr 8, 2025

Newsfragment - when it comes to SQLite at least.

Also it might break lowest dependency tests for Airflow core - I believe one reason I added it was that Airflow core was at that time some tests or even all tests failed. Maybe things changed since, but I think not.

Let's see how it works, but:

  1. in case airflow-core has some imports, they should be made optional if possible.
  2. In case tests have some imports - we can move the OpenLineage provider to "dev" dependency group.

@potiuk
Copy link
Member

potiuk commented Apr 8, 2025

Yeah - the first failure is aiosqlite not importable in airflow tests - this can be solved by adding apache-airflow-providers-sqlite to dev dependency group.

@potiuk
Copy link
Member

potiuk commented Apr 8, 2025

(And let's see what fails next)

@eladkal
Copy link
Contributor

eladkal commented Apr 8, 2025

Correct me if I am wrong but we removed SequentialExecutor and right now Sqlite can not work with LocalExecutor so the breaking change already happened isn't it? (We had a plab for Sqlite to work with LocalExecutor but I don't remember it was done)

@kaxil
Copy link
Member Author

kaxil commented Apr 8, 2025

Correct me if I am wrong but we removed SequentialExecutor and right now Sqlite can not work with LocalExecutor so the breaking change already happened isn't it? (We had a plab for Sqlite to work with LocalExecutor but I don't remember it was done)

Yeah that's done. But even that does not need anything in the Sqlite provider to work. It just has:

dependencies = [
"apache-airflow>=2.9.0",
"aiosqlite>=0.20.0",
"apache-airflow-providers-common-sql>=1.20.0",
]

Not everyone will want OL installed.
@kaxil kaxil force-pushed the remove-preinstalled branch from e86b05d to d2060c2 Compare April 8, 2025 15:29
@kaxil kaxil changed the title Remove Openlineage & Sqlite from list of pre-installed providers Remove Openlineage from list of pre-installed providers Apr 8, 2025
@kaxil
Copy link
Member Author

kaxil commented Apr 8, 2025

Separating Sqlite removal to separate PR

@kaxil kaxil merged commit 6203e23 into apache:main Apr 8, 2025
49 checks passed
@kaxil kaxil deleted the remove-preinstalled branch April 8, 2025 16:14
@ashb
Copy link
Member

ashb commented Apr 8, 2025

and right now Sqlite can not work with LocalExecutor

@eladkal Not true anymore, I fixed it a few months back.

@kaxil
Copy link
Member Author

kaxil commented Apr 8, 2025

in #44839

@potiuk
Copy link
Member

potiuk commented Apr 8, 2025

The sqlite provider was never technically needed to use SQLite as meta-data. Those two have nothing to do with each other - provider simply create a DBAPI Hook so that SQLite (but not the one that has airflow metadata) can be used via common.sql operators.

The SQlite database is part of stdlib and the only depedency extra it has is "aiosqlite" -> Which BTW I think this is somewhat wrong - aiosqlite should be added to the core. I commented it in #48950

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants