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

Move default_celery.py to inside the provider #32628

Merged
merged 1 commit into from
Jul 16, 2023

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Jul 15, 2023

This has been missed in #32526 and is extracted out from #32604 in an attempt to make it smaller and separately reviewable.

This one adds also deprecation warning to handle the configuration value that people might already have in the [celery] iccelery_config_options"


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

Copy link
Member

@hussein-awala hussein-awala left a comment

Choose a reason for hiding this comment

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

Nit

Copy link
Member

Choose a reason for hiding this comment

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

Personally, I am not a fan of relying on Airflow configurations from providers. The reason for my stance is that providers can be installed on various versions of Airflow, and if we add a new feature relies on a new configuration, it may not be supported by older versions of Airflow. IMHO, it would be beneficial to explore alternative methods for configuring executors with a minimal usage of Airflow configurations.

Copy link
Member Author

@potiuk potiuk Jul 16, 2023

Choose a reason for hiding this comment

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

Personally, I am not a fan of relying on Airflow configurations from providers. The reason for my stance is that providers can be installed on various versions of Airflow, and if we add a new feature relies on a new configuration, it may not be supported by older versions of Airflow. IMHO, it would be beneficial to explore alternative methods for configuring executors with a minimal usage of Airflow configurations.

I believe when you look in detail to where #32604 is heading, you will see the goal achieved and you will see that what we got there, we achieve everything you wanted, without breaking compatibility with the current system and with 100% reuse of what we already have as various mechanisms where configuration can be set.

I think this is actually the goal of #32604 when (after I extract various parts of it it to separate PRs and merge them, we are heading up. It's not by introducing a different mechanism for providers, but by making sure that the configuration they use is tied with the provider version not airflow version

Generally speaking, in providers we will only use the "framework" from airfow. basically confg.get() which is supported by any and all versions of Airflow and the whole mechanism of being able to set it via various mechanisms: file, env, secret backend etc.. Everything else will be separately controlled in each provider.

What #32604 does (and very gently without breaking any compatiblities).

  • Each provider might be able to use the set of configurations (group/param) that they are bound to and use - and will be able to separately add or remove configuraiton in whatever provider (not Airflow) version. So provider, for each version will use the same set of parameters - regardless from the version of Airflow it is installed in.

  • Each provider version will have embedded (as provider.yaml/get_provider_info entrypoint) description of the configuration they support. Each version of provider will have their own variant of it. If you add configuration in version 3.1.0, version 3.0.0 will not have/use it

  • Each provider which has its own configuration will have their own dedicated configuration page (see screnshots in Allow configuration to be contributed by providers #32604). And it also will be versioned. You will be able to switch the version and you will be able to see which set of configuration parameters is supported by each version.

  • You will even be able to see version_added there (which will be version of the provider, not airflow)

  • Those provider configuration variables will NOT be shown at the main https://airflow.apache.org/docs/apache-airflow/stable/configurations-ref.html - they will be linked from there via https://airflow.apache.org/docs/apache-airflow-providers/core-extensions/index.html (there will be dedicated page which will show which providers have configurations and link to their respective (stable) versions - but from there you will be able to see historically how configuration parameter changed for that provider.

Copy link
Member Author

Choose a reason for hiding this comment

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

BTW. This is the goal of #32604 mostly - to show where we are heading so that we can see why certain steps are executed as intermediate ones.

@potiuk potiuk added the full tests needed We need to run full set of tests for this PR to merge label Jul 16, 2023
@potiuk potiuk force-pushed the move-default-config-for-celery branch from 7fc2512 to 5d17bbd Compare July 16, 2023 05:39
@potiuk
Copy link
Member Author

potiuk commented Jul 16, 2023

BTW. Re-running with full tests needed -> the problem with "build docs" should be gone when both airflow + celery docs are rebuilt (I think it was because airflow's index came from inventory and celery ones from sourceS).

@potiuk
Copy link
Member Author

potiuk commented Jul 16, 2023

And @hussein-awala -> if you have any doubts about it, I am super happy to explain and alleviate any concerns with that one. I really think that when you look at the end-goal you will see that we could achieve what you want without braeking any compatiblity and we will achieve full config-independence by providers.

@potiuk potiuk removed the full tests needed We need to run full set of tests for this PR to merge label Jul 16, 2023
This has been missed in apache#32526 and is extracted out from apache#32604
in an attempt to make it smaller and separately reviewable.

This one adds also deprecation warning to handle the
configuration value that people might already have in
the [celery] iccelery_config_options"
@potiuk potiuk force-pushed the move-default-config-for-celery branch from 5d17bbd to 1520b6a Compare July 16, 2023 06:55
@potiuk
Copy link
Member Author

potiuk commented Jul 16, 2023

Nope. Different problem (needed small refactor in retrieving the ssl value as Sphinx thought it has been defined in two places at top-level of the configuration

@potiuk potiuk merged commit ea0deaa into apache:main Jul 16, 2023
@potiuk potiuk deleted the move-default-config-for-celery branch July 16, 2023 07:20
@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Aug 2, 2023
@ephraimbuddy ephraimbuddy added this to the Airflow 2.7.0 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) provider:celery
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants