Add a mechanism to warn if executors override existing CLI commands #33423

vandonr-amz · 2023-08-15T22:29:27Z

Currently, if an executor redefines and existing command (allowed by #29055), I think it silently overrides the existing one, which can be an issue.
I'm in the process of adding one more way to add commands, so doing this seems like a good idea.

Also fixed a test side-effect, which led to fixing a bunch of tests that were only working because of the order in which they were called.

airflow/cli/cli_parser.py

vandonr-amz · 2023-08-16T21:49:12Z

airflow/cli/cli_parser.py

-airflow_commands = core_commands
+airflow_commands = core_commands.copy()  # make a copy to prevent bad interactions in tests


since we do airflow_commands.extend(...) after that, and airflow_commands is just pointing to core_commands, we were actually modifying core_commands, and whatever we were adding in there stayed there for all tests.

vandonr-amz · 2023-08-16T21:50:31Z

tests/cli/conftest.py

-custom_executor_module.CustomCeleryExecutor = type(  # type:  ignore
-    "CustomLocalExecutor", (celery_executor.CeleryExecutor,), {}
-)
-custom_executor_module.CustomCeleryKubernetesExecutor = type(  # type: ignore
-    "CustomLocalKubernetesExecutor", (celery_kubernetes_executor.CeleryKubernetesExecutor,), {}
+custom_executor_module.CustomLocalExecutor = type(  # type:  ignore
+    "CustomLocalExecutor", (local_executor.LocalExecutor,), {}
 )
-custom_executor_module.CustomCeleryExecutor = type(  # type:  ignore
-    "CustomKubernetesExecutor", (celery_executor.CeleryExecutor,), {}
+custom_executor_module.CustomLocalKubernetesExecutor = type(  # type: ignore
+    "CustomLocalKubernetesExecutor", (local_kubernetes_executor.LocalKubernetesExecutor,), {}
 )
-custom_executor_module.CustomCeleryKubernetesExecutor = type(  # type: ignore
-    "CustomCeleryKubernetesExecutor", (celery_kubernetes_executor.CeleryKubernetesExecutor,), {}
+custom_executor_module.CustomKubernetesExecutor = type(  # type:  ignore
+    "CustomKubernetesExecutor", (kubernetes_executor.KubernetesExecutor,), {}


this whole thing was a mess of copy-paste mistakes. Tests were passing only because commands were never removed.

vandonr-amz · 2023-08-16T21:54:08Z

tests/cli/test_cli_parser.py

-            "custom_executor.CustomCeleryKubernetesExecutor",
-        ],
-    )
-    def test_dag_parser_celery_command_accept_celery_executor(self, executor):


this test was originally checking that celery executors were accepting the celery sub-command, but this is now included in the parameterized test below, with the kubernetes command at the same time.

vandonr-amz · 2023-08-16T21:55:47Z

tests/cli/test_cli_parser.py

                    parser.parse_args([expected_arg, "--help"])
-                    assert e.value.code == 0


this assert was never executed because it was in the block that captures the exception

vandonr-amz · 2023-08-16T21:57:40Z

tests/cli/test_cli_parser.py

                stderr = stderr.getvalue()
                assert "airflow command error" not in stderr

-    def test_dag_parser_config_command_dont_required_celery_executor(self):


this was a obsolete test remaining from when there was code to check the executor when executing a command. It was introduced after a bugfix on that code (#17071), but the tested code has been removed since, so it's not testing anything anymore.

vincbeck · 2023-08-17T14:36:35Z

airflow/cli/cli_parser.py

+        f"This can be due to the executor '{ExecutorLoader.get_default_executor_name()}' "
+        f"redefining core airflow CLI commands."


Do we want to have this message? In the future it might no longer be true and other components such as auth managers could be able to also define their own CLI command

yes, we'll need to edit the message then, but in my opinion, we cannot crash the whole thing in the face of the user without giving them as much information as possible on why this crash is happening.

Or maybe the only people who would encounter this error would be developers trying to add new commands and a vague message would be enough because they know what they just modified ?

but I think we cannot entirely rule out the possibility that a user would face this error, especially as we add more "command vendors", the number of combination raises quadratically, and we cannot expect developers to test them all.
We can try to enforce it with "officially" provided components, but with custom components, all bets are off.

Yeah I see what you mean and I dont disagree but at the same time, it might not be related at all to the default executor and then point the wrong direction to the user

I dont disagree but at the same time, it might not be related at all to the default executor and then point the wrong direction to the user

I think the language used in the message is already pretty cautious, it just suggests that the executor could be the issue:

This can be due to the executor ...

We could try soften the language even more, but personally I think this is okay for now

but I think we cannot entirely rule out the possibility that a user would face this error, especially as we add more "command vendors", the number of combination raises quadratically, and we cannot expect developers to test them all.
We can try to enforce it with "officially" provided components, but with custom components, all bets are off.

Yes. But one comment/ suggestion.

This is the same with "config" we handle now in exactly the same way. I think there are two solutions for "all bets off" and potential conflicts:

introduce some kind of central registry of commands and "give them" to those who want certain id (pessimistic, top-down-driven, centralized, does not play well with free/open-source project). This for example what IP addresses, DNS names, IMEI numbers for simcards are about as well as few others.

make those who introduce custom commands painfully aware that THEY have to do everythig to make their commands unique. This is far softer, distributed and more self-regulating. The problem is that those who develop things will have problem if they use non-unique names. So anyone doing "aws executors" ;) should have commands starting with "aws-" to avoid contention. That is more or less what java and python packages follow. Nobody keeps the registry of those, yet somehow everyone takes care to not use generic package names following common conventions ("google.",. "aws.") and it just works (TM).

Obviously 2) is better for us.

This is something we might want to document as "soft convention" and mention why ("When you create a custom config/CLI, you should make sure to use unique-enough prefix indicating your unique product/service/etc. to avoid conflicts with other commands/configs added by others.")

Maybe we should mention it somewhere where we document how to create custom executor (and likely should be the same on custom provider for config). Once we document it, it's really on those who will add new commands to worry about it (and yes I know you are on both sides of it :D ).

vincbeck · 2023-08-17T14:37:50Z

tests/cli/conftest.py

-custom_executor_module.CustomCeleryExecutor = type(  # type:  ignore
-    "CustomLocalExecutor", (celery_executor.CeleryExecutor,), {}
-)
-custom_executor_module.CustomCeleryKubernetesExecutor = type(  # type: ignore
-    "CustomLocalKubernetesExecutor", (celery_kubernetes_executor.CeleryKubernetesExecutor,), {}
+custom_executor_module.CustomLocalExecutor = type(  # type:  ignore
+    "CustomLocalExecutor", (local_executor.LocalExecutor,), {}
 )
-custom_executor_module.CustomCeleryExecutor = type(  # type:  ignore
-    "CustomKubernetesExecutor", (celery_executor.CeleryExecutor,), {}
+custom_executor_module.CustomLocalKubernetesExecutor = type(  # type: ignore
+    "CustomLocalKubernetesExecutor", (local_kubernetes_executor.LocalKubernetesExecutor,), {}
 )
-custom_executor_module.CustomCeleryKubernetesExecutor = type(  # type: ignore
-    "CustomCeleryKubernetesExecutor", (celery_kubernetes_executor.CeleryKubernetesExecutor,), {}
+custom_executor_module.CustomKubernetesExecutor = type(  # type:  ignore
+    "CustomKubernetesExecutor", (kubernetes_executor.KubernetesExecutor,), {}


potiuk · 2023-08-19T19:50:14Z

I am merging it, but I have a comment for the future (and also part of earlier discussion with @o-nikolas ). We do not currently have "How to create custom executor" docs. And it's (IMHO) one of the most important thing to complete AIP-51. We need to make sure in the future docs that we mention the "conventions" we have for CLI/Config uniqueness.

Just a note so that we do not forget about it.

potiuk · 2023-08-19T19:51:19Z

Marked it as 2.7.1 as well - the sooner we release it, the better.

o-nikolas · 2023-08-21T21:24:43Z

We do not currently have "How to create custom executor" docs. And it's (IMHO) one of the most important thing to complete AIP-51.

@potiuk Yupp, still on my radar. I've been pushing hard to get our first executor itself completed, but I'll try make some time for this task this week!

ephraimbuddy · 2023-08-28T12:01:39Z

We can't be able to cherrpick this one to 2.7.1 due to a dependence on #33279.

potiuk · 2023-08-28T12:09:14Z

Yeah. It's not a bugfix as well - can be easily added in 2.8

add a mechanism to warn if executors override existing CLI commands

d25f00a

boring-cyborg bot added the area:CLI label Aug 15, 2023

jedcunningham reviewed Aug 15, 2023

View reviewed changes

airflow/cli/cli_parser.py Show resolved Hide resolved

rewrite test to not interfer other tests on existing commands

4c747bb

uranusjr reviewed Aug 16, 2023

View reviewed changes

airflow/cli/cli_parser.py Outdated Show resolved Hide resolved

vandonr-amz added 4 commits August 16, 2023 14:26

throw an exception instead of logging a warning

112d48c

fix test that was passing only because of side effects from other tests

afdfcca

improve/remove tests that stopped making sense

d1180cd

mini fix

623a720

vandonr-amz commented Aug 16, 2023

View reviewed changes

vandonr-amz added 2 commits August 16, 2023 15:24

fix setup of celery tests

08d17fc

same for kubernetes command tests

923fcdc

vincbeck reviewed Aug 17, 2023

View reviewed changes

o-nikolas approved these changes Aug 17, 2023

View reviewed changes

add little warning about collisions in docstring

55a86d1

vandonr-amz requested review from kaxil, XD-DENG, ashb, pierrejeambrun and hussein-awala as code owners August 17, 2023 18:50

potiuk approved these changes Aug 17, 2023

View reviewed changes

potiuk merged commit 1945c1a into apache:main Aug 19, 2023
42 checks passed

potiuk added this to the Airflow 2.7.1 milestone Aug 19, 2023

vandonr-amz deleted the vandonr/fab2 branch August 19, 2023 23:50

ephraimbuddy added the type:bug-fix Changelog: Bug Fixes label Aug 27, 2023

ephraimbuddy modified the milestones: Airflow 2.7.1, Airflow 2.8.0 Aug 28, 2023

ephraimbuddy added type:improvement Changelog: Improvements and removed type:bug-fix Changelog: Bug Fixes labels Nov 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a mechanism to warn if executors override existing CLI commands #33423

Add a mechanism to warn if executors override existing CLI commands #33423

vandonr-amz commented Aug 15, 2023 •

edited

vandonr-amz Aug 16, 2023

vandonr-amz Aug 16, 2023

vincbeck Aug 17, 2023

vandonr-amz Aug 16, 2023

vandonr-amz Aug 16, 2023

vandonr-amz Aug 16, 2023

vincbeck Aug 17, 2023

vandonr-amz Aug 17, 2023

vandonr-amz Aug 17, 2023

vincbeck Aug 17, 2023

o-nikolas Aug 17, 2023

potiuk Aug 17, 2023

vincbeck Aug 17, 2023

potiuk commented Aug 19, 2023

potiuk commented Aug 19, 2023

o-nikolas commented Aug 21, 2023

ephraimbuddy commented Aug 28, 2023

potiuk commented Aug 28, 2023

		airflow_commands = core_commands
		airflow_commands = core_commands.copy() # make a copy to prevent bad interactions in tests

		parser.parse_args([expected_arg, "--help"])
		assert e.value.code == 0

		f"This can be due to the executor '{ExecutorLoader.get_default_executor_name()}' "
		f"redefining core airflow CLI commands."

Add a mechanism to warn if executors override existing CLI commands #33423

Add a mechanism to warn if executors override existing CLI commands #33423

Conversation

vandonr-amz commented Aug 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

potiuk commented Aug 19, 2023

potiuk commented Aug 19, 2023

o-nikolas commented Aug 21, 2023

ephraimbuddy commented Aug 28, 2023

potiuk commented Aug 28, 2023

vandonr-amz commented Aug 15, 2023 •

edited