Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Airflow Standard Provider #41564

Merged
merged 31 commits into from
Sep 18, 2024
Merged

Conversation

romsharon98
Copy link
Contributor

@romsharon98 romsharon98 commented Aug 18, 2024

Extract all time operators and sensors from airflow core and remove them to standard provider
Mailing list thread: https://lists.apache.org/thread/2dmlqkcmyomm4q7rrovygs6bw655zx07


^ 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.

@romsharon98 romsharon98 changed the title Airflow Core Provider Airflow Standard Provider Aug 25, 2024
@romsharon98 romsharon98 self-assigned this Aug 25, 2024
@romsharon98 romsharon98 marked this pull request as ready for review August 25, 2024 11:16
@potiuk
Copy link
Member

potiuk commented Aug 25, 2024

One more thing - at least for now, "standard" should be added to list of preinstalled providers in hatch_build.py

PRE_INSTALLED_PROVIDERS = [
    "common.compat",
    "common.io",
    "common.sql",
    "fab>=1.0.2",
    "ftp",
    "http",
    "imap",
    "smtp",
    "sqlite",
]

@potiuk
Copy link
Member

potiuk commented Aug 25, 2024

We might change where it is added in Airflow 3, but for now it should be added here.

Copy link
Contributor

@eladkal eladkal left a comment

Choose a reason for hiding this comment

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

