You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "uranusjr (via GitHub)" <gi...@apache.org> on 2023/02/16 09:32:54 UTC
[GitHub] [airflow] uranusjr opened a new pull request, #29569: Avoid importing executor during conf validation
uranusjr opened a new pull request, #29569:
URL: https://github.com/apache/airflow/pull/29569
Loading an executor is heavy work that is only needed in a scheduler. We should avoid doing that.
This moves the validation to when the executor is actually loaded; this slightly degrades the UX when there's an invalid configuration (the scheduler fails later), but the gain is worthwhile IMO. For the webserver, for example, the startup time of a "bare" Airflow installation (no providers or plugins) is down >25% on my machine (roughly 8s to 6s).
--
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 merged pull request #29569: Avoid importing executor during conf validation
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr merged PR #29569:
URL: https://github.com/apache/airflow/pull/29569
--
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 #29569: Avoid importing executor during conf validation
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #29569:
URL: https://github.com/apache/airflow/pull/29569#issuecomment-1434273684
I tried a few approaches. Simply calling `job.scheduler` in `scheduler_command` feels wrong to me (although it does the right thing) since it leaks the fact that the attribute is actually a cached property instantiating the default executor. But putting the validation in ExecutionLoader is actually not optimal since that makes the logic runs every time we instantiate an executor (e.g. for backfill).
Right now I’m moving the validation logic to `conf` as a free function and make it cached so it’s called exactly once. We’ll still call it in ExecutorLoader (to make sure all loader calls return a valid executor class), but also call it eagerly in `scheduler_command`.
--
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] Taragolis commented on a diff in pull request #29569: Avoid importing executor during conf validation
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on code in PR #29569:
URL: https://github.com/apache/airflow/pull/29569#discussion_r1111060163
##########
tests/www/views/test_views_tasks.py:
##########
@@ -492,7 +496,7 @@ def test_code_from_db_all_example_dags(admin_client):
dag_id="example_bash_operator",
ignore_all_deps="false",
ignore_ti_state="true",
- execution_date=DEFAULT_DATE,
+ dag_run_id=DEFAULT_DAGRUN,
Review Comment:
I guess we use mock client rather than actually call views here: https://flask.palletsprojects.com/en/2.2.x/testing/#sending-requests-with-the-test-client
https://github.com/apache/airflow/blob/f17e2ba48b59525655a92e04684db664a672918f/tests/www/views/conftest.py#L64-L65
--
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] ashb commented on a diff in pull request #29569: Avoid importing executor during conf validation
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #29569:
URL: https://github.com/apache/airflow/pull/29569#discussion_r1108262943
##########
airflow/configuration.py:
##########
@@ -429,30 +428,34 @@ def _validate_enums(self):
)
def _validate_config_dependencies(self):
- """
- Validate that config based on condition.
+ """Validate that config based on condition.
Values are considered invalid when they conflict with other config values
or system-level limitations and requirements.
"""
- executor, _ = ExecutorLoader.import_default_executor_cls()
- is_sqlite = "sqlite" in self.get("database", "sql_alchemy_conn")
+ self._validate_sqlite3_version()
Review Comment:
Rather than calling this from in here, call this function directly from The existing `validate()` function.
--
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 a diff in pull request #29569: Avoid importing executor during conf validation
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #29569:
URL: https://github.com/apache/airflow/pull/29569#discussion_r1111432584
##########
tests/www/views/test_views_tasks.py:
##########
@@ -492,7 +496,7 @@ def test_code_from_db_all_example_dags(admin_client):
dag_id="example_bash_operator",
ignore_all_deps="false",
ignore_ti_state="true",
- execution_date=DEFAULT_DATE,
+ dag_run_id=DEFAULT_DAGRUN,
Review Comment:
I think the mock client still executes the views (it simply skips the HTTP and directly calls the routers).
--
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 a diff in pull request #29569: Avoid importing executor during conf validation
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #29569:
URL: https://github.com/apache/airflow/pull/29569#discussion_r1109482175
##########
airflow/executors/executor_loader.py:
##########
@@ -41,6 +41,22 @@
from airflow.executors.base_executor import BaseExecutor
+def _validate_database_executor_compatibility(executor: type[BaseExecutor]) -> None:
Review Comment:
Added
--
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 a diff in pull request #29569: Avoid importing executor during conf validation
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #29569:
URL: https://github.com/apache/airflow/pull/29569#discussion_r1109578184
##########
tests/www/views/test_views_tasks.py:
##########
@@ -492,7 +496,7 @@ def test_code_from_db_all_example_dags(admin_client):
dag_id="example_bash_operator",
ignore_all_deps="false",
ignore_ti_state="true",
- execution_date=DEFAULT_DATE,
+ dag_run_id=DEFAULT_DAGRUN,
Review Comment:
How did this ever work before? The view does not accept `execution_date` (not anymore, I think) but only `dag_run_id`.
--
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] andriisoldatenko commented on a diff in pull request #29569: Avoid importing executor during conf validation
Posted by "andriisoldatenko (via GitHub)" <gi...@apache.org>.
andriisoldatenko commented on code in PR #29569:
URL: https://github.com/apache/airflow/pull/29569#discussion_r1108253832
##########
airflow/executors/executor_loader.py:
##########
@@ -41,6 +41,22 @@
from airflow.executors.base_executor import BaseExecutor
+def _validate_database_executor_compatibility(executor: type[BaseExecutor]) -> None:
Review Comment:
do we want to write tests for this function?
--
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