Skip to content
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

Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
72b84ca
ignore loading error
noklam May 16, 2022
176f89f
Move setuptools to pyproject.toml to align with PEP-518
noklam May 17, 2022
a19ee63
revert changes
noklam May 17, 2022
9d77ddd
[WIP] - replace pkg_resource with importlib_metadata
noklam May 17, 2022
96500ad
[WIP] - replace pkg_resource
noklam May 17, 2022
cd45c3d
Replace entrypoint discovery with importlib
noklam May 18, 2022
9ce96e9
Tidy up logging message, linting
noklam May 18, 2022
a0765e6
small changes to tidy up the variables
noklam May 18, 2022
3c12bcc
Add conditional import to fix importlib not available in python 3.7
noklam May 18, 2022
139a075
Fix test cases
noklam May 18, 2022
c4d655c
Update error message
noklam May 18, 2022
0ce181a
Switch back to importlib_metadata and fix tests
noklam May 19, 2022
3abfdc9
add importlib as requirements
noklam May 19, 2022
200405b
Fix test with py37
noklam May 19, 2022
cd7ba3a
Merge branch 'main' into fix/1487-contextualversionconflict-error-sto…
noklam May 19, 2022
3d4aa55
Fix linting
noklam May 19, 2022
4de3c11
Merge remote-tracking branch 'origin/fix/1487-contextualversionconfli…
noklam May 19, 2022
91fb855
Apply changes from review
noklam May 20, 2022
3209dbe
Refactor and fix wrong test
noklam May 20, 2022
e20183c
Fix some tests... should work now
noklam May 20, 2022
d804e77
I forgot to put the setuptools back...
noklam May 20, 2022
fc28f4a
I have gone too far and removed a necessary dependency
noklam May 20, 2022
b3dbbab
Merge branch 'main' into fix/1487-contextualversionconflict-error-sto…
noklam May 20, 2022
c61fd82
apply comments after reviews
noklam May 20, 2022
d7dff70
Merge remote-tracking branch 'origin/fix/1487-contextualversionconfli…
noklam May 20, 2022
66a1b7d
linting
noklam May 20, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 10 additions & 7 deletions kedro/framework/cli/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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")
Expand All @@ -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):
Copy link
Contributor

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 the try?

Copy link
Contributor

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 single kedro command like the other instances so it doesn't matter if there's uncaught exceptions anyway.

Copy link
Contributor Author

@noklam noklam May 19, 2022

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? 3c12bcc

image

Copy link
Contributor Author

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.

Copy link
Contributor

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 👍

Copy link
Contributor Author

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 the select 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 with importlib_metadata, unless this extra dependencies is causing trouble.

Copy link
Contributor

@antonymilne antonymilne May 19, 2022

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.

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()
Expand Down Expand Up @@ -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):
Copy link
Contributor

Choose a reason for hiding this comment

The 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 load_entry_points in utils.py? At a glance they look very similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original test for _init_plugins is actually incorrectly testing the load exception, it should be testing the error during hook initialisation instead.

init_hook = entry_point.load() # previously the the error was triggered here
init_hook()  # this is what we want to test instead

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
Copy link
Contributor

Choose a reason for hiding this comment

The 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 try/except altogether and just do a plain

for init_hook in init_hooks:
    init_hooks()

Reasoning:

  • exceptions raised by entry_point.load() are now handled by load_entry_points
  • exceptions raised by init_hook() should still be raised, not hidden and logged, and there's not much point wrapping them in KedroCliError like we used to. This is analogous to what happens e.g. if a plugin with a hook raises an exception - it doesn't get caught and logged or converted into a kedro error message

All this is kind of minor because realistically no one uses init_hooks and it's probably redundant now we have before_command_run anyway... But if it's not too annoying to chance I think this would be a small improvement.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
logger.warning(KedroCliError(f"Fail to initialize {entry_point}"))
logger.warning(exc)
logger.warning(f"Failed to initialise %s. Full exception: %s", entry_point, exc)

Unless there's as good reason to use the KedroCliError still?



class KedroCLI(CommandCollection):
Expand Down
16 changes: 12 additions & 4 deletions kedro/framework/cli/utils.py
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
Expand All @@ -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
Expand All @@ -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.
Expand Down Expand Up @@ -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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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. kedro-viz is defintely ok to just ignore it, but for kedro-mlflow type of plugin that actually modify the run behavior, it may be problematic so I am using warning level.

Copy link
Contributor

Choose a reason for hiding this comment

The 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


Expand Down
1 change: 0 additions & 1 deletion kedro/framework/session/session.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
# pylint: disable=invalid-name,global-statement
"""This module implements Kedro session responsible for project lifecycle."""
import getpass
import logging
Expand Down
5 changes: 5 additions & 0 deletions pyproject.toml
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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've got "setuptools>=38.0" and wheel requirements in various other places in the repo (just do a Ctrl+F to see them). Does this mean we can remove them from there?

If yes then that would be nice but a bit of a bigger change, so probably worth doing in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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"

Expand Down
2 changes: 1 addition & 1 deletion requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,12 @@ cookiecutter~=1.7.0
dynaconf>=3.1.2,<4.0.0
fsspec>=2021.4, <=2022.1
gitpython~=3.0
importlib_metadata >=4.4 # Compatible with Python 3.10 importlib.metadata
noklam marked this conversation as resolved.
Show resolved Hide resolved
jmespath>=0.9.5, <1.0
pip-tools~=6.5
pluggy~=1.0.0
python-json-logger~=2.0
PyYAML>=4.2, <7.0
rope~=0.21.0 # subject to LGPLv3 license
setuptools>=38.0
toml~=0.10
toposort~=1.5 # Needs to be at least 1.5 to be able to raise CircularDependencyError
5 changes: 3 additions & 2 deletions tests/framework/cli/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,14 @@

@fixture
def entry_points(mocker):
return mocker.patch("pkg_resources.iter_entry_points")
return mocker.patch("importlib_metadata.entry_points")


@fixture
def entry_point(mocker, entry_points):
ep = mocker.MagicMock()
entry_points.return_value = [ep]
entry_points.return_value = ep
entry_points.return_value.select.return_value = [ep]
return ep


Expand Down
45 changes: 26 additions & 19 deletions tests/framework/cli/test_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
print(result.output)

assert result.exit_code == 0
assert (
"bob: 1.0.2 (entry points:cli_hooks,global,hooks,init,line_magic,project)"
Expand Down Expand Up @@ -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(
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 entry_points API, I have to mock 2 layers which makes the assertion a bit longer.

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:
Expand Down
1 change: 1 addition & 0 deletions tests/framework/cli/test_cli_hooks.py
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ def test_kedro_cli_should_invoke_cli_hooks_from_plugin(
mocker,
fake_metadata,
fake_plugin_distribution,
entry_points, # pylint: disable=unused-argument
):
# Workaround to ensure that the log messages are picked up by caplog.
# https://github.com/pytest-dev/pytest/issues/3697
Expand Down