You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "potiuk (via GitHub)" <gi...@apache.org> on 2023/07/14 13:50:41 UTC
[GitHub] [airflow] potiuk opened a new pull request, #32604: Allow configuration to be contributed by providers
potiuk opened a new pull request, #32604:
URL: https://github.com/apache/airflow/pull/32604
The changes implemented:
* provider.yaml files for providers can optionally contribute extra configuration, the configuration is exposed via "get_provider_info" entrypoint, thus allowing Airflow to discover the configuration from both - sources (in Breeze and local development) and from installed packages
* Documentation for configuration contributed by providers is generated as part of Provider documentation. It is also discoverable by having a "core-extension" page displaying all community providers that contribute their own configuration.
* Celery configuration (and in the future Kubernetes configuration) is linked directly from the airflow documentation - the providers are preinstalled, which means that celery (and Kubernetes in the future) configuration is considered as important to be directly mentioned and linked from the core. Similarly Celery and Kubernetes executor documentation remains in the core documentation (still configuration options are detailed only in the provider documentation and only linked from the core.
* the `airflow config list` command becomes more powerful, with new flags showing optionally decriptions, examples, environment variables corresponding to the commands, showing sources where the configuration value comes from (including provider information and also allowing to show just default values - which nicely replaces the "default_airflow.cfg"
In order to that, howver, some crucial changes had to be done to refactor the code for configuration retrieval.
This change refactors configuration retrieval quite heavily in order to allow the default configuration to be contributed by providers.
It simplifies and removes some of the historical internals of configuration reading and writing, removing unnecessary and unused code and finally replacing the mechanism where we kept configuration description in both yaml files and pre-rendered template files, thus removing the need of keeping the rendered configuration in the airflow package.
* config.yml becomes the only source of truth of default configuration values and configuration description (including comments) for Airflow core
* default_airflow.cfg and default_test.cfg templates are deleted and default_airflow.cfg content is replaced with comments explaining how to get default config (with airflow config defaults command). Those templates were confusing because they almost looked like airflow configuration but they weren't (they contained JINJA directives)
* unittest.cfg is not used any more - test configuration is dynamically generated when tests are starting with pytest fixture. The source of truth for test configuration is config_unit_test.yml
* removed airflow_ci.cfg which was yet another configuration file used only in CI - yet another source of confusion. All the test configuration needed is now part of config_unit_tests.yml. Thanks to that, unit tests for configuration work now in CI and local venv without having to set special environment variables or configuration
* old methods of processing the default_*.cfg and writing the config have been removed as we do not have the JINJA templates any more for those.
* the code that generates documentation from configuration has been refactored to avoid unnecessary repetitions
* secret configuration tests using Hashicorp have been moved to Hashicorp provider, thanks to that we can run config tests without installing hvac.
* there were test covering type coertion for configuration - they were using "kerberos" configuration but thanks to config_unit_tests.yml approach, converted them to "abstract" tests
* moved default_celery.py configuration to provider (this is a remnant of the celery executor moving to provider. There is no need for backwards compatibility and PEP 562, it is only used by Celery* executors.
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
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](https://github.com/apache/airflow/tree/main/newsfragments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ferruzzi commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1265783122
##########
docs/apache-airflow-providers/index.rst:
##########
@@ -51,6 +51,15 @@ provider package and re-start Airflow, those become automatically available to A
The summary of the core functionalities that can be extended are available in
:doc:`/core-extensions/index`.
+Configuration
+'''''''''''''
+
+The providers can have their own configuration options, That allow you to configure the way how they
+work:
+
+You can see all the community-managed providers with their own configuration in
Review Comment:
```suggestion
You can see all community-managed providers with their own configuration in
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1266214514
##########
airflow/config_templates/config_unit_tests.yml:
##########
@@ -0,0 +1,221 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# Defaults specified in this file replace the default values from
+# config.yml file during unit tests. If you want a value to have
+# different default during unit tests, specify it here.
+
+---
+
+core:
+ description: ~
+ options:
+ dags_folder:
+ description: |
+ The folder where your airflow pipelines live, most likely a
+ subfolder in a code repository. This path must be absolute.
+ version_added: ~
+ type: string
+ example: ~
+ default: "{AIRFLOW_HOME}/dags"
+ executor:
+ description: |
+ The executor class that airflow should use. Choices include
+ ``SequentialExecutor``, ``LocalExecutor``, ``CeleryExecutor``, ``DaskExecutor``,
+ ``KubernetesExecutor``, ``CeleryKubernetesExecutor`` or the
+ full import path to the class when using a custom executor.
+ version_added: ~
+ type: string
+ example: ~
+ default: "LocalExecutor"
+ dags_are_paused_at_creation:
+ description: |
+ Are DAGs paused by default at creation
+ version_added: ~
+ type: string
+ example: ~
+ default: "False"
+ load_examples:
+ description: |
+ Whether to load the DAG examples that ship with Airflow. It's good to
+ get started, but you probably want to set this to ``False`` in a production
+ environment
+ version_added: ~
+ type: string
+ example: ~
+ default: "True"
+ fernet_key:
+ description: |
+ Secret key to save connection passwords in the db
+ version_added: ~
+ type: string
+ sensitive: true
+ example: ~
+ default: "{FERNET_KEY}"
+ donot_pickle:
+ description: |
+ Whether to disable pickling dags
+ version_added: ~
+ type: string
+ example: ~
+ default: "False"
+ default_impersonation:
+ description: |
+ If set, tasks without a ``run_as_user`` argument will be run with this user
+ Can be used to de-elevate a sudo user running Airflow when executing tasks
+ version_added: ~
+ type: string
+ example: ~
+ default: ""
+ unit_test_mode:
+ description: |
+ Turn unit test mode on (overwrites many configuration options with test
+ values at runtime)
+ version_added: ~
+ type: string
+ example: ~
+ default: "True"
+database:
+ description: ~
+ options:
+ sql_alchemy_conn:
+ description: |
+ The SqlAlchemy connection string to the metadata database.
+ SqlAlchemy supports many different database engines.
+ More information here:
+ http://airflow.apache.org/docs/apache-airflow/stable/howto/set-up-database.html#database-uri
+ version_added: 2.3.0
+ type: string
+ sensitive: true
+ example: ~
+ default: "sqlite:///{AIRFLOW_HOME}/airflow.db"
+ load_default_connections:
+ description: |
+ Whether to load the default connections that ship with Airflow. It's good to
+ get started, but you probably want to set this to ``False`` in a production
+ environment
+ version_added: 2.3.0
+ type: string
+ example: ~
+ default: "True"
+logging:
+ description: ~
+ options:
+ base_log_folder:
+ description: |
+ The folder where airflow should store its log files.
+ This path must be absolute.
+ There are a few existing configurations that assume this is set to the default.
+ If you choose to override this you may need to update the dag_processor_manager_log_location and
+ dag_processor_manager_log_location settings as well.
+ version_added: 2.0.0
+ type: string
+ example: ~
+ default: "{AIRFLOW_HOME}/logs"
+hive:
+ description: ~
+ options:
+ default_hive_mapred_queue:
+ description: |
+ Default mapreduce queue for HiveOperator tasks
+ version_added: ~
+ type: string
+ example: ~
+ default: "airflow"
+smtp:
+ description: |
+ If you want airflow to send emails on retries, failure, and you want to use
Review Comment:
Same here. I will not correct those YET. Those will come as separate change
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1269951356
##########
airflow/__main__.py:
##########
@@ -45,6 +45,13 @@ def main():
parser = cli_parser.get_parser()
argcomplete.autocomplete(parser)
args = parser.parse_args()
+
+ # Here we ensure that the default configuration is written if needed before running any command
+ # that might need it. This used to be done during configuration initialization but having it
+ # im main ensures that it is not done during tests and other ways airflow imports are used
Review Comment:
```suggestion
# in main ensures that it is not done during tests and other ways airflow imports are used
```
##########
docs/providers-configurations-ref.rst:
##########
Review Comment:
Since this is basically the same as the core page, should we add comments to remind folks to keep them in sync? Is there a way we can reuse it instead of duplicating it?
##########
docs/providers-configurations-ref.rst:
##########
@@ -0,0 +1,99 @@
+ .. Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ .. http://www.apache.org/licenses/LICENSE-2.0
+
+ .. Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+
+.. jinja:: config_ctx
+
+ Configuration Reference
+ .......................
+
+ This page contains the list of all available Airflow configurations for the
+ ``{{ package_name }}`` provider that can be set in ``airflow.cfg`` file or using environment variables.
Review Comment:
```suggestion
``{{ package_name }}`` provider that can be set in the ``airflow.cfg`` file or using environment variables.
```
##########
docs/providers-configurations-ref.rst:
##########
@@ -0,0 +1,99 @@
+ .. Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ .. http://www.apache.org/licenses/LICENSE-2.0
+
+ .. Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+
+.. jinja:: config_ctx
+
+ Configuration Reference
+ .......................
+
+ This page contains the list of all available Airflow configurations for the
+ ``{{ package_name }}`` provider that can be set in ``airflow.cfg`` file or using environment variables.
+
+ .. note::
+ For more information on setting the configuration, see :doc:`apache-airflow:howto/set-config`
Review Comment:
```suggestion
For more information see :doc:`apache-airflow:howto/set-config`.
```
I think this would render better: "For more information see <Setting Configuration Options>."
##########
docs/apache-airflow/configurations-ref.rst:
##########
@@ -32,6 +32,13 @@ generated using the secret key has a short expiry time though - make sure that t
that you run airflow components on is synchronized (for example using ntpd) otherwise you might get
"forbidden" errors when the logs are accessed.
+Some of the providers have their own configuration options, you will find details of their configuration
+in the provider's documentation. The pre-installed providers that you likely want to configure are:
Review Comment:
```suggestion
in the provider's documentation. The pre-installed providers that you may want to configure are:
```
I think we should soften this, 2 of the 3 common executors wouldn't need to touch this.
##########
airflow/configuration.py:
##########
@@ -1644,6 +1765,61 @@ def remove_all_read_configurations(self):
for section in self.sections():
self.remove_section(section)
+ @property
+ def providers_configuration_loaded(self) -> bool:
+ """Checks if providers have been loaded."""
+ return self._providers_configuration_loaded
+
+ def load_providers_configuration(self):
+ """
+ Loads configuration for providers.
+
+ This should be done after initial configuration have been performed. Initializing and discovering
+ providers is an expensive operation and cannot be performed when we load configuration for the first
+ time when airflow starts, because we initialize configuration very early, during importing of the
+ `airflow` package and the module is not yet ready to be used when it happens and until configuration
+ and settings are loaded. Therefore, in order to reload provider configuration we need to additionally
+ load provider - specific configuration.
+ """
+ log.debug("Loading providers configuration")
+ from airflow.providers_manager import ProvidersManager
+
+ for provider, config in ProvidersManager().already_initialized_provider_configs:
+ for provider_section, provider_section_content in config.items():
+ provider_options = provider_section_content["options"]
+ section_in_current_config = self.configuration_description.get(provider_section)
+ if not section_in_current_config:
+ # merge whole sections into existing config
+ self.configuration_description[provider_section] = deepcopy(provider_section_content)
+ section_in_current_config = self.configuration_description.get(provider_section)
+ for option in provider_options:
+ section_in_current_config["options"][option]["source"] = f"default-{provider}"
+ else:
+ for option in provider_options:
+ option_in_current_config = section_in_current_config["options"].get(option)
+ if not option_in_current_config:
+ # merge new options into existing sections
+ self.configuration_description[provider_section]["options"][option] = deepcopy(
+ provider_options[option]
+ )
+ option_in_current_config = section_in_current_config["options"].get(option)
+ option_in_current_config["source"] = f"default-{provider}"
+ else:
+ # replace default values for existing options
Review Comment:
Yeah, this feels wrong to me.
Is this not a race condition too, if more than 1 provider decided to override a certain value?
I'd rather we raise here.
##########
airflow/cli/commands/config_command.py:
##########
@@ -48,6 +49,14 @@ def show_config(args):
def get_value(args):
"""Get one value from configuration."""
+ # while this will make get_value quite a bit slower we must initialize configuration
+ # for providers because we do not know what sections and options will be available after
+ # providers are initialized. Theoretically Providers might add new sections and options
+ # but also override defaults for existing options, so without loading all providers we
Review Comment:
Allowing providers to override core defaults feels risky... Do we really need that behavior?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32604:
URL: https://github.com/apache/airflow/pull/32604#issuecomment-1638232431
> I like the approach, it definitely will be cleaner to have provider-related config in provider itself. Getting rid of confusing redundancy in default_airflow.cfg is also nice.
Yeah. I am just about to extract (and rather improve) the "gist" of the config refactor (I had to make few more fixes it turned out and come back to storing the defaults in the ConfigParser due to interpolation issues, but it's coming along rather nicely - three of extracted PRs for this one have been merged already. I will have two more : "config" refactor (with removal of templates) and adding "providers" mechanism to it. Stay tuned.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1270702788
##########
docs/providers-configurations-ref.rst:
##########
Review Comment:
:sweat: I think I got it.... Indentation wrong in Sphinx makes me crazy :sweat:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1270866893
##########
airflow/cli/commands/config_command.py:
##########
@@ -48,6 +49,14 @@ def show_config(args):
def get_value(args):
"""Get one value from configuration."""
+ # while this will make get_value quite a bit slower we must initialize configuration
+ # for providers because we do not know what sections and options will be available after
+ # providers are initialized. Theoretically Providers might add new sections and options
+ # but also override defaults for existing options, so without loading all providers we
Review Comment:
@jedcunningham -> saw your comment in slack. And yeah, agree that we shoudl "fail hard" if somoene attemps to override a section. Changing it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32604:
URL: https://github.com/apache/airflow/pull/32604#issuecomment-1644367502
I also did one more thing - since we are modifying "providers" documentation quite heavily here, I noticed we had a lot of old/missing/outdated/badly structured documentation (for example we really had airflow community provider developer inter-mixed with "user" documentaion - and no clear page describing how to make custom providers. So I refactored the documentation slightly, added missing documentation about custom provider features - so the documentation there is much more consistent now across the board.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1266215228
##########
docs/exts/logging.rst.jinja2:
##########
@@ -22,7 +22,7 @@
:doc:`{{ provider }}:logging/index`
-{% for handler in provider_dict['handlers'] -%}
+{% for handler in provider_dict['logging'] -%}
Review Comment:
I **think** it is right as it is, but I will check it when I rebase the final PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ferruzzi commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1265784486
##########
docs/exts/logging.rst.jinja2:
##########
@@ -22,7 +22,7 @@
:doc:`{{ provider }}:logging/index`
-{% for handler in provider_dict['handlers'] -%}
+{% for handler in provider_dict['logging'] -%}
Review Comment:
I think this is right, but just double-checking this wasn't changed by accident.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1266171404
##########
airflow/providers/celery/executors/celery_kubernetes_executor.py:
##########
@@ -55,14 +55,21 @@ class CeleryKubernetesExecutor(LoggingMixin):
callback_sink: BaseCallbackSink | None = None
- KUBERNETES_QUEUE = conf.get("celery_kubernetes_executor", "kubernetes_queue")
+ @property
Review Comment:
We should not do it straight away, because we can reload the configuration - we would have to remove cached property after reload and I think that would be a bit too much of a coupling. But I will see what I can do in the final version (I am already reloading sensitive variables list in a similar way).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #32604: Allow configuration to be contributed by providers
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #32604:
URL: https://github.com/apache/airflow/pull/32604#issuecomment-1643450256
I don’t have particular issues with this, but have some reservations toward allowing any provider to contribute any configuration. Not sure if I missed this being discussed elsewhere. It’d be a good idea to make these specific configs an exception for convenience (and compatibility), but making the feature universally available may have weird implications.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1269502973
##########
airflow/configuration.py:
##########
@@ -1732,6 +1787,31 @@ def create_default_config_parser(configuration_description: dict[str, dict[str,
return parser
+def write_default_airflow_configuration_if_needed():
+ if not os.path.isfile(AIRFLOW_CONFIG):
+ import rich
+
+ rich.print("Creating new Airflow config file in: %s", AIRFLOW_CONFIG)
Review Comment:
Yes. To make it nicely coloured..
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1269524486
##########
airflow/providers/celery/provider.yaml:
##########
@@ -60,3 +60,256 @@ sensors:
executors:
- airflow.providers.celery.executors.celery_executor.CeleryExecutor
- airflow.providers.celery.executors.celery_kubernetes_executor.CeleryKubernetesExecutor
+
+config:
+ celery_kubernetes_executor:
+ description: |
+ This section only applies if you are using the ``CeleryKubernetesExecutor`` in
+ ``[core]`` section above
Review Comment:
I also corrected all those missing ".", some inconsistencies. and reformatted the descriptions slightly. For now only in celary part - but later will do it as well for others that I will extract CC: @ferruzzi.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] vincbeck commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1269488082
##########
airflow/configuration.py:
##########
@@ -151,15 +151,31 @@ def _default_config_file_path(file_name: str) -> str:
return os.path.join(templates_dir, file_name)
-def retrieve_configuration_description() -> dict[str, dict[str, Any]]:
+def retrieve_configuration_description(
+ include_airflow: bool = True,
+ include_providers: bool = True,
+ selected_provider: str | None = None,
+) -> dict[str, dict[str, Any]]:
"""
Read Airflow configuration description from YAML file.
+ :param include_airflow: Include Airflow configs
+ :param include_providers: Include provider configs
+ :param selected_provider: If specified, include selected provider only
+ :param config_file_name: name of the file in "config_templates" directory to read default config from
Review Comment:
```suggestion
```
##########
airflow/providers/celery/provider.yaml:
##########
@@ -60,3 +60,256 @@ sensors:
executors:
- airflow.providers.celery.executors.celery_executor.CeleryExecutor
- airflow.providers.celery.executors.celery_kubernetes_executor.CeleryKubernetesExecutor
+
+config:
+ celery_kubernetes_executor:
+ description: |
+ This section only applies if you are using the ``CeleryKubernetesExecutor`` in
+ ``[core]`` section above
Review Comment:
Is "section above" still correct? It is now in a different config right? Same comment for all occurrences of "section above"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1270379439
##########
airflow/configuration.py:
##########
@@ -1732,6 +1787,31 @@ def create_default_config_parser(configuration_description: dict[str, dict[str,
return parser
+def write_default_airflow_configuration_if_needed():
+ if not os.path.isfile(AIRFLOW_CONFIG):
+ import rich
+
+ rich.print("Creating new Airflow config file in: %s", AIRFLOW_CONFIG)
Review Comment:
No, i meant can we import it the way I mentioned?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #32604:
URL: https://github.com/apache/airflow/pull/32604
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1270908162
##########
airflow/configuration.py:
##########
@@ -1664,6 +1785,61 @@ def remove_all_read_configurations(self):
for section in self.sections():
self.remove_section(section)
+ @property
+ def providers_configuration_loaded(self) -> bool:
+ """Checks if providers have been loaded."""
+ return self._providers_configuration_loaded
+
+ def load_providers_configuration(self):
+ """
+ Loads configuration for providers.
+
+ This should be done after initial configuration have been performed. Initializing and discovering
+ providers is an expensive operation and cannot be performed when we load configuration for the first
+ time when airflow starts, because we initialize configuration very early, during importing of the
+ `airflow` package and the module is not yet ready to be used when it happens and until configuration
+ and settings are loaded. Therefore, in order to reload provider configuration we need to additionally
+ load provider - specific configuration.
+ """
+ log.debug("Loading providers configuration")
+ from airflow.providers_manager import ProvidersManager
+
+ self.restore_core_default_configuration()
+ for provider, config in ProvidersManager().already_initialized_provider_configs:
+ for provider_section, provider_section_content in config.items():
+ provider_options = provider_section_content["options"]
+ section_in_current_config = self.configuration_description.get(provider_section)
+ if not section_in_current_config:
+ self.configuration_description[provider_section] = deepcopy(provider_section_content)
+ section_in_current_config = self.configuration_description.get(provider_section)
+ section_in_current_config["source"] = f"default-{provider}"
+ for option in provider_options:
+ section_in_current_config["options"][option]["source"] = f"default-{provider}"
+ else:
+ section_source = section_in_current_config.get("source", "Airflow's core package").split(
+ "default-"
+ )[-1]
+ raise AirflowConfigException(
+ f"The provider {provider} is attempting to contribute "
+ f"configuration section {provider_section} that "
+ f"has already been added before. The source of it: {section_source}."
+ "This is forbidden. A provider can only add new sections but it"
+ "cannot contribute options to existing ones or override other "
Review Comment:
```suggestion
"This is forbidden. A provider can only add new sections. It"
"cannot contribute options to existing sections or override other "
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1269329310
##########
docs/apache-airflow-providers/index.rst:
##########
@@ -48,16 +48,24 @@ describe all the custom capabilities.
Airflow automatically discovers which providers add those additional capabilities and, once you install
provider package and re-start Airflow, those become automatically available to Airflow Users.
-The summary of the core functionalities that can be extended are available in
+The summary of all core functionalities that can be extended are available in
:doc:`/core-extensions/index`.
+Configuration
+'''''''''''''
+
+Providers can have their own configuration options which allow you to configure how they work:
+
+You can see all community-managed providers with their own configuration in
+:doc:`/core-extensions/configurations`
+
Auth backends
'''''''''''''
The providers can add custom authentication backends, that allow you to configure the way how your
web server authenticates your users, integrating it with public or private authentication services.
-You can see all the authentication backends available via community-managed providers in
+You can see all authentication backends available via community-managed providers in
Review Comment:
I NEVER know when to use "a" or "the" - there is no concept of those in Polish, so I semi-randomly throw them in when this feels like a good idea :).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1268176743
##########
airflow/cli/cli_config.py:
##########
@@ -793,6 +814,13 @@ def string_lower_type(val):
action="store_true",
)
+# IMPORTANT NOTE!
+#
+# Celery configs below have explicit fallback values because celery provider defaults are not yet loaded
+# via provider at the time we parse the command line, so in case it is not set, we need to have manual
+# fallback
+# DO NOT REMOVE THE FALLBACKS even if you are tempted to.
+# TODO: possibly move the commands to providers but that could be big performance hit on the CLI
Review Comment:
I have added test/protection to see if there is an accidental usage of providers manager while running CLI parsing. I think it might be useful once you will be merging #29055 - see https://github.com/apache/airflow/pull/32694
I think with the right lazy loading approach it should be possible to the the #29055 as well. But let's see.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1270315748
##########
airflow/providers/celery/provider.yaml:
##########
@@ -60,3 +60,256 @@ sensors:
executors:
- airflow.providers.celery.executors.celery_executor.CeleryExecutor
- airflow.providers.celery.executors.celery_kubernetes_executor.CeleryKubernetesExecutor
+
+config:
+ celery_kubernetes_executor:
+ description: |
+ This section only applies if you are using the ``CeleryKubernetesExecutor`` in
+ ``[core]`` section above
+ options:
+ kubernetes_queue:
+ description: |
+ Define when to send a task to ``KubernetesExecutor`` when using ``CeleryKubernetesExecutor``.
+ When the queue of a task is the value of ``kubernetes_queue`` (default ``kubernetes``),
+ the task is executed via ``KubernetesExecutor``,
+ otherwise via ``CeleryExecutor``
+ version_added: 2.0.0
Review Comment:
I think this is confusing.
It's about airflow version but now that we are in providers users may think it means version 2.0.0 of Celery provider
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1266173257
##########
airflow/configuration.py:
##########
@@ -1474,84 +1679,123 @@ def _generate_fernet_key() -> str:
return Fernet.generate_key().decode()
-def initialize_config() -> AirflowConfigParser:
+def _create_values_from_config_description(
+ default_config_description: dict[str, dict[str, Any]]
+) -> dict[str, dict[str, Any]]:
"""
- Load the Airflow config files.
+ Creates dictionary of default values from configuration description, using
+ based on variables defined in this module (expansion variables).
- Called for you automatically as part of the Airflow boot process.
+ :param default_config_description: description of configuration to read including defaults
+ :return: dictionary of default values based on description and variables defined in this module
"""
- global FERNET_KEY, AIRFLOW_HOME, WEBSERVER_CONFIG
-
- default_config = _parameterized_config_from_template("default_airflow.cfg")
+ all_vars = get_all_expansion_variables()
+ default_values: dict[str, dict[str, Any]] = {}
+ for section, section_desc in default_config_description.items():
+ section_dict: dict[str, Any] = {}
+ default_values[section] = section_dict
+ options = section_desc["options"]
+ for key in options:
+ default_value = options[key]["default"]
+ if default_value is not None:
+ section_dict[key] = (
+ default_value.format(**all_vars) if isinstance(default_value, str) else default_value
+ )
+ return default_values
- local_conf = AirflowConfigParser(default_config=default_config)
- if local_conf.getboolean("core", "unit_test_mode"):
- # Load test config only
- if not os.path.isfile(TEST_CONFIG_FILE):
- from cryptography.fernet import Fernet
+def create_airflow_config_parser(default_config_description: dict[str, dict[str, Any]]):
+ """
+ Creates Airflow config parser.
- log.info("Creating new Airflow config file for unit tests in: %s", TEST_CONFIG_FILE)
- pathlib.Path(AIRFLOW_HOME).mkdir(parents=True, exist_ok=True)
+ Creates Airflow config parser based on variables defined in this module (expansion variables)
+ and default configuration description passed as parameter which serves as source of default values.
- FERNET_KEY = Fernet.generate_key().decode()
+ :param default_config_description: default configuration description - retrieved from config*.yaml files
+ following the schema defined in "config.yml.schema.json" in the config_templates folder.
+ :return: Airflow config parser that can be used to read configuration values from.
+ """
+ return AirflowConfigParser(
+ default_config=_create_values_from_config_description(default_config_description),
+ default_config_description=default_config_description,
+ )
- with open(TEST_CONFIG_FILE, "w") as file:
- cfg = _parameterized_config_from_template("default_test.cfg")
- file.write(cfg)
- make_group_other_inaccessible(TEST_CONFIG_FILE)
- local_conf.load_test_config()
- else:
- # Load normal config
- if not os.path.isfile(AIRFLOW_CONFIG):
- from cryptography.fernet import Fernet
+def load_standard_airflow_configuration(airflow_config_parser: AirflowConfigParser):
+ """
+ Loads standard airflow configuration.
- log.info("Creating new Airflow config file in: %s", AIRFLOW_CONFIG)
- pathlib.Path(AIRFLOW_HOME).mkdir(parents=True, exist_ok=True)
+ In case it finds that the configuration file is missing, it will create it and write the default
+ configuration values there, based on defaults passed, and will add the comments and examples
+ from the default configuration description passed.
- FERNET_KEY = Fernet.generate_key().decode()
+ :param airflow_config_parser: parser to which the configuration will be loaded
+ """
+ global FERNET_KEY, AIRFLOW_HOME
+ if not os.path.isfile(AIRFLOW_CONFIG):
+ from cryptography.fernet import Fernet
+
+ log.info("Creating new Airflow config file in: %s", AIRFLOW_CONFIG)
+ pathlib.Path(AIRFLOW_HOME).mkdir(parents=True, exist_ok=True)
+ FERNET_KEY = Fernet.generate_key().decode()
+ if airflow_config_parser._default_values and airflow_config_parser.default_config_description:
with open(AIRFLOW_CONFIG, "w") as file:
- file.write(default_config)
- make_group_other_inaccessible(AIRFLOW_CONFIG)
-
- log.info("Reading the config from %s", AIRFLOW_CONFIG)
+ airflow_config_parser.write(file)
+ make_group_other_inaccessible(AIRFLOW_CONFIG)
+ log.info("Reading the config from %s", AIRFLOW_CONFIG)
+ airflow_config_parser.read(AIRFLOW_CONFIG)
+ if airflow_config_parser.has_option("core", "AIRFLOW_HOME"):
+ msg = (
+ "Specifying both AIRFLOW_HOME environment variable and airflow_home "
+ "in the config file is deprecated. Please use only the AIRFLOW_HOME "
+ "environment variable and remove the config file entry."
+ )
+ if "AIRFLOW_HOME" in os.environ:
+ warnings.warn(msg, category=DeprecationWarning)
+ elif airflow_config_parser.get("core", "airflow_home") == AIRFLOW_HOME:
+ warnings.warn(
+ "Specifying airflow_home in the config file is deprecated. As you "
+ "have left it at the default value you should remove the setting "
+ "from your airflow.cfg and suffer no change in behaviour.",
+ category=DeprecationWarning,
+ )
+ else:
+ # there
+ AIRFLOW_HOME = airflow_config_parser.get("core", "airflow_home") # type: ignore[assignment]
+ warnings.warn(msg, category=DeprecationWarning)
- local_conf.read(AIRFLOW_CONFIG)
- if local_conf.has_option("core", "AIRFLOW_HOME"):
- msg = (
- "Specifying both AIRFLOW_HOME environment variable and airflow_home "
- "in the config file is deprecated. Please use only the AIRFLOW_HOME "
- "environment variable and remove the config file entry."
- )
- if "AIRFLOW_HOME" in os.environ:
- warnings.warn(msg, category=DeprecationWarning)
- elif local_conf.get("core", "airflow_home") == AIRFLOW_HOME:
- warnings.warn(
- "Specifying airflow_home in the config file is deprecated. As you "
- "have left it at the default value you should remove the setting "
- "from your airflow.cfg and suffer no change in behaviour.",
- category=DeprecationWarning,
- )
- else:
- # there
- AIRFLOW_HOME = local_conf.get("core", "airflow_home") # type: ignore[assignment]
- warnings.warn(msg, category=DeprecationWarning)
+def initialize_config() -> AirflowConfigParser:
+ """
+ Load the Airflow config files.
- # They _might_ have set unit_test_mode in the airflow.cfg, we still
- # want to respect that and then load the unittests.cfg
- if local_conf.getboolean("core", "unit_test_mode"):
- local_conf.load_test_config()
+ Called for you automatically as part of the Airflow boot process.
+ """
+ global FERNET_KEY, AIRFLOW_HOME, WEBSERVER_CONFIG
Review Comment:
They are actually used by
```
def get_all_expansion_variables() -> dict[str, Any]:
return {k: v for d in [globals(), locals()] for k, v in d.items()}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1266172311
##########
airflow/configuration.py:
##########
@@ -136,30 +139,121 @@ def _default_config_file_path(file_name: str) -> str:
return os.path.join(templates_dir, file_name)
-def default_config_yaml() -> dict[str, Any]:
+def default_configuration_description(
+ include_airflow: bool = True,
+ include_providers: bool = True,
+ selected_provider: str | None = None,
+ config_file_name: str = "config.yml",
+) -> dict[str, dict[str, Any]]:
"""
- Read Airflow configs from YAML file.
+ Read Airflow configs from YAML file, and merge them with provider configs optionally.
+ :param include_airflow: Include Airflow configs
+ :param include_providers: Include provider configs
+ :param selected_provider: If specified, include selected provider only
+ :param config_file_name: name of the file in "config_templates" directory to read default config from
:return: Python dictionary containing configs & their info
"""
- with open(_default_config_file_path("config.yml")) as config_file:
- return yaml.safe_load(config_file)
+ base_configuration_description: dict[str, dict[str, Any]] = {}
+ if include_airflow:
+ with open(_default_config_file_path(config_file_name)) as config_file:
+ base_configuration_description.update(yaml.safe_load(config_file))
+ if include_providers:
+ from airflow.providers_manager import ProvidersManager
+
+ for provider, config in ProvidersManager().provider_configs:
+ if selected_provider and provider != selected_provider:
+ continue
+ base_configuration_description.update(config)
+ return base_configuration_description
+
+
+def dejinjafy(value: Any) -> str:
+ if isinstance(value, str):
+ if "{{{{" in value:
+ return value.replace("{{{{", "{{").replace("}}}}", "}}")
+ return value.replace("{{", "{").replace("}}", "}")
+ return value
Review Comment:
I do it WAY better in the final version of those. Wait for it :).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32604:
URL: https://github.com/apache/airflow/pull/32604#issuecomment-1635926757
Looking forward for comments :). Especially - is that OK to merge that one as a single PR, or would reviewers prefer to split that one into smaller ones?
Small sneak-peek of the output generated:
```
airflow config list --include-examples --include-sources --include-variables --include-descriptions
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1263795085
##########
docs/exts/operators_and_hooks_ref.py:
##########
@@ -341,10 +204,24 @@ def render_content(self, *, tags: set[str] | None, header_separator: str = DEFAU
raise NotImplementedError("Tou need to override render_content method.")
+def _common_render_list_content(*, header_separator: str, resource_type: str, template: str):
Review Comment:
@ferruzzi -> we talked about it recently. This is so much nicer now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mobuchowski commented on pull request #32604: Allow configuration to be contributed by providers
Posted by "mobuchowski (via GitHub)" <gi...@apache.org>.
mobuchowski commented on PR #32604:
URL: https://github.com/apache/airflow/pull/32604#issuecomment-1638226188
I like the approach, it definitely will be cleaner to have provider-related config in provider itself. Getting rid of confusing redundancy in `default_airflow.cfg` is also nice.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32604:
URL: https://github.com/apache/airflow/pull/32604#issuecomment-1635919217
Some tests will still fail but I wanted to put that one up for review.
And if you think that this one is huge, then well, yes it is. But worry not - in case we agree it is too huge I have a plan to split that one into a few smaller ones. I just wanted to show what is the target we are heading to - because some of the smaller PRs are better explained seeing the target.
This is something I've attempted already 3 times I think, and I think this time I succeeded - refactoring the way how configuration is read and generated, and converting our config.yml to be the single source of truth - all that while making it possible for providers to contribute their own config.
I hope this PR once merged, will remove a LOT of confusion on how our configuration is read for "production" and "test" mode.
The main change is to allow to contribute configuration by provider (via config: section in provider.yaml) - but there are many side-effects
Among the side-effects of this change:
* no more templated almost-looking-like-real-but-not-entirely-working default_airflow.cfg is removed and replaced with simplte `airflow config list --defaults` (which BTW will also include configuration from installed providers)
* no more generated unittest.cfg (I actually delete one when I find it to get rid of the confusion) - the test configuration gets its new home (config_unit_tests.yml) in `config_templates"
* running configuration tests locally and in CI or breeze will have much higher chance to yield the same results, no more confusion with wher test configuration comes from (hint: there were three places where it could come from)
* provider-contributed configuration is lazy - loaded only after provider's manager is initialized, this way the config cli command and a number of other configuration options will work at the "airflow bootstraping" phase without the need of discovering providers - which is very important for performance of airflow startup and CLI responsivenes. That was quite tricky to figure out, but I think I nailed finally.
* `airflow config list` became a swiss-army-knife of showing all things config, you can decide what you are going to see as an output, including the capability of writing default configuration to a file manually if you want (including providers). It also - similarly to our API shows sources for each configuration value (if you ask for it) and even shows which provider contributed the default value (that's really nice actually)
The code is - I think - much more readable, understandable. more type-hinted, intentions of the code are documented with docstrings and any "special cases" are also documented with comments - why we do things in some strange ways. I removed/replaced quite a bit of "strange" code that seemed to be implemented as band-aid-for-band-aid.
I **think** I got 100% backwards compatibility
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1269322903
##########
airflow/providers/celery/executors/celery_kubernetes_executor.py:
##########
@@ -55,14 +56,21 @@ class CeleryKubernetesExecutor(LoggingMixin):
callback_sink: BaseCallbackSink | None = None
- KUBERNETES_QUEUE = conf.get("celery_kubernetes_executor", "kubernetes_queue")
+ @cached_property
+ def kubernetes_queue(self) -> str:
+ # lazily retrieve the value of kubernetes_queue from the configuration
+ # because it might need providers
+ from airflow.providers_manager import ProvidersManager
+
+ ProvidersManager().initialize_providers_configuration()
+ return conf.get("celery_kubernetes_executor", "kubernetes_queue")
Review Comment:
Not needed because Providers Manager configuration is already initialized two lines above and fallback is retrieved from the configuratoin loaded - fallbacks are only needed for "Arg parsing" - because during arg parsing we should not have providers manager loaded (expensive operation to just parse args). That's why we need hard-coded fallbacks there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk closed pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk closed pull request #32604: Allow configuration to be contributed by providers
URL: https://github.com/apache/airflow/pull/32604
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1269331683
##########
docs/exts/configuration.rst.jinja2:
##########
@@ -0,0 +1,21 @@
+{#
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+#}
+{%for provider_package_name in items %}
+* :doc:`Configuration for {{ provider_package_name }} <{{ provider_package_name }}:configurations-ref>`
+{% endfor %}
Review Comment:
Sure. It's JINJA. You can generate anything :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1270380262
##########
docs/apache-airflow-providers/index.rst:
##########
@@ -48,16 +48,24 @@ describe all the custom capabilities.
Airflow automatically discovers which providers add those additional capabilities and, once you install
provider package and re-start Airflow, those become automatically available to Airflow Users.
-The summary of the core functionalities that can be extended are available in
+The summary of all core functionalities that can be extended are available in
:doc:`/core-extensions/index`.
+Configuration
+'''''''''''''
+
+Providers can have their own configuration options which allow you to configure how they work:
+
+You can see all community-managed providers with their own configuration in
+:doc:`/core-extensions/configurations`
+
Auth backends
'''''''''''''
The providers can add custom authentication backends, that allow you to configure the way how your
web server authenticates your users, integrating it with public or private authentication services.
-You can see all the authentication backends available via community-managed providers in
+You can see all authentication backends available via community-managed providers in
Review Comment:
Haha that's ok!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1270393896
##########
airflow/configuration.py:
##########
@@ -1732,6 +1787,31 @@ def create_default_config_parser(configuration_description: dict[str, dict[str,
return parser
+def write_default_airflow_configuration_if_needed():
+ if not os.path.isfile(AIRFLOW_CONFIG):
+ import rich
+
+ rich.print("Creating new Airflow config file in: %s", AIRFLOW_CONFIG)
Review Comment:
yes. Local imports. We often use it to delay the moment when the imports happen (similar case as with Airlfow's DAG top level code best practice to avoid expensive operations at parsing time).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1263790491
##########
airflow/cli/cli_config.py:
##########
@@ -90,7 +91,7 @@ def _check_value(self, action, value):
f"executors derived from them, your current executor: {executor}, subclassed from: "
f'{", ".join([base_cls.__qualname__ for base_cls in executor_cls.__bases__])}'
)
- raise ArgumentError(action, message)
+ warnings.warn(message)
Review Comment:
I think this one is way better - it issues warning, while allowing you to still run `airflow celery worker --help` and see the options even if you do not have CeleryExecutor configured yet.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1269241851
##########
airflow/configuration.py:
##########
@@ -1732,6 +1787,31 @@ def create_default_config_parser(configuration_description: dict[str, dict[str,
return parser
+def write_default_airflow_configuration_if_needed():
+ if not os.path.isfile(AIRFLOW_CONFIG):
+ import rich
+
+ rich.print("Creating new Airflow config file in: %s", AIRFLOW_CONFIG)
Review Comment:
from rich import print here?
##########
airflow/cli/cli_config.py:
##########
@@ -818,6 +823,13 @@ def string_lower_type(val):
action="store_true",
)
+# IMPORTANT NOTE!
+#
+# Celery configs below have explicit fallback values because celery provider defaults are not yet loaded
+# via provider at the time we parse the command line, so in case it is not set, we need to have manual
+# fallback
+# DO NOT REMOVE THE FALLBACKS even if you are tempted to.
+# TODO: possibly move the commands to providers but that could be big performance hit on the CLI
Review Comment:
Should we rather have it at the top of the page or elsewhere?
##########
airflow/configuration.py:
##########
@@ -151,15 +151,32 @@ def _default_config_file_path(file_name: str) -> str:
return os.path.join(templates_dir, file_name)
-def retrieve_configuration_description() -> dict[str, dict[str, Any]]:
+def retrieve_configuration_description(
+ include_airflow: bool = True,
+ include_providers: bool = True,
+ selected_provider: str | None = None,
+ config_file_name: str = "config.yml",
+) -> dict[str, dict[str, Any]]:
"""
Read Airflow configuration description from YAML file.
+ :param include_airflow: Include Airflow configs
+ :param include_providers: Include provider configs
+ :param selected_provider: If specified, include selected provider only
+ :param config_file_name: name of the file in "config_templates" directory to read default config from
:return: Python dictionary containing configs & their info
"""
base_configuration_description: dict[str, dict[str, Any]] = {}
- with open(_default_config_file_path("config.yml")) as config_file:
- base_configuration_description.update(yaml.safe_load(config_file))
+ if include_airflow:
+ with open(_default_config_file_path("config.yml")) as config_file:
+ base_configuration_description.update(yaml.safe_load(config_file))
+ if include_providers:
+ from airflow.providers_manager import ProvidersManager
+
+ for provider, config in ProvidersManager().provider_configs:
+ if selected_provider and provider != selected_provider:
+ continue
+ base_configuration_description.update(config)
Review Comment:
Is it a valid scenario to include both together? Or should it be an if-else if
##########
docs/apache-airflow/core-concepts/executor/celery_kubernetes.rst:
##########
@@ -28,6 +28,8 @@ An executor is chosen to run a task based on the task's queue.
``CeleryKubernetesExecutor`` inherits the scalability of the ``CeleryExecutor`` to
handle the high load at the peak time and runtime isolation of the ``KubernetesExecutor``.
+The configuration parameters of the Celery Executor can be found in :doc:`apache-airflow-providers-celery:configurations-ref`.
Review Comment:
Should this be a note instead?
##########
airflow/cli/commands/config_command.py:
##########
@@ -48,6 +52,9 @@ def show_config(args):
def get_value(args):
"""Get one value from configuration."""
+ from airflow.providers_manager import ProvidersManager
+
+ ProvidersManager().initialize_providers_configuration()
Review Comment:
Is `initialize_providers_configuration` an idempotent operation, why do we need to add it here?
##########
airflow/providers/celery/executors/celery_kubernetes_executor.py:
##########
@@ -55,14 +56,21 @@ class CeleryKubernetesExecutor(LoggingMixin):
callback_sink: BaseCallbackSink | None = None
- KUBERNETES_QUEUE = conf.get("celery_kubernetes_executor", "kubernetes_queue")
+ @cached_property
+ def kubernetes_queue(self) -> str:
+ # lazily retrieve the value of kubernetes_queue from the configuration
+ # because it might need providers
+ from airflow.providers_manager import ProvidersManager
+
+ ProvidersManager().initialize_providers_configuration()
+ return conf.get("celery_kubernetes_executor", "kubernetes_queue")
Review Comment:
Add a fallback here?
##########
docs/apache-airflow-providers/index.rst:
##########
@@ -75,7 +83,7 @@ Extra links
The providers can add extra custom links to operators delivered by the provider. Those will be visible in
task details view of the task.
-You can see all the extra links available via community-managed providers in
+You can see all extra links available via community-managed providers in
Review Comment:
super nit:
```suggestion
You can see all the extra links available via community-managed providers in
```
##########
docs/apache-airflow/core-concepts/executor/celery.rst:
##########
@@ -29,6 +29,8 @@ change your ``airflow.cfg`` to point the executor parameter to
For more information about setting up a Celery broker, refer to the
exhaustive `Celery documentation on the topic <https://docs.celeryq.dev/en/latest/getting-started/>`_.
+The configuration parameters of the Celery Executor can be found in :doc:`apache-airflow-providers-celery:configurations-ref`.
Review Comment:
Should these be notes instead?
##########
docs/apache-airflow-providers/index.rst:
##########
@@ -48,16 +48,24 @@ describe all the custom capabilities.
Airflow automatically discovers which providers add those additional capabilities and, once you install
provider package and re-start Airflow, those become automatically available to Airflow Users.
-The summary of the core functionalities that can be extended are available in
+The summary of all core functionalities that can be extended are available in
:doc:`/core-extensions/index`.
+Configuration
+'''''''''''''
+
+Providers can have their own configuration options which allow you to configure how they work:
+
+You can see all community-managed providers with their own configuration in
+:doc:`/core-extensions/configurations`
+
Auth backends
'''''''''''''
The providers can add custom authentication backends, that allow you to configure the way how your
web server authenticates your users, integrating it with public or private authentication services.
-You can see all the authentication backends available via community-managed providers in
+You can see all authentication backends available via community-managed providers in
Review Comment:
super nit:
```suggestion
You can see all the authentication backends available via community-managed providers in
```
##########
docs/exts/configuration.rst.jinja2:
##########
@@ -0,0 +1,21 @@
+{#
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+#}
+{%for provider_package_name in items %}
+* :doc:`Configuration for {{ provider_package_name }} <{{ provider_package_name }}:configurations-ref>`
+{% endfor %}
Review Comment:
Wow didnt know this way possible!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1269320232
##########
airflow/cli/commands/config_command.py:
##########
@@ -48,6 +52,9 @@ def show_config(args):
def get_value(args):
"""Get one value from configuration."""
+ from airflow.providers_manager import ProvidersManager
+
+ ProvidersManager().initialize_providers_configuration()
Review Comment:
Because we want to force loading configuration of providers information. Providers data is lazy-loaded, when needed because it is expensive operation (querying provider entrypoints from installed packages, lots of imports done etc. etc. and we load it at the very last moment we need to do it).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1269502530
##########
airflow/configuration.py:
##########
@@ -151,15 +151,32 @@ def _default_config_file_path(file_name: str) -> str:
return os.path.join(templates_dir, file_name)
-def retrieve_configuration_description() -> dict[str, dict[str, Any]]:
+def retrieve_configuration_description(
+ include_airflow: bool = True,
+ include_providers: bool = True,
+ selected_provider: str | None = None,
+ config_file_name: str = "config.yml",
+) -> dict[str, dict[str, Any]]:
"""
Read Airflow configuration description from YAML file.
+ :param include_airflow: Include Airflow configs
+ :param include_providers: Include provider configs
+ :param selected_provider: If specified, include selected provider only
+ :param config_file_name: name of the file in "config_templates" directory to read default config from
:return: Python dictionary containing configs & their info
"""
base_configuration_description: dict[str, dict[str, Any]] = {}
- with open(_default_config_file_path("config.yml")) as config_file:
- base_configuration_description.update(yaml.safe_load(config_file))
+ if include_airflow:
+ with open(_default_config_file_path("config.yml")) as config_file:
+ base_configuration_description.update(yaml.safe_load(config_file))
+ if include_providers:
+ from airflow.providers_manager import ProvidersManager
+
+ for provider, config in ProvidersManager().provider_configs:
+ if selected_provider and provider != selected_provider:
+ continue
+ base_configuration_description.update(config)
Review Comment:
Absolutely valid. When you want "all configuration" you should include both.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1269504189
##########
airflow/providers/celery/provider.yaml:
##########
@@ -60,3 +60,256 @@ sensors:
executors:
- airflow.providers.celery.executors.celery_executor.CeleryExecutor
- airflow.providers.celery.executors.celery_kubernetes_executor.CeleryKubernetesExecutor
+
+config:
+ celery_kubernetes_executor:
+ description: |
+ This section only applies if you are using the ``CeleryKubernetesExecutor`` in
+ ``[core]`` section above
Review Comment:
Ah. Good spot :). It makes little sense anyway :).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1263789241
##########
TESTING.rst:
##########
@@ -55,8 +55,33 @@ Follow the guidelines when writing unit tests:
tests, so we run Pytest with ``--disable-warnings`` but instead we have ``pytest-capture-warnings`` plugin that
overrides ``recwarn`` fixture behaviour.
-**NOTE:** We plan to convert all unit tests to standard "asserts" semi-automatically, but this will be done later
-in Airflow 2.0 development phase. That will include setUp/tearDown/context managers and decorators.
+
+.. note::
+
+ We are in the process of convert all unit tests to standard "asserts" and pytest fixtures
Review Comment:
```suggestion
We are in the process of converting all unit tests to standard "asserts" and pytest fixtures
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1263799803
##########
scripts/in_container/airflow_ci.cfg:
##########
@@ -1,59 +0,0 @@
-#
Review Comment:
@mobuchowski @amoghrajesh @hussein-awala @dstandish -> we 've been discussing all the confusion about that one several times. Now this one is gone with single source of truth in `config_unit_tests.yml`. No more unittest.cfg file generated in AIRFLOW_HOME. defailts from config_unit_tests.yml are loaded directly during tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32604:
URL: https://github.com/apache/airflow/pull/32604#issuecomment-1636987332
BTW. Converted to a draft and extracting parts of it to separate PRs to make it all reviewable and mergable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1270242124
##########
docs/apache-airflow/configurations-ref.rst:
##########
@@ -32,6 +32,13 @@ generated using the secret key has a short expiry time though - make sure that t
that you run airflow components on is synchronized (for example using ntpd) otherwise you might get
"forbidden" errors when the logs are accessed.
+Some of the providers have their own configuration options, you will find details of their configuration
+in the provider's documentation. The pre-installed providers that you likely want to configure are:
Review Comment:
Yep.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] o-nikolas commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1265730580
##########
TESTING.rst:
##########
@@ -55,8 +55,33 @@ Follow the guidelines when writing unit tests:
tests, so we run Pytest with ``--disable-warnings`` but instead we have ``pytest-capture-warnings`` plugin that
overrides ``recwarn`` fixture behaviour.
-**NOTE:** We plan to convert all unit tests to standard "asserts" semi-automatically, but this will be done later
-in Airflow 2.0 development phase. That will include setUp/tearDown/context managers and decorators.
+
+.. note::
+
+ We are in the process of convert all unit tests to standard "asserts" and pytest fixtures
+ so if you find some tests that are still using classic setUp/tearDown approach or unittest asserts, feel
+ free to convert them to pytest.
+
+Airflow configuration for unit tests
+------------------------------------
+
+Some of the unit tests require special configuration set as "default" one. This is done automatically by
+automatically adding ``AIRFLOW__CORE__UNIT_TEST_MODE=True`` to the environment variables in Pytest auto-used
Review Comment:
```suggestion
Some of the unit tests require special configuration set as the "default". This is done automatically by
adding ``AIRFLOW__CORE__UNIT_TEST_MODE=True`` to the environment variables in Pytest auto-used
```
##########
TESTING.rst:
##########
@@ -55,8 +55,33 @@ Follow the guidelines when writing unit tests:
tests, so we run Pytest with ``--disable-warnings`` but instead we have ``pytest-capture-warnings`` plugin that
overrides ``recwarn`` fixture behaviour.
-**NOTE:** We plan to convert all unit tests to standard "asserts" semi-automatically, but this will be done later
-in Airflow 2.0 development phase. That will include setUp/tearDown/context managers and decorators.
+
+.. note::
+
+ We are in the process of convert all unit tests to standard "asserts" and pytest fixtures
+ so if you find some tests that are still using classic setUp/tearDown approach or unittest asserts, feel
+ free to convert them to pytest.
+
+Airflow configuration for unit tests
+------------------------------------
+
+Some of the unit tests require special configuration set as "default" one. This is done automatically by
+automatically adding ``AIRFLOW__CORE__UNIT_TEST_MODE=True`` to the environment variables in Pytest auto-used
+fixture. This in turn makes airflow configuration load test configuration from
+``airflow/config_templates/config_unit_test.yml`` - defaults from the test configuration replace the original
+defaults stored in ``airflow/config_templates/config.yml``. If you want to add some test-only configuration,
+you should copy the option definition to ``config_unit_test.yml`` and change default value there.
+
+.. note::
+
+ The test configuration for Airflow before July 2023 was automatically generated in a file named
+ ``AIRFLOW_HOME/unittest.cfg`` and template for it was stored in "config_templates" next to the yaml file,
Review Comment:
```suggestion
``AIRFLOW_HOME/unittest.cfg`` and the template for it was stored in "config_templates" next to the yaml file,
```
##########
TESTING.rst:
##########
@@ -55,8 +55,33 @@ Follow the guidelines when writing unit tests:
tests, so we run Pytest with ``--disable-warnings`` but instead we have ``pytest-capture-warnings`` plugin that
overrides ``recwarn`` fixture behaviour.
-**NOTE:** We plan to convert all unit tests to standard "asserts" semi-automatically, but this will be done later
-in Airflow 2.0 development phase. That will include setUp/tearDown/context managers and decorators.
+
+.. note::
+
+ We are in the process of convert all unit tests to standard "asserts" and pytest fixtures
+ so if you find some tests that are still using classic setUp/tearDown approach or unittest asserts, feel
+ free to convert them to pytest.
+
+Airflow configuration for unit tests
+------------------------------------
+
+Some of the unit tests require special configuration set as "default" one. This is done automatically by
+automatically adding ``AIRFLOW__CORE__UNIT_TEST_MODE=True`` to the environment variables in Pytest auto-used
+fixture. This in turn makes airflow configuration load test configuration from
+``airflow/config_templates/config_unit_test.yml`` - defaults from the test configuration replace the original
+defaults stored in ``airflow/config_templates/config.yml``. If you want to add some test-only configuration,
+you should copy the option definition to ``config_unit_test.yml`` and change default value there.
+
+.. note::
+
+ The test configuration for Airflow before July 2023 was automatically generated in a file named
+ ``AIRFLOW_HOME/unittest.cfg`` and template for it was stored in "config_templates" next to the yaml file,
+ however this was only done for the first time you run airflow and you had to manually maintain the file,
+ and it was pretty cumbrsome and arcane knowledge, also this file has been overwritten in Breeze environment
Review Comment:
```suggestion
however this was only done for the first time you run airflow and you had to manually maintain the file,
and it was pretty cumbersome and arcane knowledge, also this file has been overwritten in Breeze environment
```
##########
airflow/cli/cli_config.py:
##########
@@ -90,7 +91,7 @@ def _check_value(self, action, value):
f"executors derived from them, your current executor: {executor}, subclassed from: "
f'{", ".join([base_cls.__qualname__ for base_cls in executor_cls.__bases__])}'
)
- raise ArgumentError(action, message)
+ warnings.warn(message)
Review Comment:
Most of this code will leave soon anyways when I finish the CLI abstraction for Airflow CLI (see our [convo here](https://github.com/apache/airflow/pull/29055#discussion_r1082091328)).
##########
airflow/cli/cli_config.py:
##########
@@ -793,6 +814,13 @@ def string_lower_type(val):
action="store_true",
)
+# IMPORTANT NOTE!
+#
+# Celery configs below have explicit fallback values because celery provider defaults are not yet loaded
+# via provider at the time we parse the command line, so in case it is not set, we need to have manual
+# fallback
+# DO NOT REMOVE THE FALLBACKS even if you are tempted to.
+# TODO: possibly move the commands to providers but that could be big performance hit on the CLI
Review Comment:
@potiuk
AIP-51 PR https://github.com/apache/airflow/pull/29055 will move the commands to the executor modules (which will be in providers now) so this will actually be done by that.
I've spent time optimizing the import times of the executor modules so it should not slow down the CLI much at all.
Was there another concern you had with regards to performance?
##########
airflow/config_templates/default_airflow.cfg:
##########
@@ -16,1507 +15,22 @@
# specific language governing permissions and limitations
# under the License.
-# This is the template for Airflow's default configuration. When Airflow is
-# imported, it looks for a configuration file at $AIRFLOW_HOME/airflow.cfg. If
-# it doesn't exist, Airflow uses this template to generate it by replacing
-# variables in curly braces with their global values from configuration.py.
-
-# Users should not modify this file; they should customize the generated
-# airflow.cfg instead.
-
-
-# ----------------------- TEMPLATE BEGINS HERE -----------------------
-
-[core]
-# The folder where your airflow pipelines live, most likely a
-# subfolder in a code repository. This path must be absolute.
-dags_folder = {AIRFLOW_HOME}/dags
-
-# Hostname by providing a path to a callable, which will resolve the hostname.
-# The format is "package.function".
#
-# For example, default value "airflow.utils.net.getfqdn" means that result from patched
-# version of socket.getfqdn() - see https://github.com/python/cpython/issues/49254.
+# NOTE IF YOU ARE LOOKING FOR DEFAULT CONFIGURATION FILE HERE !!! LOOK NO MORE. READ NOTE BELOW!
#
-# No argument should be required in the function specified.
-# If using IP address as hostname is preferred, use value ``airflow.utils.net.get_host_ip_address``
-hostname_callable = airflow.utils.net.getfqdn
-
-# A callable to check if a python file has airflow dags defined or not
-# with argument as: `(file_path: str, zip_file: zipfile.ZipFile | None = None)`
-# return True if it has dags otherwise False
-# If this is not provided, Airflow uses its own heuristic rules.
-might_contain_dag_callable = airflow.utils.file.might_contain_dag_via_default_heuristic
-
-# Default timezone in case supplied date times are naive
-# can be utc (default), system, or any IANA timezone string (e.g. Europe/Amsterdam)
-default_timezone = utc
-
-# The executor class that airflow should use. Choices include
-# ``SequentialExecutor``, ``LocalExecutor``, ``CeleryExecutor``, ``DaskExecutor``,
-# ``KubernetesExecutor``, ``CeleryKubernetesExecutor`` or the
-# full import path to the class when using a custom executor.
-executor = SequentialExecutor
-
-# The auth manager class that airflow should use. Full import path to the auth manager class.
-auth_manager = airflow.auth.managers.fab.fab_auth_manager.FabAuthManager
-
-# This defines the maximum number of task instances that can run concurrently per scheduler in
-# Airflow, regardless of the worker count. Generally this value, multiplied by the number of
-# schedulers in your cluster, is the maximum number of task instances with the running
-# state in the metadata database.
-parallelism = 32
-
-# The maximum number of task instances allowed to run concurrently in each DAG. To calculate
-# the number of tasks that is running concurrently for a DAG, add up the number of running
-# tasks for all DAG runs of the DAG. This is configurable at the DAG level with ``max_active_tasks``,
-# which is defaulted as ``max_active_tasks_per_dag``.
+# This file used to have something that reminded default Airflow configuration but it was really a template
+# That was used to generate it and it was confusing if you copied it to your configuration and some
+# configuration keys were wrong.
#
-# An example scenario when this would be useful is when you want to stop a new dag with an early
-# start date from stealing all the executor slots in a cluster.
-max_active_tasks_per_dag = 16
-
-# Are DAGs paused by default at creation
-dags_are_paused_at_creation = True
-
-# The maximum number of active DAG runs per DAG. The scheduler will not create more DAG runs
-# if it reaches the limit. This is configurable at the DAG level with ``max_active_runs``,
-# which is defaulted as ``max_active_runs_per_dag``.
-max_active_runs_per_dag = 16
-
-# The name of the method used in order to start Python processes via the multiprocessing module.
-# This corresponds directly with the options available in the Python docs:
-# https://docs.python.org/3/library/multiprocessing.html#multiprocessing.set_start_method.
-# Must be one of the values returned by:
-# https://docs.python.org/3/library/multiprocessing.html#multiprocessing.get_all_start_methods.
-# Example: mp_start_method = fork
-# mp_start_method =
-
-# Whether to load the DAG examples that ship with Airflow. It's good to
-# get started, but you probably want to set this to ``False`` in a production
-# environment
-load_examples = True
-
-# Path to the folder containing Airflow plugins
-plugins_folder = {AIRFLOW_HOME}/plugins
-
-# Should tasks be executed via forking of the parent process ("False",
-# the speedier option) or by spawning a new python process ("True" slow,
-# but means plugin changes picked up by tasks straight away)
-execute_tasks_new_python_interpreter = False
-
-# Secret key to save connection passwords in the db
-fernet_key = {FERNET_KEY}
-
-# Whether to disable pickling dags
-donot_pickle = True
-
-# How long before timing out a python file import
-dagbag_import_timeout = 30.0
-
-# Should a traceback be shown in the UI for dagbag import errors,
-# instead of just the exception message
-dagbag_import_error_tracebacks = True
-
-# If tracebacks are shown, how many entries from the traceback should be shown
-dagbag_import_error_traceback_depth = 2
-
-# How long before timing out a DagFileProcessor, which processes a dag file
-dag_file_processor_timeout = 50
-
-# The class to use for running task instances in a subprocess.
-# Choices include StandardTaskRunner, CgroupTaskRunner or the full import path to the class
-# when using a custom task runner.
-task_runner = StandardTaskRunner
-
-# If set, tasks without a ``run_as_user`` argument will be run with this user
-# Can be used to de-elevate a sudo user running Airflow when executing tasks
-default_impersonation =
-
-# What security module to use (for example kerberos)
-security =
-
-# Turn unit test mode on (overwrites many configuration options with test
-# values at runtime)
-unit_test_mode = False
-
-# Whether to enable pickling for xcom (note that this is insecure and allows for
-# RCE exploits).
-enable_xcom_pickling = False
-
-# What classes can be imported during deserialization. This is a multi line value.
-# The individual items will be parsed as regexp. Python built-in classes (like dict)
-# are always allowed. Bare "." will be replaced so you can set airflow.* .
-allowed_deserialization_classes = airflow\..*
-
-# When a task is killed forcefully, this is the amount of time in seconds that
-# it has to cleanup after it is sent a SIGTERM, before it is SIGKILLED
-killed_task_cleanup_time = 60
-
-# Whether to override params with dag_run.conf. If you pass some key-value pairs
-# through ``airflow dags backfill -c`` or
-# ``airflow dags trigger -c``, the key-value pairs will override the existing ones in params.
-dag_run_conf_overrides_params = True
-
-# If enabled, Airflow will only scan files containing both ``DAG`` and ``airflow`` (case-insensitive).
-dag_discovery_safe_mode = True
-
-# The pattern syntax used in the ".airflowignore" files in the DAG directories. Valid values are
-# ``regexp`` or ``glob``.
-dag_ignore_file_syntax = regexp
-
-# The number of retries each task is going to have by default. Can be overridden at dag or task level.
-default_task_retries = 0
-
-# The number of seconds each task is going to wait by default between retries. Can be overridden at
-# dag or task level.
-default_task_retry_delay = 300
-
-# The maximum delay (in seconds) each task is going to wait by default between retries.
-# This is a global setting and cannot be overridden at task or DAG level.
-max_task_retry_delay = 86400
-
-# The weighting method used for the effective total priority weight of the task
-default_task_weight_rule = downstream
-
-# The default task execution_timeout value for the operators. Expected an integer value to
-# be passed into timedelta as seconds. If not specified, then the value is considered as None,
-# meaning that the operators are never timed out by default.
-default_task_execution_timeout =
-
-# Updating serialized DAG can not be faster than a minimum interval to reduce database write rate.
-min_serialized_dag_update_interval = 30
-
-# If True, serialized DAGs are compressed before writing to DB.
-# Note: this will disable the DAG dependencies view
-compress_serialized_dags = False
-
-# Fetching serialized DAG can not be faster than a minimum interval to reduce database
-# read rate. This config controls when your DAGs are updated in the Webserver
-min_serialized_dag_fetch_interval = 10
-
-# Maximum number of Rendered Task Instance Fields (Template Fields) per task to store
-# in the Database.
-# All the template_fields for each of Task Instance are stored in the Database.
-# Keeping this number small may cause an error when you try to view ``Rendered`` tab in
-# TaskInstance view for older tasks.
-max_num_rendered_ti_fields_per_task = 30
-
-# On each dagrun check against defined SLAs
-check_slas = True
-
-# Path to custom XCom class that will be used to store and resolve operators results
-# Example: xcom_backend = path.to.CustomXCom
-xcom_backend = airflow.models.xcom.BaseXCom
-
-# By default Airflow plugins are lazily-loaded (only loaded when required). Set it to ``False``,
-# if you want to load plugins whenever 'airflow' is invoked via cli or loaded from module.
-lazy_load_plugins = True
-
-# By default Airflow providers are lazily-discovered (discovery and imports happen only when required).
-# Set it to False, if you want to discover providers whenever 'airflow' is invoked via cli or
-# loaded from module.
-lazy_discover_providers = True
-
-# Hide sensitive Variables or Connection extra json keys from UI and task logs when set to True
-#
-# (Connection passwords are always hidden in logs)
-hide_sensitive_var_conn_fields = True
-
-# A comma-separated list of extra sensitive keywords to look for in variables names or connection's
-# extra JSON.
-sensitive_var_conn_names =
-
-# Task Slot counts for ``default_pool``. This setting would not have any effect in an existing
-# deployment where the ``default_pool`` is already created. For existing deployments, users can
-# change the number of slots using Webserver, API or the CLI
-default_pool_task_slot_count = 128
-
-# The maximum list/dict length an XCom can push to trigger task mapping. If the pushed list/dict has a
-# length exceeding this value, the task pushing the XCom will be failed automatically to prevent the
-# mapped tasks from clogging the scheduler.
-max_map_length = 1024
-
-# The default umask to use for process when run in daemon mode (scheduler, worker, etc.)
-#
-# This controls the file-creation mode mask which determines the initial value of file permission bits
-# for newly created files.
+# Airflow will generate default configuration for you when you run it for the first time in
Review Comment:
```suggestion
# Airflow will generate the default configuration for you when you run it for the first time in
```
##########
airflow/config_templates/default_airflow.cfg:
##########
@@ -16,1507 +15,22 @@
# specific language governing permissions and limitations
# under the License.
-# This is the template for Airflow's default configuration. When Airflow is
-# imported, it looks for a configuration file at $AIRFLOW_HOME/airflow.cfg. If
-# it doesn't exist, Airflow uses this template to generate it by replacing
-# variables in curly braces with their global values from configuration.py.
-
-# Users should not modify this file; they should customize the generated
-# airflow.cfg instead.
-
-
-# ----------------------- TEMPLATE BEGINS HERE -----------------------
-
-[core]
-# The folder where your airflow pipelines live, most likely a
-# subfolder in a code repository. This path must be absolute.
-dags_folder = {AIRFLOW_HOME}/dags
-
-# Hostname by providing a path to a callable, which will resolve the hostname.
-# The format is "package.function".
#
-# For example, default value "airflow.utils.net.getfqdn" means that result from patched
-# version of socket.getfqdn() - see https://github.com/python/cpython/issues/49254.
+# NOTE IF YOU ARE LOOKING FOR DEFAULT CONFIGURATION FILE HERE !!! LOOK NO MORE. READ NOTE BELOW!
#
-# No argument should be required in the function specified.
-# If using IP address as hostname is preferred, use value ``airflow.utils.net.get_host_ip_address``
-hostname_callable = airflow.utils.net.getfqdn
-
-# A callable to check if a python file has airflow dags defined or not
-# with argument as: `(file_path: str, zip_file: zipfile.ZipFile | None = None)`
-# return True if it has dags otherwise False
-# If this is not provided, Airflow uses its own heuristic rules.
-might_contain_dag_callable = airflow.utils.file.might_contain_dag_via_default_heuristic
-
-# Default timezone in case supplied date times are naive
-# can be utc (default), system, or any IANA timezone string (e.g. Europe/Amsterdam)
-default_timezone = utc
-
-# The executor class that airflow should use. Choices include
-# ``SequentialExecutor``, ``LocalExecutor``, ``CeleryExecutor``, ``DaskExecutor``,
-# ``KubernetesExecutor``, ``CeleryKubernetesExecutor`` or the
-# full import path to the class when using a custom executor.
-executor = SequentialExecutor
-
-# The auth manager class that airflow should use. Full import path to the auth manager class.
-auth_manager = airflow.auth.managers.fab.fab_auth_manager.FabAuthManager
-
-# This defines the maximum number of task instances that can run concurrently per scheduler in
-# Airflow, regardless of the worker count. Generally this value, multiplied by the number of
-# schedulers in your cluster, is the maximum number of task instances with the running
-# state in the metadata database.
-parallelism = 32
-
-# The maximum number of task instances allowed to run concurrently in each DAG. To calculate
-# the number of tasks that is running concurrently for a DAG, add up the number of running
-# tasks for all DAG runs of the DAG. This is configurable at the DAG level with ``max_active_tasks``,
-# which is defaulted as ``max_active_tasks_per_dag``.
+# This file used to have something that reminded default Airflow configuration but it was really a template
+# That was used to generate it and it was confusing if you copied it to your configuration and some
+# configuration keys were wrong.
Review Comment:
```suggestion
# This file used to have something that was similar to the default Airflow configuration but it was really just a template.
# It was used to generate the final configuration and it was confusing if you copied it to your configuration and some of
# keys were wrong.
```
##########
TESTING.rst:
##########
@@ -55,8 +55,33 @@ Follow the guidelines when writing unit tests:
tests, so we run Pytest with ``--disable-warnings`` but instead we have ``pytest-capture-warnings`` plugin that
overrides ``recwarn`` fixture behaviour.
-**NOTE:** We plan to convert all unit tests to standard "asserts" semi-automatically, but this will be done later
-in Airflow 2.0 development phase. That will include setUp/tearDown/context managers and decorators.
+
+.. note::
+
+ We are in the process of convert all unit tests to standard "asserts" and pytest fixtures
+ so if you find some tests that are still using classic setUp/tearDown approach or unittest asserts, feel
+ free to convert them to pytest.
+
+Airflow configuration for unit tests
+------------------------------------
+
+Some of the unit tests require special configuration set as "default" one. This is done automatically by
+automatically adding ``AIRFLOW__CORE__UNIT_TEST_MODE=True`` to the environment variables in Pytest auto-used
+fixture. This in turn makes airflow configuration load test configuration from
+``airflow/config_templates/config_unit_test.yml`` - defaults from the test configuration replace the original
+defaults stored in ``airflow/config_templates/config.yml``. If you want to add some test-only configuration,
+you should copy the option definition to ``config_unit_test.yml`` and change default value there.
Review Comment:
```suggestion
you should copy the option definition to ``config_unit_test.yml`` and change the default value there.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32604:
URL: https://github.com/apache/airflow/pull/32604#issuecomment-1642156847
OK. This one is rebased on top of #32669 - so looks big but only last commit counts assuming #32669 will be merged - it is now what remains from the big original PR, all the rest is currently separated out to separate PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1268155992
##########
airflow/providers/celery/executors/celery_kubernetes_executor.py:
##########
@@ -55,14 +55,21 @@ class CeleryKubernetesExecutor(LoggingMixin):
callback_sink: BaseCallbackSink | None = None
- KUBERNETES_QUEUE = conf.get("celery_kubernetes_executor", "kubernetes_queue")
+ @property
Review Comment:
Actualy it was possible to do without any problems. The method inside config_property actually initializes providers manager and reads the configuration before it cached, so we are absolutely ok with turning that one into cached_property.
Fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32604:
URL: https://github.com/apache/airflow/pull/32604#issuecomment-1644592803
> Looks like we’re being hit by the Cython 3.0 release. I’ve heard a few instances of it breaking the build.
Very much that. We are hit rather small-ish way (pymssql on ARM) but this is a mayhem for the whole Python ecosystem it seems.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1270242735
##########
docs/providers-configurations-ref.rst:
##########
Review Comment:
Yes. I can likely use include. Good point.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #32604: Allow configuration to be contributed by providers
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #32604:
URL: https://github.com/apache/airflow/pull/32604#issuecomment-1644589443
Looks like we’re being hit by the Cython 3.0 release. I’ve heard a few instances of it breaking the build.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1269522866
##########
airflow/configuration.py:
##########
@@ -151,15 +151,31 @@ def _default_config_file_path(file_name: str) -> str:
return os.path.join(templates_dir, file_name)
-def retrieve_configuration_description() -> dict[str, dict[str, Any]]:
+def retrieve_configuration_description(
+ include_airflow: bool = True,
+ include_providers: bool = True,
+ selected_provider: str | None = None,
+) -> dict[str, dict[str, Any]]:
"""
Read Airflow configuration description from YAML file.
+ :param include_airflow: Include Airflow configs
+ :param include_providers: Include provider configs
+ :param selected_provider: If specified, include selected provider only
+ :param config_file_name: name of the file in "config_templates" directory to read default config from
Review Comment:
Right.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1270241742
##########
airflow/cli/commands/config_command.py:
##########
@@ -48,6 +49,14 @@ def show_config(args):
def get_value(args):
"""Get one value from configuration."""
+ # while this will make get_value quite a bit slower we must initialize configuration
+ # for providers because we do not know what sections and options will be available after
+ # providers are initialized. Theoretically Providers might add new sections and options
+ # but also override defaults for existing options, so without loading all providers we
Review Comment:
Discussion https://lists.apache.org/thread/lrlmsl7qz0rq3206zlybvf2yfzw29nby
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1269330826
##########
docs/apache-airflow/core-concepts/executor/celery.rst:
##########
@@ -29,6 +29,8 @@ change your ``airflow.cfg`` to point the executor parameter to
For more information about setting up a Celery broker, refer to the
exhaustive `Celery documentation on the topic <https://docs.celeryq.dev/en/latest/getting-started/>`_.
+The configuration parameters of the Celery Executor can be found in :doc:`apache-airflow-providers-celery:configurations-ref`.
Review Comment:
This is a basic information, not some kind of "additional" thing.
##########
docs/apache-airflow/core-concepts/executor/celery_kubernetes.rst:
##########
@@ -28,6 +28,8 @@ An executor is chosen to run a task based on the task's queue.
``CeleryKubernetesExecutor`` inherits the scalability of the ``CeleryExecutor`` to
handle the high load at the peak time and runtime isolation of the ``KubernetesExecutor``.
+The configuration parameters of the Celery Executor can be found in :doc:`apache-airflow-providers-celery:configurations-ref`.
Review Comment:
Same here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1270390552
##########
airflow/configuration.py:
##########
@@ -1644,6 +1765,61 @@ def remove_all_read_configurations(self):
for section in self.sections():
self.remove_section(section)
+ @property
+ def providers_configuration_loaded(self) -> bool:
+ """Checks if providers have been loaded."""
+ return self._providers_configuration_loaded
+
+ def load_providers_configuration(self):
+ """
+ Loads configuration for providers.
+
+ This should be done after initial configuration have been performed. Initializing and discovering
+ providers is an expensive operation and cannot be performed when we load configuration for the first
+ time when airflow starts, because we initialize configuration very early, during importing of the
+ `airflow` package and the module is not yet ready to be used when it happens and until configuration
+ and settings are loaded. Therefore, in order to reload provider configuration we need to additionally
+ load provider - specific configuration.
+ """
+ log.debug("Loading providers configuration")
+ from airflow.providers_manager import ProvidersManager
+
+ for provider, config in ProvidersManager().already_initialized_provider_configs:
+ for provider_section, provider_section_content in config.items():
+ provider_options = provider_section_content["options"]
+ section_in_current_config = self.configuration_description.get(provider_section)
+ if not section_in_current_config:
+ # merge whole sections into existing config
+ self.configuration_description[provider_section] = deepcopy(provider_section_content)
+ section_in_current_config = self.configuration_description.get(provider_section)
+ for option in provider_options:
+ section_in_current_config["options"][option]["source"] = f"default-{provider}"
+ else:
+ for option in provider_options:
+ option_in_current_config = section_in_current_config["options"].get(option)
+ if not option_in_current_config:
+ # merge new options into existing sections
+ self.configuration_description[provider_section]["options"][option] = deepcopy(
+ provider_options[option]
+ )
+ option_in_current_config = section_in_current_config["options"].get(option)
+ option_in_current_config["source"] = f"default-{provider}"
+ else:
+ # replace default values for existing options
Review Comment:
Let's see what the discussion brings. I am fine with raising as well, as I think it gives more versatilty (see the logging/celery case in the discussion).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1270703666
##########
airflow/configuration.py:
##########
@@ -1644,6 +1765,61 @@ def remove_all_read_configurations(self):
for section in self.sections():
self.remove_section(section)
+ @property
+ def providers_configuration_loaded(self) -> bool:
+ """Checks if providers have been loaded."""
+ return self._providers_configuration_loaded
+
+ def load_providers_configuration(self):
+ """
+ Loads configuration for providers.
+
+ This should be done after initial configuration have been performed. Initializing and discovering
+ providers is an expensive operation and cannot be performed when we load configuration for the first
+ time when airflow starts, because we initialize configuration very early, during importing of the
+ `airflow` package and the module is not yet ready to be used when it happens and until configuration
+ and settings are loaded. Therefore, in order to reload provider configuration we need to additionally
+ load provider - specific configuration.
+ """
+ log.debug("Loading providers configuration")
+ from airflow.providers_manager import ProvidersManager
+
+ for provider, config in ProvidersManager().already_initialized_provider_configs:
+ for provider_section, provider_section_content in config.items():
+ provider_options = provider_section_content["options"]
+ section_in_current_config = self.configuration_description.get(provider_section)
+ if not section_in_current_config:
+ # merge whole sections into existing config
+ self.configuration_description[provider_section] = deepcopy(provider_section_content)
+ section_in_current_config = self.configuration_description.get(provider_section)
+ for option in provider_options:
+ section_in_current_config["options"][option]["source"] = f"default-{provider}"
+ else:
+ for option in provider_options:
+ option_in_current_config = section_in_current_config["options"].get(option)
+ if not option_in_current_config:
+ # merge new options into existing sections
+ self.configuration_description[provider_section]["options"][option] = deepcopy(
+ provider_options[option]
+ )
+ option_in_current_config = section_in_current_config["options"].get(option)
+ option_in_current_config["source"] = f"default-{provider}"
+ else:
+ # replace default values for existing options
Review Comment:
OK. I assumed the result of the discussion (at least I am leaning towards it) is that only whole sections can be contributed (and sections cannot be overwritten by another provider). Updated it and added test cases covering it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1263791726
##########
airflow/config_templates/config.yml:
##########
@@ -805,13 +805,15 @@ logging:
version_added: 2.0.0
type: string
example: "{{ti.dag_id}}-{{ti.task_id}}-{{execution_date}}-{{try_number}}"
+ is_template: true
Review Comment:
Better way to say which fileds contain templates (used be endswith("_template").
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ferruzzi commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1265785329
##########
docs/exts/operators_and_hooks_ref.py:
##########
@@ -341,10 +204,24 @@ def render_content(self, *, tags: set[str] | None, header_separator: str = DEFAU
raise NotImplementedError("Tou need to override render_content method.")
+def _common_render_list_content(*, header_separator: str, resource_type: str, template: str):
Review Comment:
Oh yeah, It's beautiful. :heart_eyes:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1270330511
##########
airflow/providers/celery/provider.yaml:
##########
@@ -60,3 +60,256 @@ sensors:
executors:
- airflow.providers.celery.executors.celery_executor.CeleryExecutor
- airflow.providers.celery.executors.celery_kubernetes_executor.CeleryKubernetesExecutor
+
+config:
+ celery_kubernetes_executor:
+ description: |
+ This section only applies if you are using the ``CeleryKubernetesExecutor`` in
+ ``[core]`` section above
+ options:
+ kubernetes_queue:
+ description: |
+ Define when to send a task to ``KubernetesExecutor`` when using ``CeleryKubernetesExecutor``.
+ When the queue of a task is the value of ``kubernetes_queue`` (default ``kubernetes``),
+ the task is executed via ``KubernetesExecutor``,
+ otherwise via ``CeleryExecutor``
+ version_added: 2.0.0
Review Comment:
Right. We should remove it simply. It makes no sense to have it here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32604:
URL: https://github.com/apache/airflow/pull/32604#issuecomment-1644578831
All is good except ARM builds (which I am working on) . Definitely ready for review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1270215447
##########
airflow/cli/commands/config_command.py:
##########
@@ -48,6 +49,14 @@ def show_config(args):
def get_value(args):
"""Get one value from configuration."""
+ # while this will make get_value quite a bit slower we must initialize configuration
+ # for providers because we do not know what sections and options will be available after
+ # providers are initialized. Theoretically Providers might add new sections and options
+ # but also override defaults for existing options, so without loading all providers we
Review Comment:
Good point. I was thinkin about some aspects of it that might be good to dicuss.
I will open a devlist dicussion on that as I think it might be something that we should at least discuss (and possibly decide) there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1270703666
##########
airflow/configuration.py:
##########
@@ -1644,6 +1765,61 @@ def remove_all_read_configurations(self):
for section in self.sections():
self.remove_section(section)
+ @property
+ def providers_configuration_loaded(self) -> bool:
+ """Checks if providers have been loaded."""
+ return self._providers_configuration_loaded
+
+ def load_providers_configuration(self):
+ """
+ Loads configuration for providers.
+
+ This should be done after initial configuration have been performed. Initializing and discovering
+ providers is an expensive operation and cannot be performed when we load configuration for the first
+ time when airflow starts, because we initialize configuration very early, during importing of the
+ `airflow` package and the module is not yet ready to be used when it happens and until configuration
+ and settings are loaded. Therefore, in order to reload provider configuration we need to additionally
+ load provider - specific configuration.
+ """
+ log.debug("Loading providers configuration")
+ from airflow.providers_manager import ProvidersManager
+
+ for provider, config in ProvidersManager().already_initialized_provider_configs:
+ for provider_section, provider_section_content in config.items():
+ provider_options = provider_section_content["options"]
+ section_in_current_config = self.configuration_description.get(provider_section)
+ if not section_in_current_config:
+ # merge whole sections into existing config
+ self.configuration_description[provider_section] = deepcopy(provider_section_content)
+ section_in_current_config = self.configuration_description.get(provider_section)
+ for option in provider_options:
+ section_in_current_config["options"][option]["source"] = f"default-{provider}"
+ else:
+ for option in provider_options:
+ option_in_current_config = section_in_current_config["options"].get(option)
+ if not option_in_current_config:
+ # merge new options into existing sections
+ self.configuration_description[provider_section]["options"][option] = deepcopy(
+ provider_options[option]
+ )
+ option_in_current_config = section_in_current_config["options"].get(option)
+ option_in_current_config["source"] = f"default-{provider}"
+ else:
+ # replace default values for existing options
Review Comment:
OK. I assumed the result of the discussion (at least I am leaning towards it) is that only whole sections can be contributed. Updated it and added test cases covering it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ferruzzi commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1265768024
##########
TESTING.rst:
##########
@@ -55,8 +55,33 @@ Follow the guidelines when writing unit tests:
tests, so we run Pytest with ``--disable-warnings`` but instead we have ``pytest-capture-warnings`` plugin that
overrides ``recwarn`` fixture behaviour.
-**NOTE:** We plan to convert all unit tests to standard "asserts" semi-automatically, but this will be done later
-in Airflow 2.0 development phase. That will include setUp/tearDown/context managers and decorators.
+
+.. note::
+
+ We are in the process of convert all unit tests to standard "asserts" and pytest fixtures
+ so if you find some tests that are still using classic setUp/tearDown approach or unittest asserts, feel
+ free to convert them to pytest.
+
+Airflow configuration for unit tests
+------------------------------------
+
+Some of the unit tests require special configuration set as "default" one. This is done automatically by
+automatically adding ``AIRFLOW__CORE__UNIT_TEST_MODE=True`` to the environment variables in Pytest auto-used
+fixture. This in turn makes airflow configuration load test configuration from
+``airflow/config_templates/config_unit_test.yml`` - defaults from the test configuration replace the original
+defaults stored in ``airflow/config_templates/config.yml``. If you want to add some test-only configuration,
+you should copy the option definition to ``config_unit_test.yml`` and change default value there.
Review Comment:
Lots of repetition makes this hard to parse. I'm going to attempt to rephrase it, feel free to take it or now.
```suggestion
Some unit tests require special configurations set as "default". A Pytest auto-use fixture automatically adds ``AIRFLOW__CORE__UNIT_TEST_MODE=True`` to the environment variables to handle this. This, in turn, loads the test's configuration from
``airflow/config_templates/config_unit_test.yml``. Defaults from the test configuration replace the original
defaults stored in ``airflow/config_templates/config.yml``. If you want to add some test-only configuration,
you should copy the option definition to ``config_unit_test.yml`` and change default values there.
```
##########
TESTING.rst:
##########
@@ -55,8 +55,33 @@ Follow the guidelines when writing unit tests:
tests, so we run Pytest with ``--disable-warnings`` but instead we have ``pytest-capture-warnings`` plugin that
overrides ``recwarn`` fixture behaviour.
-**NOTE:** We plan to convert all unit tests to standard "asserts" semi-automatically, but this will be done later
-in Airflow 2.0 development phase. That will include setUp/tearDown/context managers and decorators.
+
+.. note::
+
+ We are in the process of convert all unit tests to standard "asserts" and pytest fixtures
+ so if you find some tests that are still using classic setUp/tearDown approach or unittest asserts, feel
+ free to convert them to pytest.
+
+Airflow configuration for unit tests
+------------------------------------
+
+Some of the unit tests require special configuration set as "default" one. This is done automatically by
+automatically adding ``AIRFLOW__CORE__UNIT_TEST_MODE=True`` to the environment variables in Pytest auto-used
+fixture. This in turn makes airflow configuration load test configuration from
+``airflow/config_templates/config_unit_test.yml`` - defaults from the test configuration replace the original
+defaults stored in ``airflow/config_templates/config.yml``. If you want to add some test-only configuration,
+you should copy the option definition to ``config_unit_test.yml`` and change default value there.
+
+.. note::
+
+ The test configuration for Airflow before July 2023 was automatically generated in a file named
+ ``AIRFLOW_HOME/unittest.cfg`` and template for it was stored in "config_templates" next to the yaml file,
+ however this was only done for the first time you run airflow and you had to manually maintain the file,
+ and it was pretty cumbrsome and arcane knowledge, also this file has been overwritten in Breeze environment
+ with another CI-specific file. Using ````config_unit_test.yml`` as a single source of configuration for
+ tests is much more convenient and it is automatically used by Pytest. If you have the automatically
+ generated ``unittest.cfg`` file it will be automatically removed when you run airflow.
+
Review Comment:
```suggestion
The test configuration for Airflow before July 2023 was automatically generated in a file named
``AIRFLOW_HOME/unittest.cfg`` and the template for it was stored in "config_templates" next to the yaml file. This was only done for the first time you run Airflow and you had to manually maintain the file, which was pretty cumbersome and arcane knowledge. This file has been overwritten in the Breeze environment with another CI-specific file. Using ````config_unit_test.yml`` as a single source of configuration for tests is much more convenient and it is automatically used by Pytest. If you have the automatically generated ``unittest.cfg`` file it will be automatically removed when you run Airflow.
```
##########
airflow/config_templates/config_unit_tests.yml:
##########
@@ -0,0 +1,221 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# Defaults specified in this file replace the default values from
+# config.yml file during unit tests. If you want a value to have
+# different default during unit tests, specify it here.
+
+---
+
+core:
+ description: ~
+ options:
+ dags_folder:
+ description: |
+ The folder where your airflow pipelines live, most likely a
+ subfolder in a code repository. This path must be absolute.
+ version_added: ~
+ type: string
+ example: ~
+ default: "{AIRFLOW_HOME}/dags"
+ executor:
+ description: |
+ The executor class that airflow should use. Choices include
Review Comment:
```suggestion
The executor class that Airflow should use. Choices include
```
##########
airflow/config_templates/config_unit_tests.yml:
##########
@@ -0,0 +1,221 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# Defaults specified in this file replace the default values from
+# config.yml file during unit tests. If you want a value to have
+# different default during unit tests, specify it here.
+
+---
+
+core:
+ description: ~
+ options:
+ dags_folder:
+ description: |
+ The folder where your airflow pipelines live, most likely a
+ subfolder in a code repository. This path must be absolute.
+ version_added: ~
+ type: string
+ example: ~
+ default: "{AIRFLOW_HOME}/dags"
+ executor:
+ description: |
+ The executor class that airflow should use. Choices include
+ ``SequentialExecutor``, ``LocalExecutor``, ``CeleryExecutor``, ``DaskExecutor``,
+ ``KubernetesExecutor``, ``CeleryKubernetesExecutor`` or the
+ full import path to the class when using a custom executor.
+ version_added: ~
+ type: string
+ example: ~
+ default: "LocalExecutor"
+ dags_are_paused_at_creation:
+ description: |
+ Are DAGs paused by default at creation
+ version_added: ~
+ type: string
+ example: ~
+ default: "False"
+ load_examples:
+ description: |
+ Whether to load the DAG examples that ship with Airflow. It's good to
+ get started, but you probably want to set this to ``False`` in a production
+ environment
+ version_added: ~
+ type: string
+ example: ~
+ default: "True"
+ fernet_key:
+ description: |
+ Secret key to save connection passwords in the db
+ version_added: ~
+ type: string
+ sensitive: true
+ example: ~
+ default: "{FERNET_KEY}"
+ donot_pickle:
+ description: |
+ Whether to disable pickling dags
+ version_added: ~
+ type: string
+ example: ~
+ default: "False"
+ default_impersonation:
+ description: |
+ If set, tasks without a ``run_as_user`` argument will be run with this user
+ Can be used to de-elevate a sudo user running Airflow when executing tasks
+ version_added: ~
+ type: string
+ example: ~
+ default: ""
+ unit_test_mode:
+ description: |
+ Turn unit test mode on (overwrites many configuration options with test
+ values at runtime)
+ version_added: ~
+ type: string
+ example: ~
+ default: "True"
+database:
+ description: ~
+ options:
+ sql_alchemy_conn:
+ description: |
+ The SqlAlchemy connection string to the metadata database.
+ SqlAlchemy supports many different database engines.
+ More information here:
+ http://airflow.apache.org/docs/apache-airflow/stable/howto/set-up-database.html#database-uri
+ version_added: 2.3.0
+ type: string
+ sensitive: true
+ example: ~
+ default: "sqlite:///{AIRFLOW_HOME}/airflow.db"
+ load_default_connections:
+ description: |
+ Whether to load the default connections that ship with Airflow. It's good to
+ get started, but you probably want to set this to ``False`` in a production
+ environment
+ version_added: 2.3.0
+ type: string
+ example: ~
+ default: "True"
+logging:
+ description: ~
+ options:
+ base_log_folder:
+ description: |
+ The folder where airflow should store its log files.
+ This path must be absolute.
+ There are a few existing configurations that assume this is set to the default.
+ If you choose to override this you may need to update the dag_processor_manager_log_location and
+ dag_processor_manager_log_location settings as well.
+ version_added: 2.0.0
+ type: string
+ example: ~
+ default: "{AIRFLOW_HOME}/logs"
+hive:
+ description: ~
+ options:
+ default_hive_mapred_queue:
+ description: |
+ Default mapreduce queue for HiveOperator tasks
+ version_added: ~
+ type: string
+ example: ~
+ default: "airflow"
+smtp:
+ description: |
+ If you want airflow to send emails on retries, failure, and you want to use
Review Comment:
```suggestion
If you want Airflow to send emails on retries, failure, and you want to use
```
##########
airflow/config_templates/config_unit_tests.yml:
##########
@@ -0,0 +1,221 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# Defaults specified in this file replace the default values from
+# config.yml file during unit tests. If you want a value to have
+# different default during unit tests, specify it here.
+
+---
+
+core:
+ description: ~
+ options:
+ dags_folder:
+ description: |
+ The folder where your airflow pipelines live, most likely a
Review Comment:
```suggestion
The folder where your Airflow pipelines live, most likely a
```
##########
airflow/config_templates/config_unit_tests.yml:
##########
@@ -0,0 +1,221 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# Defaults specified in this file replace the default values from
+# config.yml file during unit tests. If you want a value to have
+# different default during unit tests, specify it here.
+
+---
+
+core:
+ description: ~
+ options:
+ dags_folder:
+ description: |
+ The folder where your airflow pipelines live, most likely a
+ subfolder in a code repository. This path must be absolute.
+ version_added: ~
+ type: string
+ example: ~
+ default: "{AIRFLOW_HOME}/dags"
+ executor:
+ description: |
+ The executor class that airflow should use. Choices include
+ ``SequentialExecutor``, ``LocalExecutor``, ``CeleryExecutor``, ``DaskExecutor``,
+ ``KubernetesExecutor``, ``CeleryKubernetesExecutor`` or the
+ full import path to the class when using a custom executor.
+ version_added: ~
+ type: string
+ example: ~
+ default: "LocalExecutor"
+ dags_are_paused_at_creation:
+ description: |
+ Are DAGs paused by default at creation
Review Comment:
Absolutely the smallest of nitpicks, some Description fields end with punctuation, others don't.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1266151753
##########
airflow/cli/cli_config.py:
##########
@@ -793,6 +814,13 @@ def string_lower_type(val):
action="store_true",
)
+# IMPORTANT NOTE!
+#
+# Celery configs below have explicit fallback values because celery provider defaults are not yet loaded
+# via provider at the time we parse the command line, so in case it is not set, we need to have manual
+# fallback
+# DO NOT REMOVE THE FALLBACKS even if you are tempted to.
+# TODO: possibly move the commands to providers but that could be big performance hit on the CLI
Review Comment:
There is one concern I have, after my chnages, some of the commands (celery, kubernetes ) will have to initialize ProvidersManager's initialize_configuration() - which is going to be costly and it cannot be done during parsing of the arguments, so I am afraid extracting them to providers are not going to be "really" possible. I think we will have to eventually leave the CLI definition for Celery and Kubernetes commands in the core because of that. But I suggest we wait for it until we see it, we can likely have it separated out, but not moved to inside provider's code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ferruzzi commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1266024635
##########
airflow/configuration.py:
##########
@@ -136,30 +139,121 @@ def _default_config_file_path(file_name: str) -> str:
return os.path.join(templates_dir, file_name)
-def default_config_yaml() -> dict[str, Any]:
+def default_configuration_description(
+ include_airflow: bool = True,
+ include_providers: bool = True,
+ selected_provider: str | None = None,
+ config_file_name: str = "config.yml",
+) -> dict[str, dict[str, Any]]:
"""
- Read Airflow configs from YAML file.
+ Read Airflow configs from YAML file, and merge them with provider configs optionally.
+ :param include_airflow: Include Airflow configs
+ :param include_providers: Include provider configs
+ :param selected_provider: If specified, include selected provider only
+ :param config_file_name: name of the file in "config_templates" directory to read default config from
:return: Python dictionary containing configs & their info
"""
- with open(_default_config_file_path("config.yml")) as config_file:
- return yaml.safe_load(config_file)
+ base_configuration_description: dict[str, dict[str, Any]] = {}
+ if include_airflow:
+ with open(_default_config_file_path(config_file_name)) as config_file:
+ base_configuration_description.update(yaml.safe_load(config_file))
+ if include_providers:
+ from airflow.providers_manager import ProvidersManager
+
+ for provider, config in ProvidersManager().provider_configs:
+ if selected_provider and provider != selected_provider:
+ continue
+ base_configuration_description.update(config)
+ return base_configuration_description
+
+
+def dejinjafy(value: Any) -> str:
+ if isinstance(value, str):
+ if "{{{{" in value:
+ return value.replace("{{{{", "{{").replace("}}}}", "}}")
+ return value.replace("{{", "{").replace("}}", "}")
+ return value
Review Comment:
I looked at that line for so long, convinced there was a better way LOL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1265958755
##########
airflow/configuration.py:
##########
@@ -136,30 +139,121 @@ def _default_config_file_path(file_name: str) -> str:
return os.path.join(templates_dir, file_name)
-def default_config_yaml() -> dict[str, Any]:
+def default_configuration_description(
+ include_airflow: bool = True,
+ include_providers: bool = True,
+ selected_provider: str | None = None,
+ config_file_name: str = "config.yml",
+) -> dict[str, dict[str, Any]]:
"""
- Read Airflow configs from YAML file.
+ Read Airflow configs from YAML file, and merge them with provider configs optionally.
+ :param include_airflow: Include Airflow configs
+ :param include_providers: Include provider configs
+ :param selected_provider: If specified, include selected provider only
+ :param config_file_name: name of the file in "config_templates" directory to read default config from
:return: Python dictionary containing configs & their info
"""
- with open(_default_config_file_path("config.yml")) as config_file:
- return yaml.safe_load(config_file)
+ base_configuration_description: dict[str, dict[str, Any]] = {}
+ if include_airflow:
+ with open(_default_config_file_path(config_file_name)) as config_file:
+ base_configuration_description.update(yaml.safe_load(config_file))
+ if include_providers:
+ from airflow.providers_manager import ProvidersManager
+
+ for provider, config in ProvidersManager().provider_configs:
+ if selected_provider and provider != selected_provider:
+ continue
+ base_configuration_description.update(config)
+ return base_configuration_description
+
+
+def dejinjafy(value: Any) -> str:
+ if isinstance(value, str):
+ if "{{{{" in value:
+ return value.replace("{{{{", "{{").replace("}}}}", "}}")
+ return value.replace("{{", "{").replace("}}", "}")
+ return value
class AirflowConfigParser(ConfigParser):
"""Custom Airflow Configparser supporting defaults and deprecated options."""
+ def __init__(
+ self,
+ default_config: str | dict[str, dict[str, Any]] | None = None,
+ default_config_description: dict[str, dict[str, Any]] | None = None,
+ *args,
+ **kwargs,
+ ):
+ super().__init__(*args, **kwargs)
+ self.default_config_description = default_config_description
+ self.upgraded_values = {}
+ self._default_values: dict[str, dict[str, Any]] = {}
+ if default_config is not None:
Review Comment:
Might be worth inverting this conditional, the else is pretty far away.
##########
airflow/configuration.py:
##########
@@ -327,32 +421,100 @@ def inversed_deprecated_sections(self):
def optionxform(self, optionstr: str) -> str:
return optionstr
- def __init__(self, default_config: str | None = None, *args, **kwargs):
- super().__init__(*args, **kwargs)
- self.upgraded_values = {}
-
- self.airflow_defaults = ConfigParser(*args, **kwargs)
- if default_config is not None:
- self.airflow_defaults.read_string(default_config)
- # Set the upgrade value based on the current loaded default
- default = self.airflow_defaults.get("logging", "log_filename_template", fallback=None)
- if default:
- replacement = self.deprecated_values["logging"]["log_filename_template"]
- self.deprecated_values["logging"]["log_filename_template"] = (
- replacement[0],
- default,
- replacement[2],
- )
- else:
- # In case of tests it might not exist
- with suppress(KeyError):
- del self.deprecated_values["logging"]["log_filename_template"]
- else:
- with suppress(KeyError):
- del self.deprecated_values["logging"]["log_filename_template"]
-
- self.is_validated = False
- self._suppress_future_warnings = False
+ def write( # type: ignore[override]
+ self,
+ file: IO[str],
+ section: str | None = None,
+ include_examples: bool = True,
+ include_descriptions: bool = True,
+ include_sources: bool = True,
+ include_variables: bool = True,
+ hide_sensitive_values: bool = False,
+ only_defaults: bool = False,
+ **kwargs: Any,
+ ) -> None:
+ """
+ Write configuration with comments and examples to a file.
Review Comment:
```suggestion
Write configuration with comments and examples to a file.
```
super nit :)
##########
airflow/configuration.py:
##########
@@ -136,30 +139,121 @@ def _default_config_file_path(file_name: str) -> str:
return os.path.join(templates_dir, file_name)
-def default_config_yaml() -> dict[str, Any]:
+def default_configuration_description(
+ include_airflow: bool = True,
+ include_providers: bool = True,
+ selected_provider: str | None = None,
+ config_file_name: str = "config.yml",
+) -> dict[str, dict[str, Any]]:
"""
- Read Airflow configs from YAML file.
+ Read Airflow configs from YAML file, and merge them with provider configs optionally.
+ :param include_airflow: Include Airflow configs
+ :param include_providers: Include provider configs
+ :param selected_provider: If specified, include selected provider only
+ :param config_file_name: name of the file in "config_templates" directory to read default config from
:return: Python dictionary containing configs & their info
"""
- with open(_default_config_file_path("config.yml")) as config_file:
- return yaml.safe_load(config_file)
+ base_configuration_description: dict[str, dict[str, Any]] = {}
+ if include_airflow:
+ with open(_default_config_file_path(config_file_name)) as config_file:
+ base_configuration_description.update(yaml.safe_load(config_file))
+ if include_providers:
+ from airflow.providers_manager import ProvidersManager
+
+ for provider, config in ProvidersManager().provider_configs:
+ if selected_provider and provider != selected_provider:
+ continue
+ base_configuration_description.update(config)
+ return base_configuration_description
+
+
+def dejinjafy(value: Any) -> str:
+ if isinstance(value, str):
+ if "{{{{" in value:
+ return value.replace("{{{{", "{{").replace("}}}}", "}}")
+ return value.replace("{{", "{").replace("}}", "}")
+ return value
Review Comment:
We are just concerned with up to 4 curlies right? This is simpler:
```suggestion
if not isinstance(value, str):
return value
return value.replace("{{", "{").replace("}}", "}")
```
This does the 4->2 and 2->1.
##########
airflow/providers/celery/executors/celery_kubernetes_executor.py:
##########
@@ -55,14 +55,21 @@ class CeleryKubernetesExecutor(LoggingMixin):
callback_sink: BaseCallbackSink | None = None
- KUBERNETES_QUEUE = conf.get("celery_kubernetes_executor", "kubernetes_queue")
+ @property
Review Comment:
```suggestion
@cached_property
```
This should be safe to do. (It does need to be imported though)
##########
airflow/configuration.py:
##########
@@ -1474,84 +1679,123 @@ def _generate_fernet_key() -> str:
return Fernet.generate_key().decode()
-def initialize_config() -> AirflowConfigParser:
+def _create_values_from_config_description(
+ default_config_description: dict[str, dict[str, Any]]
+) -> dict[str, dict[str, Any]]:
"""
- Load the Airflow config files.
+ Creates dictionary of default values from configuration description, using
+ based on variables defined in this module (expansion variables).
- Called for you automatically as part of the Airflow boot process.
+ :param default_config_description: description of configuration to read including defaults
+ :return: dictionary of default values based on description and variables defined in this module
"""
- global FERNET_KEY, AIRFLOW_HOME, WEBSERVER_CONFIG
-
- default_config = _parameterized_config_from_template("default_airflow.cfg")
+ all_vars = get_all_expansion_variables()
+ default_values: dict[str, dict[str, Any]] = {}
+ for section, section_desc in default_config_description.items():
+ section_dict: dict[str, Any] = {}
+ default_values[section] = section_dict
+ options = section_desc["options"]
+ for key in options:
+ default_value = options[key]["default"]
+ if default_value is not None:
+ section_dict[key] = (
+ default_value.format(**all_vars) if isinstance(default_value, str) else default_value
+ )
+ return default_values
- local_conf = AirflowConfigParser(default_config=default_config)
- if local_conf.getboolean("core", "unit_test_mode"):
- # Load test config only
- if not os.path.isfile(TEST_CONFIG_FILE):
- from cryptography.fernet import Fernet
+def create_airflow_config_parser(default_config_description: dict[str, dict[str, Any]]):
+ """
+ Creates Airflow config parser.
- log.info("Creating new Airflow config file for unit tests in: %s", TEST_CONFIG_FILE)
- pathlib.Path(AIRFLOW_HOME).mkdir(parents=True, exist_ok=True)
+ Creates Airflow config parser based on variables defined in this module (expansion variables)
+ and default configuration description passed as parameter which serves as source of default values.
- FERNET_KEY = Fernet.generate_key().decode()
+ :param default_config_description: default configuration description - retrieved from config*.yaml files
+ following the schema defined in "config.yml.schema.json" in the config_templates folder.
+ :return: Airflow config parser that can be used to read configuration values from.
+ """
+ return AirflowConfigParser(
+ default_config=_create_values_from_config_description(default_config_description),
+ default_config_description=default_config_description,
+ )
- with open(TEST_CONFIG_FILE, "w") as file:
- cfg = _parameterized_config_from_template("default_test.cfg")
- file.write(cfg)
- make_group_other_inaccessible(TEST_CONFIG_FILE)
- local_conf.load_test_config()
- else:
- # Load normal config
- if not os.path.isfile(AIRFLOW_CONFIG):
- from cryptography.fernet import Fernet
+def load_standard_airflow_configuration(airflow_config_parser: AirflowConfigParser):
+ """
+ Loads standard airflow configuration.
- log.info("Creating new Airflow config file in: %s", AIRFLOW_CONFIG)
- pathlib.Path(AIRFLOW_HOME).mkdir(parents=True, exist_ok=True)
+ In case it finds that the configuration file is missing, it will create it and write the default
+ configuration values there, based on defaults passed, and will add the comments and examples
+ from the default configuration description passed.
- FERNET_KEY = Fernet.generate_key().decode()
+ :param airflow_config_parser: parser to which the configuration will be loaded
+ """
+ global FERNET_KEY, AIRFLOW_HOME
+ if not os.path.isfile(AIRFLOW_CONFIG):
+ from cryptography.fernet import Fernet
+
+ log.info("Creating new Airflow config file in: %s", AIRFLOW_CONFIG)
+ pathlib.Path(AIRFLOW_HOME).mkdir(parents=True, exist_ok=True)
+ FERNET_KEY = Fernet.generate_key().decode()
+ if airflow_config_parser._default_values and airflow_config_parser.default_config_description:
with open(AIRFLOW_CONFIG, "w") as file:
- file.write(default_config)
- make_group_other_inaccessible(AIRFLOW_CONFIG)
-
- log.info("Reading the config from %s", AIRFLOW_CONFIG)
+ airflow_config_parser.write(file)
+ make_group_other_inaccessible(AIRFLOW_CONFIG)
+ log.info("Reading the config from %s", AIRFLOW_CONFIG)
+ airflow_config_parser.read(AIRFLOW_CONFIG)
+ if airflow_config_parser.has_option("core", "AIRFLOW_HOME"):
+ msg = (
+ "Specifying both AIRFLOW_HOME environment variable and airflow_home "
+ "in the config file is deprecated. Please use only the AIRFLOW_HOME "
+ "environment variable and remove the config file entry."
+ )
+ if "AIRFLOW_HOME" in os.environ:
+ warnings.warn(msg, category=DeprecationWarning)
+ elif airflow_config_parser.get("core", "airflow_home") == AIRFLOW_HOME:
+ warnings.warn(
+ "Specifying airflow_home in the config file is deprecated. As you "
+ "have left it at the default value you should remove the setting "
+ "from your airflow.cfg and suffer no change in behaviour.",
+ category=DeprecationWarning,
+ )
+ else:
+ # there
+ AIRFLOW_HOME = airflow_config_parser.get("core", "airflow_home") # type: ignore[assignment]
+ warnings.warn(msg, category=DeprecationWarning)
- local_conf.read(AIRFLOW_CONFIG)
- if local_conf.has_option("core", "AIRFLOW_HOME"):
- msg = (
- "Specifying both AIRFLOW_HOME environment variable and airflow_home "
- "in the config file is deprecated. Please use only the AIRFLOW_HOME "
- "environment variable and remove the config file entry."
- )
- if "AIRFLOW_HOME" in os.environ:
- warnings.warn(msg, category=DeprecationWarning)
- elif local_conf.get("core", "airflow_home") == AIRFLOW_HOME:
- warnings.warn(
- "Specifying airflow_home in the config file is deprecated. As you "
- "have left it at the default value you should remove the setting "
- "from your airflow.cfg and suffer no change in behaviour.",
- category=DeprecationWarning,
- )
- else:
- # there
- AIRFLOW_HOME = local_conf.get("core", "airflow_home") # type: ignore[assignment]
- warnings.warn(msg, category=DeprecationWarning)
+def initialize_config() -> AirflowConfigParser:
+ """
+ Load the Airflow config files.
- # They _might_ have set unit_test_mode in the airflow.cfg, we still
- # want to respect that and then load the unittests.cfg
- if local_conf.getboolean("core", "unit_test_mode"):
- local_conf.load_test_config()
+ Called for you automatically as part of the Airflow boot process.
+ """
+ global FERNET_KEY, AIRFLOW_HOME, WEBSERVER_CONFIG
Review Comment:
```suggestion
global WEBSERVER_CONFIG
```
Don't we just need this one? We aren't doing anything with the other 2?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1268165435
##########
airflow/configuration.py:
##########
@@ -136,30 +139,121 @@ def _default_config_file_path(file_name: str) -> str:
return os.path.join(templates_dir, file_name)
-def default_config_yaml() -> dict[str, Any]:
+def default_configuration_description(
+ include_airflow: bool = True,
+ include_providers: bool = True,
+ selected_provider: str | None = None,
+ config_file_name: str = "config.yml",
+) -> dict[str, dict[str, Any]]:
"""
- Read Airflow configs from YAML file.
+ Read Airflow configs from YAML file, and merge them with provider configs optionally.
+ :param include_airflow: Include Airflow configs
+ :param include_providers: Include provider configs
+ :param selected_provider: If specified, include selected provider only
+ :param config_file_name: name of the file in "config_templates" directory to read default config from
:return: Python dictionary containing configs & their info
"""
- with open(_default_config_file_path("config.yml")) as config_file:
- return yaml.safe_load(config_file)
+ base_configuration_description: dict[str, dict[str, Any]] = {}
+ if include_airflow:
+ with open(_default_config_file_path(config_file_name)) as config_file:
+ base_configuration_description.update(yaml.safe_load(config_file))
+ if include_providers:
+ from airflow.providers_manager import ProvidersManager
+
+ for provider, config in ProvidersManager().provider_configs:
+ if selected_provider and provider != selected_provider:
+ continue
+ base_configuration_description.update(config)
+ return base_configuration_description
+
+
+def dejinjafy(value: Any) -> str:
+ if isinstance(value, str):
+ if "{{{{" in value:
+ return value.replace("{{{{", "{{").replace("}}}}", "}}")
+ return value.replace("{{", "{").replace("}}", "}")
+ return value
class AirflowConfigParser(ConfigParser):
"""Custom Airflow Configparser supporting defaults and deprecated options."""
+ def __init__(
+ self,
+ default_config: str | dict[str, dict[str, Any]] | None = None,
+ default_config_description: dict[str, dict[str, Any]] | None = None,
+ *args,
+ **kwargs,
+ ):
+ super().__init__(*args, **kwargs)
+ self.default_config_description = default_config_description
+ self.upgraded_values = {}
+ self._default_values: dict[str, dict[str, Any]] = {}
+ if default_config is not None:
Review Comment:
It is addressed now I think :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1269336209
##########
airflow/cli/cli_config.py:
##########
@@ -818,6 +823,13 @@ def string_lower_type(val):
action="store_true",
)
+# IMPORTANT NOTE!
+#
+# Celery configs below have explicit fallback values because celery provider defaults are not yet loaded
+# via provider at the time we parse the command line, so in case it is not set, we need to have manual
+# fallback
+# DO NOT REMOVE THE FALLBACKS even if you are tempted to.
+# TODO: possibly move the commands to providers but that could be big performance hit on the CLI
Review Comment:
No - because it only applies to celery configuration.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32604:
URL: https://github.com/apache/airflow/pull/32604#issuecomment-1635952877
Also some docs screenshots:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32604:
URL: https://github.com/apache/airflow/pull/32604#issuecomment-1643938711
OK. I think this PR should be ready for final review and has a chance to get green. It is already quite small in fact after extracting the other changes from it - and mostly builds on top of what has been already merged in the previous PRs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1269330352
##########
docs/apache-airflow/core-concepts/executor/celery.rst:
##########
@@ -29,6 +29,8 @@ change your ``airflow.cfg`` to point the executor parameter to
For more information about setting up a Celery broker, refer to the
exhaustive `Celery documentation on the topic <https://docs.celeryq.dev/en/latest/getting-started/>`_.
+The configuration parameters of the Celery Executor can be found in :doc:`apache-airflow-providers-celery:configurations-ref`.
Review Comment:
I think it's better to have them as just regular paragraphe.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1268172384
##########
docs/apache-airflow-providers/index.rst:
##########
@@ -51,6 +51,15 @@ provider package and re-start Airflow, those become automatically available to A
The summary of the core functionalities that can be extended are available in
:doc:`/core-extensions/index`.
+Configuration
+'''''''''''''
+
+The providers can have their own configuration options, That allow you to configure the way how they
+work:
+
+You can see all the community-managed providers with their own configuration in
Review Comment:
updated.
##########
docs/apache-airflow-providers/index.rst:
##########
@@ -51,6 +51,15 @@ provider package and re-start Airflow, those become automatically available to A
The summary of the core functionalities that can be extended are available in
:doc:`/core-extensions/index`.
+Configuration
+'''''''''''''
+
+The providers can have their own configuration options, That allow you to configure the way how they
+work:
Review Comment:
updated,
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1268171284
##########
docs/exts/logging.rst.jinja2:
##########
@@ -22,7 +22,7 @@
:doc:`{{ provider }}:logging/index`
-{% for handler in provider_dict['handlers'] -%}
+{% for handler in provider_dict['logging'] -%}
Review Comment:
Yep. It's ok. Double checked.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32604:
URL: https://github.com/apache/airflow/pull/32604#issuecomment-1639333733
I addressed most point (they are coming in the new extracted PR with configuration "consolidation"). Those that are not "resolved" yet I will address is the final one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1266214093
##########
airflow/config_templates/config_unit_tests.yml:
##########
@@ -0,0 +1,221 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# Defaults specified in this file replace the default values from
+# config.yml file during unit tests. If you want a value to have
+# different default during unit tests, specify it here.
+
+---
+
+core:
+ description: ~
+ options:
+ dags_folder:
+ description: |
+ The folder where your airflow pipelines live, most likely a
+ subfolder in a code repository. This path must be absolute.
+ version_added: ~
+ type: string
+ example: ~
+ default: "{AIRFLOW_HOME}/dags"
+ executor:
+ description: |
+ The executor class that airflow should use. Choices include
+ ``SequentialExecutor``, ``LocalExecutor``, ``CeleryExecutor``, ``DaskExecutor``,
+ ``KubernetesExecutor``, ``CeleryKubernetesExecutor`` or the
+ full import path to the class when using a custom executor.
+ version_added: ~
+ type: string
+ example: ~
+ default: "LocalExecutor"
+ dags_are_paused_at_creation:
+ description: |
+ Are DAGs paused by default at creation
Review Comment:
I will address those separately. I do not want to mix "real" change in config to make it easier for review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1269955978
##########
airflow/configuration.py:
##########
@@ -1644,6 +1765,61 @@ def remove_all_read_configurations(self):
for section in self.sections():
self.remove_section(section)
+ @property
+ def providers_configuration_loaded(self) -> bool:
+ """Checks if providers have been loaded."""
+ return self._providers_configuration_loaded
+
+ def load_providers_configuration(self):
+ """
+ Loads configuration for providers.
+
+ This should be done after initial configuration have been performed. Initializing and discovering
+ providers is an expensive operation and cannot be performed when we load configuration for the first
+ time when airflow starts, because we initialize configuration very early, during importing of the
+ `airflow` package and the module is not yet ready to be used when it happens and until configuration
+ and settings are loaded. Therefore, in order to reload provider configuration we need to additionally
+ load provider - specific configuration.
+ """
+ log.debug("Loading providers configuration")
+ from airflow.providers_manager import ProvidersManager
+
+ for provider, config in ProvidersManager().already_initialized_provider_configs:
+ for provider_section, provider_section_content in config.items():
+ provider_options = provider_section_content["options"]
+ section_in_current_config = self.configuration_description.get(provider_section)
+ if not section_in_current_config:
+ # merge whole sections into existing config
+ self.configuration_description[provider_section] = deepcopy(provider_section_content)
+ section_in_current_config = self.configuration_description.get(provider_section)
+ for option in provider_options:
+ section_in_current_config["options"][option]["source"] = f"default-{provider}"
+ else:
+ for option in provider_options:
+ option_in_current_config = section_in_current_config["options"].get(option)
+ if not option_in_current_config:
+ # merge new options into existing sections
+ self.configuration_description[provider_section]["options"][option] = deepcopy(
+ provider_options[option]
+ )
+ option_in_current_config = section_in_current_config["options"].get(option)
+ option_in_current_config["source"] = f"default-{provider}"
+ else:
+ # replace default values for existing options
Review Comment:
Yeah, this feels wrong to me.
What if more than 1 provider decided to override a certain value?
I'd rather we raise here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1270702096
##########
airflow/providers/celery/provider.yaml:
##########
@@ -60,3 +60,256 @@ sensors:
executors:
- airflow.providers.celery.executors.celery_executor.CeleryExecutor
- airflow.providers.celery.executors.celery_kubernetes_executor.CeleryKubernetesExecutor
+
+config:
+ celery_kubernetes_executor:
+ description: |
+ This section only applies if you are using the ``CeleryKubernetesExecutor`` in
+ ``[core]`` section above
+ options:
+ kubernetes_queue:
+ description: |
+ Define when to send a task to ``KubernetesExecutor`` when using ``CeleryKubernetesExecutor``.
+ When the queue of a task is the value of ``kubernetes_queue`` (default ``kubernetes``),
+ the task is executed via ``KubernetesExecutor``,
+ otherwise via ``CeleryExecutor``
+ version_added: 2.0.0
Review Comment:
Removed all version_added except one which was **just** added and I marked it as version_added: 3.3.0 (upcoming celery provider version).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ferruzzi commented on a diff in pull request #32604: Allow configuration to be contributed by providers
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on code in PR #32604:
URL: https://github.com/apache/airflow/pull/32604#discussion_r1265782734
##########
docs/apache-airflow-providers/index.rst:
##########
@@ -51,6 +51,15 @@ provider package and re-start Airflow, those become automatically available to A
The summary of the core functionalities that can be extended are available in
:doc:`/core-extensions/index`.
+Configuration
+'''''''''''''
+
+The providers can have their own configuration options, That allow you to configure the way how they
+work:
Review Comment:
```suggestion
Providers can have their own configuration options which allow you to configure how they work:
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org