-
Notifications
You must be signed in to change notification settings - Fork 915
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
noklam
merged 26 commits into
main
from
fix/1487-contextualversionconflict-error-stop-kedro-running-when-dependency-clashes
May 20, 2022
Merged
Handle plugin registration failure ContextualVersionConflict with log instead of raising error #1542
Changes from all commits
Commits
Show all changes
26 commits
Select commit
Hold shift + click to select a range
72b84ca
ignore loading error
noklam 176f89f
Move setuptools to pyproject.toml to align with PEP-518
noklam a19ee63
revert changes
noklam 9d77ddd
[WIP] - replace pkg_resource with importlib_metadata
noklam 96500ad
[WIP] - replace pkg_resource
noklam cd45c3d
Replace entrypoint discovery with importlib
noklam 9ce96e9
Tidy up logging message, linting
noklam a0765e6
small changes to tidy up the variables
noklam 3c12bcc
Add conditional import to fix importlib not available in python 3.7
noklam 139a075
Fix test cases
noklam c4d655c
Update error message
noklam 0ce181a
Switch back to importlib_metadata and fix tests
noklam 3abfdc9
add importlib as requirements
noklam 200405b
Fix test with py37
noklam cd7ba3a
Merge branch 'main' into fix/1487-contextualversionconflict-error-sto…
noklam 3d4aa55
Fix linting
noklam 4de3c11
Merge remote-tracking branch 'origin/fix/1487-contextualversionconfli…
noklam 91fb855
Apply changes from review
noklam 3209dbe
Refactor and fix wrong test
noklam e20183c
Fix some tests... should work now
noklam d804e77
I forgot to put the setuptools back...
noklam fc28f4a
I have gone too far and removed a necessary dependency
noklam b3dbbab
Merge branch 'main' into fix/1487-contextualversionconflict-error-sto…
noklam c61fd82
apply comments after reviews
noklam d7dff70
Merge remote-tracking branch 'origin/fix/1487-contextualversionconfli…
noklam 66a1b7d
linting
noklam File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -104,9 +104,8 @@ 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"]) | ||
|
@@ -312,37 +311,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 "Failed 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 "Failed 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): | ||
entry_point.load.side_effect = Exception() | ||
with raises(KedroCliError, match="Initializing"): | ||
entry_point.load.return_value.side_effect = Exception() | ||
with raises(Exception): | ||
_init_plugins() | ||
entry_points.assert_called_once_with(group="kedro.init") | ||
entry_points.return_value.select.assert_called_once_with(group="kedro.init") | ||
|
||
|
||
class TestKedroCLI: | ||
|
@@ -356,6 +362,7 @@ def test_project_commands_no_clipy(self, mocker, fake_metadata): | |
"kedro.framework.cli.cli.bootstrap_project", return_value=fake_metadata | ||
) | ||
kedro_cli = KedroCLI(fake_metadata.project_path) | ||
print(kedro_cli.project_groups) | ||
assert len(kedro_cli.project_groups) == 6 | ||
assert kedro_cli.project_groups == [ | ||
catalog_cli, | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.