-
Notifications
You must be signed in to change notification settings - Fork 906
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
Handle plugin registration failure ContextualVersionConflict with log instead of raising error #1542
Handle plugin registration failure ContextualVersionConflict with log instead of raising error #1542
Changes from 17 commits
72b84ca
176f89f
a19ee63
9d77ddd
96500ad
cd45c3d
9ce96e9
a0765e6
3c12bcc
139a075
c4d655c
0ce181a
3abfdc9
200405b
cd7ba3a
3d4aa55
4de3c11
91fb855
3209dbe
e20183c
d804e77
fc28f4a
b3dbbab
c61fd82
d7dff70
66a1b7d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -3,14 +3,15 @@ | |||||||
This module implements commands available from the kedro CLI. | ||||||||
""" | ||||||||
import importlib | ||||||||
import logging | ||||||||
import sys | ||||||||
import webbrowser | ||||||||
from collections import defaultdict | ||||||||
from pathlib import Path | ||||||||
from typing import Sequence | ||||||||
|
||||||||
import click | ||||||||
import pkg_resources | ||||||||
import importlib_metadata | ||||||||
|
||||||||
# pylint: disable=unused-import | ||||||||
import kedro.config.default_logger # noqa | ||||||||
|
@@ -41,6 +42,8 @@ | |||||||
v{version} | ||||||||
""" | ||||||||
|
||||||||
logger = logging.getLogger(__name__) | ||||||||
|
||||||||
|
||||||||
@click.group(context_settings=CONTEXT_SETTINGS, name="Kedro") | ||||||||
@click.version_option(version, "--version", "-V", help="Show version and exit") | ||||||||
|
@@ -65,10 +68,9 @@ def info(): | |||||||
plugin_versions = {} | ||||||||
plugin_entry_points = defaultdict(set) | ||||||||
for plugin_entry_point, group in ENTRY_POINT_GROUPS.items(): | ||||||||
for entry_point in pkg_resources.iter_entry_points(group=group): | ||||||||
for entry_point in importlib_metadata.entry_points().select(group=group): | ||||||||
module_name = entry_point.module_name.split(".")[0] | ||||||||
plugin_version = pkg_resources.get_distribution(module_name).version | ||||||||
plugin_versions[module_name] = plugin_version | ||||||||
plugin_versions[module_name] = entry_point.dist.version | ||||||||
plugin_entry_points[module_name].add(plugin_entry_point) | ||||||||
|
||||||||
click.echo() | ||||||||
|
@@ -98,12 +100,13 @@ def docs(): | |||||||
|
||||||||
def _init_plugins(): | ||||||||
group = ENTRY_POINT_GROUPS["init"] | ||||||||
for entry_point in pkg_resources.iter_entry_points(group=group): | ||||||||
for entry_point in importlib_metadata.entry_points().select(group=group): | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it make sense to refactor this function a bit so it uses There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The original test for
|
||||||||
try: | ||||||||
init_hook = entry_point.load() | ||||||||
init_hook() | ||||||||
except Exception as exc: | ||||||||
raise KedroCliError(f"Initializing {entry_point}") from exc | ||||||||
except Exception as exc: # pylint: disable=broad-except | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now you've made it clearer exactly what raises the exception here, I actually think we should remove the
Reasoning:
All this is kind of minor because realistically no one uses There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch, I think it is actually wrong to catch the error, error should be raised! |
||||||||
logger.warning(KedroCliError(f"Fail to initialize {entry_point}")) | ||||||||
logger.warning(exc) | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Unless there's as good reason to use the |
||||||||
|
||||||||
|
||||||||
class KedroCLI(CommandCollection): | ||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
"""Utilities for use with click.""" | ||
import difflib | ||
import logging | ||
import re | ||
import shlex | ||
import shutil | ||
|
@@ -16,7 +17,7 @@ | |
from typing import Any, Dict, Iterable, List, Mapping, Sequence, Set, Tuple, Union | ||
|
||
import click | ||
import pkg_resources | ||
import importlib_metadata | ||
|
||
CONTEXT_SETTINGS = dict(help_option_names=["-h", "--help"]) | ||
MAX_SUGGESTIONS = 3 | ||
|
@@ -33,6 +34,8 @@ | |
"cli_hooks": "kedro.cli_hooks", | ||
} | ||
|
||
logger = logging.getLogger(__name__) | ||
|
||
|
||
def call(cmd: List[str], **kwargs): # pragma: no cover | ||
"""Run a subprocess command and raise if it fails. | ||
|
@@ -328,13 +331,18 @@ def load_entry_points(name: str) -> Sequence[click.MultiCommand]: | |
List of entry point commands. | ||
|
||
""" | ||
entry_points = pkg_resources.iter_entry_points(group=ENTRY_POINT_GROUPS[name]) | ||
entry_points = importlib_metadata.entry_points() | ||
entry_points = entry_points.select(group=ENTRY_POINT_GROUPS[name]) | ||
|
||
entry_point_commands = [] | ||
for entry_point in entry_points: | ||
try: | ||
entry_point_commands.append(entry_point.load()) | ||
except Exception as exc: | ||
raise KedroCliError(f"Loading {name} commands from {entry_point}") from exc | ||
except Exception as exc: # pylint: disable=broad-except | ||
logger.warning( | ||
KedroCliError(f"Fail to load {name} commands from {entry_point}") | ||
) | ||
logger.warning(exc) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the most important part. It's hard to determine from the framework side whether a kedro's run is still valid if plugins fails. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar suggestion here to the other occurrence in cli.py. |
||
return entry_point_commands | ||
|
||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,8 @@ | ||
# PEP-518 https://peps.python.org/pep-0518/ | ||
[build-system] | ||
# Minimum requirements for the build system to execute. | ||
requires = ["setuptools>=38.0", "wheel"] # PEP 508 specifications. | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is something I just come across when I work on this PR, no related to the PR particularly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We've got If yes then that would be nice but a bit of a bigger change, so probably worth doing in a separate PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good point, created a new PR and will revert this soon. |
||
[tool.black] | ||
exclude = "/templates/|^features/steps/test_starter" | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -91,12 +91,12 @@ def test_print_version(self): | |||
assert result_abr.exit_code == 0 | ||||
assert version in result_abr.output | ||||
|
||||
def test_info_contains_plugin_versions(self, entry_point, mocker): | ||||
get_distribution = mocker.patch("pkg_resources.get_distribution") | ||||
get_distribution().version = "1.0.2" | ||||
def test_info_contains_plugin_versions(self, entry_point): | ||||
entry_point.dist.version = "1.0.2" | ||||
entry_point.module_name = "bob.fred" | ||||
|
||||
result = CliRunner().invoke(cli, ["info"]) | ||||
print(result.output) | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||
assert result.exit_code == 0 | ||||
assert ( | ||||
"bob: 1.0.2 (entry points:cli_hooks,global,hooks,init,line_magic,project)" | ||||
|
@@ -299,37 +299,44 @@ def test_project_groups(self, entry_points, entry_point): | |||
entry_point.load.return_value = "groups" | ||||
groups = load_entry_points("project") | ||||
assert groups == ["groups"] | ||||
entry_points.assert_called_once_with(group="kedro.project_commands") | ||||
entry_points.return_value.select.assert_called_once_with( | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the test, it's mainly just catching the log instead of catching the error. Due to the |
||||
group="kedro.project_commands" | ||||
) | ||||
|
||||
def test_project_error_is_caught(self, entry_points, entry_point): | ||||
def test_project_error_is_caught(self, entry_points, entry_point, caplog): | ||||
entry_point.load.side_effect = Exception() | ||||
with raises(KedroCliError, match="Loading project commands"): | ||||
load_entry_points("project") | ||||
|
||||
entry_points.assert_called_once_with(group="kedro.project_commands") | ||||
load_entry_points("project") | ||||
assert "Fail to load project commands" in caplog.text | ||||
entry_points.return_value.select.assert_called_once_with( | ||||
group="kedro.project_commands" | ||||
) | ||||
|
||||
def test_global_groups(self, entry_points, entry_point): | ||||
entry_point.load.return_value = "groups" | ||||
groups = load_entry_points("global") | ||||
assert groups == ["groups"] | ||||
entry_points.assert_called_once_with(group="kedro.global_commands") | ||||
entry_points.return_value.select.assert_called_once_with( | ||||
group="kedro.global_commands" | ||||
) | ||||
|
||||
def test_global_error_is_caught(self, entry_points, entry_point): | ||||
def test_global_error_is_caught(self, entry_points, entry_point, caplog): | ||||
entry_point.load.side_effect = Exception() | ||||
with raises(KedroCliError, match="Loading global commands from"): | ||||
load_entry_points("global") | ||||
entry_points.assert_called_once_with(group="kedro.global_commands") | ||||
load_entry_points("global") | ||||
assert "Fail to load global commands" in caplog.text | ||||
entry_points.return_value.select.assert_called_once_with( | ||||
group="kedro.global_commands" | ||||
) | ||||
|
||||
def test_init(self, entry_points, entry_point): | ||||
_init_plugins() | ||||
entry_points.assert_called_once_with(group="kedro.init") | ||||
entry_points.return_value.select.assert_called_once_with(group="kedro.init") | ||||
entry_point.load().assert_called_once_with() | ||||
|
||||
def test_init_error_is_caught(self, entry_points, entry_point): | ||||
def test_init_error_is_caught(self, entry_points, entry_point, caplog): | ||||
entry_point.load.side_effect = Exception() | ||||
with raises(KedroCliError, match="Initializing"): | ||||
_init_plugins() | ||||
entry_points.assert_called_once_with(group="kedro.init") | ||||
_init_plugins() | ||||
assert "Fail to initialize" in caplog.text | ||||
entry_points.return_value.select.assert_called_once_with(group="kedro.init") | ||||
|
||||
|
||||
class TestKedroCLI: | ||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this also raise some kind of exception? Or is it just the
entry_point.load
that we should wrap in thetry
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though I guess this is just
kedro info
rather than being triggered on every singlekedro
command like the other instances so it doesn't matter if there's uncaught exceptions anyway.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is indeed the solution I go with initially, but later I find out the API was not consistent. So technically only Python 3.10 can use the stdlib.
Originally I have this conditional import block in
kedro.utils
, otherwise I have to do this conditionally everywhere. Do you think it is the right place to do so? 3c12bccThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could fail, but like you said it should be just
kedro info
, unlike other plugin where the program still run without loading it. I think it is ok to leave it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Urghh, what a pain. I think what you did in 3c12bcc was ok, although probably it's more usual to just repeat the conditional import in multiple files.
BUT if the standard library API is only right in Python 3.10 then let's just forget about it and go for
importlib_metadata
all the way like you're doing now 👍There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have considered that, but it also means we have to copy the same block in multiple files, and also anywhere that use
mocker.patch
, which is quite hard to read.For the context, I started this PR with
importlib_metadata
, so I wasn't aware of the inconsistent API between the Python version. In theory, if I avoid using theselect
API, it could be compatible with python3.8-3.10. But for both case, we still need to have the conditional import, so I would rather just stick withimportlib_metadata
, unless this extra dependencies is causing trouble.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that totally makes sense. I'm definitely happy to go with
importlib_metadata
throughout. It's not worth the extra complexity doing it with standard library only for Python 3.10.