we are missing few things...

  1. we need to deprecate the existed operators in core
  2. we are missing all the operators and sensors we have in core (currently it's only for time operators)
  3. I am not sure what about the hooks folder?

@potiuk
Copy link
Member

potiuk commented Aug 25, 2024

  1. we need to deprecate the existed operators in core

Yes, but I think this should be done as separate PR to v2-10-test. I think we do not want to back-port that one to v2-10-test - and we should simply do it there (after we release standard as provider). I am not sure we want to back-port the whole PR here to v2-10-test (providers are not build nor tested there anyway).

  1. we are missing all the operators (currently it's only for time operators)

I think those should be separate, smaller PRs ?

  1. I am not sure what about the hooks folder?

I think it's the same ^^ - some should be moved but separately.

@gopidesupavan
Copy link
Member

Thanks @romsharon98 , will be adding rest of the operators and sensors to this.

@gopidesupavan
Copy link
Member

gopidesupavan commented Aug 25, 2024

we are missing few things...

  1. we need to deprecate the existed operators in core
  2. we are missing all the operators and sensors we have in core (currently it's only for time operators)
  3. I am not sure what about the hooks folder?

@eladkal me and romsharon discussed , i will be adding rest of the operators and sensors.

@potiuk
Copy link
Member

potiuk commented Aug 25, 2024

Ah .. so no ... We should not add it to preinstalled just yet - until we release it (sorry @romsharon98 -> see the failure on openapi client).

@eladkal
Copy link
Contributor

eladkal commented Aug 25, 2024

Yes, but I think this should be done as separate PR to v2-10-test. I think we do not want to back-port that one to v2-10-test - and we should simply do it there (after we release standard as provider). I am not sure we want to back-port the whole PR here to v2-10-test (providers are not build nor tested there anyway).

Then I propose this PR to remove the files from main that we migrated to the provider.
The PR to v2-10-test would be just to replace the classes with deprecation notes after we release the provider.

@potiuk
Copy link
Member

potiuk commented Aug 25, 2024

Then I propose this PR to remove the files from main that we migrated to the provider.

This is what the PR does.

Screenshot 2024-08-25 at 13 46 45

@eladkal
Copy link
Contributor

eladkal commented Aug 25, 2024

Ah! I didn't see changes to airflow/core folder so I assumed it was missed.
Not very friendly UX design from GitHub

@gopidesupavan
Copy link
Member

Yes, but I think this should be done as separate PR to v2-10-test. I think we do not want to back-port that one to v2-10-test - and we should simply do it there (after we release standard as provider). I am not sure we want to back-port the whole PR here to v2-10-test (providers are not build nor tested there anyway).

Then I propose this PR to remove the files from main that we migrated to the provider. The PR to v2-10-test would be just to replace the classes with deprecation notes after we release the provider.

Please correct me if i understood this, on v2-10-test deprecation updates required for all the changes we are doing on main?

@potiuk
Copy link
Member

potiuk commented Aug 25, 2024

Please correct me if i understood this, on v2-10-test deprecation updates required for all the changes we are doing on main?

No - only those that are supposed to affect DAG authors.

@potiuk
Copy link
Member

potiuk commented Aug 25, 2024

BTW. We will have to make the old "standard" providers still work in Airflow 3 I think so we will have to make the old operator imports work (likely with the mechanism mentioned by @ashb - sys.meta_path) - otherwise it will require many dags to change. I think we should apply the same mechanism as we will do for importing BaseOperator etc. in Airflow 3 (and still raise deprecation warning).

@ashb ? Do you agree ?

@potiuk
Copy link
Member

potiuk commented Aug 25, 2024

Interesting. The errors we see are very useful :).

  1. examples are part of the documentation of "main" airflow" - and I am afraid they will have to remain as examples in "airflow" not moved to provider, especially because point 2)...

  2. the examples should be embedded and shown in airflow when "examples" are enabled - I think we should not (at least not now) to develop a mechanism to read examples for Airflow from providers.

  3. The tests should also mock the moved packages.

<module 'airflow.sensors.time_delta' from '/usr/local/lib/python3.8/site-packages/airflow/sensors/time_delta.py'> does not have the attribute 'sleep'
FAILED tests/providers/standard/time/sensors/test_time_delta.py::TestTimeDeltaSensorAsync::test_wait_sensor[True] - AttributeError: <module 'airflow.sensors.time_delta' from '/u

@romsharon98
Copy link
Contributor Author

Interesting. The errors we see are very useful :).

  1. examples are part of the documentation of "main" airflow" - and I am afraid they will have to remain as examples in "airflow" not moved to provider, especially because point 2)...
  2. the examples should be embedded and shown in airflow when "examples" are enabled - I think we should not (at least not now) to develop a mechanism to read examples for Airflow from providers.
  3. The tests should also mock the moved packages.
<module 'airflow.sensors.time_delta' from '/usr/local/lib/python3.8/site-packages/airflow/sensors/time_delta.py'> does not have the attribute 'sleep'
FAILED tests/providers/standard/time/sensors/test_time_delta.py::TestTimeDeltaSensorAsync::test_wait_sensor[True] - AttributeError: <module 'airflow.sensors.time_delta' from '/u

revert the example dags to be under main airflow.
should I add a link in index.yaml for those example dags?

also not sure but should I change somewhere in the code to tell that this provider is mandatory and will auto installed?

@potiuk
Copy link
Member

potiuk commented Aug 25, 2024

also not sure but should I change somewhere in the code to tell that this provider is mandatory and will auto installed?

That's the preinstalled_providers - but it will only work after it's been released and it will likely change with Airflow 3 so for now - no need.

@romsharon98 romsharon98 merged commit 20ea6b7 into apache:main Sep 18, 2024
108 checks passed
joaopamaral pushed a commit to joaopamaral/airflow that referenced this pull request Oct 21, 2024
* add core.time provider

* add source-date-epoch

* change core to essentials

* revert external task sensor location

* add provider to airflow_providers_bug_report list

* change new provider name to standard

* change changelog

* add integration

* add standard provider to pre-install list

* revert hatch_build

* move examples back to airflow core

* fixing mock

* add title to commit

* fix time.rst

* change sensors example dags paths

* revert example get_test_run addition

* remove init

* revert howto docs

* fix docs

* update standard provider airflow minimum version, remove standarad from python 2.8 and 2.9 competability

* change provider as not-ready

* add changelog

* move how to docs to provider

* fix docs images

* fix import error

* revert global constant

* change time_sensors.py to time.py

* revert min version to be 2.10

* remove providers from 2.9 and 2.8

* revert global constant

* add operators guid to provider yaml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants