-
Notifications
You must be signed in to change notification settings - Fork 14k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove double warning in CLI when config value is deprecated #40319
Remove double warning in CLI when config value is deprecated #40319
Conversation
There's a double warning when getting a value for a deprecated configuration. This happens because we first check if the config exists, which issues a warning and then getting the config also issues the same warning. Now that we don't error for non-existing configs, we should not check existence before getting a config value
Hmm, the failure seems consistent. Maybe it’s not flaky? |
Yep. Looks very much related |
Locally, when you use --with-db-init, it succeeds every time. I have also noticed that this fails locally at the main branch. Maybe the tests are not run except when the cli commands are modified? |
I'd say it might be a side effect of other tests. You can reproduce it locally by running the same as in ci - i.e. all non-db tests |
https://github.com/apache/airflow/actions/runs/9579997426/job/26433542514?pr=40319#step:7:6
From:
|
Yep. Looks like loading configuration in one test might break other tests -> likely loading confguration in some tests breaks configuration in others. This is a bit of a result of the (bad) approach of ours that configuration is automatically loaded at import time, and if tests are not written carefully with proper initialization setup/teardown, one test might impact another. More why it's happening - when you run tests with xdist there are n parallel workers running the tests and they get the tests in some order, and if there is a tests that changes state and leaves some side effects (like configuration loaded) might impacts another test in the same worker. So basically what we need to do is to track down the tests that are leaving side-effects/ change state and tests that are impacted and a) remove the side effect in the first one (proper teardown) and b) make sure that setup in the failing tests cleans-up the state (configuration in this case) to desired state. |
BTW. I usually guess (if possible) or do bisecting in this case. The actual command that is executed (you can see it in the output) is this
Simply entering breeze and runnig this one:
Should reproduce it ~ 100% (that includes running the tests in 4 workers). You can use one worker Then - when you have reproducible case you can modify this pytest command to see which tests are executed (increase verbosity). Then you can try to run just the tests before and tests after and try to run only the tests before + the test that fails. Then you can generate list of the modules that are executed and run them in the same sequence as you see them in the output.
And then you can bisect the list of tests that are run before by removing half of them (and see if they still fail) - then another half and another. That allows - relatively quickly to find the tests that are causing the problem. eventually you should be able to track it down to single (or sometimes a few) tests that are causing the problem: When this fails test_y:
and this does not:
Then it mens that there is a side effect left by test_x. I used it quite a few times to track down such side-effects. |
One of the failing tests can easily be reproduced by running |
I also marked test_cli_parser.py as db test for similar reasons. My conclusion is that any code that has to load the executor in the cli/ should be marked as DB test: airflow/airflow/cli/cli_parser.py Line 63 in 8217bec
airflow/airflow/executors/executor_loader.py Line 338 in 8217bec
|
Hmm. I will take a look at that one then, maybe there is something we can do to fail such tests explicitly. But I think still it's some kind of side-effect from other tests. |
* Remove double warning in CLI when config value is deprecated There's a double warning when getting a value for a deprecated configuration. This happens because we first check if the config exists, which issues a warning and then getting the config also issues the same warning. Now that we don't error for non-existing configs, we should not check existence before getting a config value * mark test_plugins_command as db_test * mark test_cli_parser.py as db_test (cherry picked from commit 3f8c1e4)
…40319) * Remove double warning in CLI when config value is deprecated There's a double warning when getting a value for a deprecated configuration. This happens because we first check if the config exists, which issues a warning and then getting the config also issues the same warning. Now that we don't error for non-existing configs, we should not check existence before getting a config value * mark test_plugins_command as db_test * mark test_cli_parser.py as db_test
There's a double warning when getting a value for a deprecated configuration. This happens because we first check if the config exists, which issues a warning and then getting the config also issues the same warning. Now that we don't error for non-existing configs, we should not check existence before getting a config value