[GitHub] [airflow] vandonr-amz commented on a diff in pull request #33423: Add a mechanism to warn if executors override existing CLI commands
vandonr-amz commented on code in PR #33423:
URL: https://github.com/apache/airflow/pull/33423#discussion_r1297486491
##
airflow/cli/cli_parser.py:
##
@@ -59,13 +61,23 @@
"a 3.3.0+ version of the Celery provider. If using a Kubernetes
executor, install a "
"7.4.0+ version of the CNCF provider"
)
-# Do no re-raise the exception since we want the CLI to still function for
+# Do not re-raise the exception since we want the CLI to still function for
# other commands.
ALL_COMMANDS_DICT: dict[str, CLICommand] = {sp.name: sp for sp in
airflow_commands}
+# Check if sub-commands are defined twice, which could be an issue.
+if len(ALL_COMMANDS_DICT) < len(airflow_commands):
+dup = {k for k, v in collections.Counter([c.name for c in
airflow_commands]).items() if v > 1}
+raise CliConflictError(
+f"The following CLI {len(dup)} command(s) are defined more than once:
{sorted(dup)}\n"
+f"This can be due to the executor
'{ExecutorLoader.get_default_executor_name()}' "
+f"redefining core airflow CLI commands."
Review Comment:
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.
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [airflow] vandonr-amz commented on a diff in pull request #33423: Add a mechanism to warn if executors override existing CLI commands
vandonr-amz commented on code in PR #33423:
URL: https://github.com/apache/airflow/pull/33423#discussion_r1297482474
##
airflow/cli/cli_parser.py:
##
@@ -59,13 +61,23 @@
"a 3.3.0+ version of the Celery provider. If using a Kubernetes
executor, install a "
"7.4.0+ version of the CNCF provider"
)
-# Do no re-raise the exception since we want the CLI to still function for
+# Do not re-raise the exception since we want the CLI to still function for
# other commands.
ALL_COMMANDS_DICT: dict[str, CLICommand] = {sp.name: sp for sp in
airflow_commands}
+# Check if sub-commands are defined twice, which could be an issue.
+if len(ALL_COMMANDS_DICT) < len(airflow_commands):
+dup = {k for k, v in collections.Counter([c.name for c in
airflow_commands]).items() if v > 1}
+raise CliConflictError(
+f"The following CLI {len(dup)} command(s) are defined more than once:
{sorted(dup)}\n"
+f"This can be due to the executor
'{ExecutorLoader.get_default_executor_name()}' "
+f"redefining core airflow CLI commands."
Review Comment:
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 ?
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [airflow] vandonr-amz commented on a diff in pull request #33423: Add a mechanism to warn if executors override existing CLI commands
vandonr-amz commented on code in PR #33423:
URL: https://github.com/apache/airflow/pull/33423#discussion_r1296464825
##
tests/cli/conftest.py:
##
@@ -34,17 +36,14 @@
custom_executor_module.CustomCeleryKubernetesExecutor = type( # type: ignore
"CustomCeleryKubernetesExecutor",
(celery_kubernetes_executor.CeleryKubernetesExecutor,), {}
)
-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,), {}
Review Comment:
this whole thing was a mess of copy-paste mistakes. Tests were passing only
because commands were never removed.
##
tests/cli/test_cli_parser.py:
##
@@ -205,57 +230,38 @@ def test_positive_int(self):
cli_config.positive_int(allow_zero=True)("-1")
@pytest.mark.parametrize(
-"executor",
+"command",
[
"celery",
"kubernetes",
],
)
-def test_dag_parser_require_celery_executor(self, executor):
+def test_executor_specific_commands_not_accessible(self, command):
with conf_vars({("core", "executor"): "SequentialExecutor"}),
contextlib.redirect_stderr(
io.StringIO()
) as stderr:
reload(cli_parser)
parser = cli_parser.get_parser()
with pytest.raises(SystemExit):
-parser.parse_args([executor])
-stderr = stderr.getvalue()
-assert (f"airflow command error: argument GROUP_OR_COMMAND: invalid
choice: '{executor}'") in stderr
-
-@pytest.mark.parametrize(
-"executor",
-[
-"CeleryExecutor",
-"CeleryKubernetesExecutor",
-"custom_executor.CustomCeleryExecutor",
-"custom_executor.CustomCeleryKubernetesExecutor",
-],
-)
-def test_dag_parser_celery_command_accept_celery_executor(self, executor):
Review Comment:
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.
##
tests/cli/test_cli_parser.py:
##
@@ -266,20 +272,12 @@ def test_cli_parser_executors(self, executor,
expected_args):
) as stderr:
reload(cli_parser)
parser = cli_parser.get_parser()
-with pytest.raises(SystemExit) as e:
+with pytest.raises(SystemExit) as e: # running the help
command exits, so we prevent that
parser.parse_args([expected_arg, "--help"])
-assert e.value.code == 0
Review Comment:
this assert was never executed because it was in the block that captures the
exception
##
tests/cli/test_cli_parser.py:
##
@@ -266,20 +272,12 @@ def test_cli_parser_executors(self, executor,
expected_args):
) as stderr:
reload(cli_parser)
parser = cli_parser.get_parser()
-with pytest.raises(SystemExit) as e:
+with pytest.raises(SystemExit) as e: # running the help
command exits, so we prevent that
parser.parse_args([expected_arg, "--help"])
-assert e.value.code == 0
+assert e.value.code == 0, stderr.getvalue() # return code 0
== no problem
stderr = stderr.getvalue()
assert "airflow command error" not in stderr
-def test_dag_parser_config_command_dont_required_celery_executor(self):
Review Comment:
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.
##
airflow/cli/cli_parser.py:
##
@@ -41,11 +42,12 @@
GroupCommand,
core_commands,
)
+from airflow.cli.utils import CliConflictError
from airflow.exceptions
[GitHub] [airflow] vandonr-amz commented on a diff in pull request #33423: Add a mechanism to warn if executors override existing CLI commands
vandonr-amz commented on code in PR #33423:
URL: https://github.com/apache/airflow/pull/33423#discussion_r1296319339
##
airflow/cli/cli_parser.py:
##
@@ -65,6 +65,22 @@
ALL_COMMANDS_DICT: dict[str, CLICommand] = {sp.name: sp for sp in
airflow_commands}
+# Check if sub-commands are defined twice, which could be an issue.
+if len(ALL_COMMANDS_DICT) < len(airflow_commands):
+seen = set()
+dup = []
+for command in airflow_commands:
+if command.name not in seen:
+seen.add(command.name)
+else:
+dup.append(command.name)
+log.warning(
+"The following CLI %d command(s) are defined more than once, "
+"CLI behavior when using those will be unpredictable: %s",
+len(dup),
+dup,
+)
Review Comment:
actually it has to be `{k for k, v in collections.Counter([c.name for c in
airflow_commands]).items() if v > 1}`
I'll push it in a new commit, battling with tests at the moment.
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [airflow] vandonr-amz commented on a diff in pull request #33423: Add a mechanism to warn if executors override existing CLI commands
vandonr-amz commented on code in PR #33423:
URL: https://github.com/apache/airflow/pull/33423#discussion_r1295362936
##
airflow/cli/cli_parser.py:
##
@@ -65,6 +65,22 @@
ALL_COMMANDS_DICT: dict[str, CLICommand] = {sp.name: sp for sp in
airflow_commands}
+# Check if sub-commands are defined twice, which could be an issue.
+if len(ALL_COMMANDS_DICT) < len(airflow_commands):
+seen = set()
+dup = []
+for command in airflow_commands:
+if command.name not in seen:
+seen.add(command.name)
+else:
+dup.append(command.name)
+log.warning(
+"The following CLI %d command(s) are defined more than once, "
+"CLI behavior when using those will be unpredictable: %s",
+len(dup),
+dup,
+)
Review Comment:
it's more compact but I personally find it less readable.
(it's also less efficient but that does not matter 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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [airflow] vandonr-amz commented on a diff in pull request #33423: Add a mechanism to warn if executors override existing CLI commands
vandonr-amz commented on code in PR #33423: URL: https://github.com/apache/airflow/pull/33423#discussion_r1295252828 ## airflow/cli/cli_parser.py: ## Review Comment: > Yeah, that's what I meant, raise an exception. ok :p So, yeah, that'd be good if an executor introduces a command that conflicts with core, but if in core we introduce a new command that conflicts with an existing executor command, it'd be kind of bad :/ And since users can create their own executors, we'd never know beforehand. -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] vandonr-amz commented on a diff in pull request #33423: Add a mechanism to warn if executors override existing CLI commands
vandonr-amz commented on code in PR #33423: URL: https://github.com/apache/airflow/pull/33423#discussion_r1295251837 ## airflow/cli/cli_parser.py: ## Review Comment: there is `log.fatal` which is marked deprecated in favor of `log.critical`, the highest level of logging. I was thinking he was refering to that. https://docs.python.org/3/library/logging.html#logging-levels -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] vandonr-amz commented on a diff in pull request #33423: Add a mechanism to warn if executors override existing CLI commands
vandonr-amz commented on code in PR #33423: URL: https://github.com/apache/airflow/pull/33423#discussion_r1295240828 ## airflow/cli/cli_parser.py: ## Review Comment: I think Jed is not talking about an exception but more about the level of logging to use (i.e. use something _redder_) -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] vandonr-amz commented on a diff in pull request #33423: Add a mechanism to warn if executors override existing CLI commands
vandonr-amz commented on code in PR #33423: URL: https://github.com/apache/airflow/pull/33423#discussion_r1295222030 ## airflow/cli/cli_parser.py: ## Review Comment: Yeah, I wasn't sure... It's not preventing airflow from working, but it could be something needing attention indeed. -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
