-
Notifications
You must be signed in to change notification settings - Fork 914
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
Lazy load click subcommands #3883
Merged
Merged
Changes from 19 commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
cbf054e
Lazy load subcommands
ankatiyar 12a62cd
Merge branch 'main' into lazy-subcommands
ankatiyar 2dec83a
Merge branch 'main' into lazy-subcommands
ankatiyar f79fde3
Test + lint
ankatiyar d53614a
Make overwriting of commands work
ankatiyar 05ada82
Some lint fixes
ankatiyar f38676c
Some lint fixes
ankatiyar 6dde38a
Some lint fixes
ankatiyar c6954e7
Some lint fixes
ankatiyar 63a4356
Fix some tests
ankatiyar 9af13df
Some tests + lint
ankatiyar 7933c61
fix tests
ankatiyar 409621e
Move lazy group
ankatiyar dfba836
lint
ankatiyar 4adb0b8
lint
ankatiyar 8a903bf
lint
ankatiyar 29a86bd
Merge branch 'main' into lazy-subcommands
ankatiyar 0fd6cb4
Remove commented parts, add comments
ankatiyar 37764a7
test lazy commands + docs link
ankatiyar d78a4c6
Release notes
ankatiyar 6e8afed
Merge branch 'main' into lazy-subcommands
ankatiyar 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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,14 +11,13 @@ | |
from kedro import KedroDeprecationWarning | ||
from kedro import __version__ as version | ||
from kedro.framework.cli import load_entry_points | ||
from kedro.framework.cli.catalog import catalog_cli | ||
from kedro.framework.cli.cli import KedroCLI, _init_plugins, cli | ||
from kedro.framework.cli.jupyter import jupyter_cli | ||
from kedro.framework.cli.micropkg import micropkg_cli | ||
from kedro.framework.cli.pipeline import pipeline_cli | ||
from kedro.framework.cli.project import project_group | ||
from kedro.framework.cli.registry import registry_cli | ||
from kedro.framework.cli.starters import create_cli | ||
from kedro.framework.cli.cli import ( | ||
KedroCLI, | ||
_init_plugins, | ||
cli, | ||
global_commands, | ||
project_commands, | ||
) | ||
from kedro.framework.cli.utils import ( | ||
CommandCollection, | ||
KedroCliError, | ||
|
@@ -332,15 +331,19 @@ def test_project_commands_no_clipy(self, mocker, fake_metadata): | |
side_effect=cycle([ModuleNotFoundError()]), | ||
) | ||
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, | ||
jupyter_cli, | ||
pipeline_cli, | ||
micropkg_cli, | ||
project_group, | ||
registry_cli, | ||
# There is only one `LazyGroup` for project commands | ||
assert len(kedro_cli.project_groups) == 1 | ||
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. It might be helpful to update the test name or leave a comment explaining that the 1 command group loaded is everything that's not doing lazy loading. |
||
assert kedro_cli.project_groups == [project_commands] | ||
# Assert that the lazy commands are listed properly | ||
assert kedro_cli.project_groups[0].list_commands(None) == [ | ||
"catalog", | ||
"ipython", | ||
"jupyter", | ||
"micropkg", | ||
"package", | ||
"pipeline", | ||
"registry", | ||
"run", | ||
] | ||
|
||
def test_project_commands_no_project(self, mocker, tmp_path): | ||
|
@@ -371,22 +374,20 @@ def test_project_commands_valid_clipy(self, mocker, fake_metadata): | |
return_value=Module(cli=cli), | ||
) | ||
kedro_cli = KedroCLI(fake_metadata.project_path) | ||
assert len(kedro_cli.project_groups) == 7 | ||
# The project group will now have two groups, the first from the project's cli.py and | ||
# the second is the lazy project command group | ||
assert len(kedro_cli.project_groups) == 2 | ||
assert kedro_cli.project_groups == [ | ||
catalog_cli, | ||
jupyter_cli, | ||
pipeline_cli, | ||
micropkg_cli, | ||
project_group, | ||
registry_cli, | ||
cli, | ||
project_commands, | ||
] | ||
|
||
def test_kedro_cli_no_project(self, mocker, tmp_path): | ||
mocker.patch("kedro.framework.cli.cli._is_project", return_value=False) | ||
kedro_cli = KedroCLI(tmp_path) | ||
assert len(kedro_cli.global_groups) == 2 | ||
assert kedro_cli.global_groups == [cli, create_cli] | ||
# The global groups will be the cli(group for info command) and the global commands (starter and new) | ||
assert kedro_cli.global_groups == [cli, global_commands] | ||
|
||
result = CliRunner().invoke(kedro_cli, []) | ||
|
||
|
@@ -410,28 +411,17 @@ def test_kedro_run_no_project(self, mocker, tmp_path): | |
) | ||
|
||
def test_kedro_cli_with_project(self, mocker, fake_metadata): | ||
Module = namedtuple("Module", ["cli"]) | ||
mocker.patch("kedro.framework.cli.cli._is_project", return_value=True) | ||
mocker.patch( | ||
"kedro.framework.cli.cli.bootstrap_project", return_value=fake_metadata | ||
) | ||
mocker.patch( | ||
"kedro.framework.cli.cli.importlib.import_module", | ||
return_value=Module(cli=cli), | ||
) | ||
kedro_cli = KedroCLI(fake_metadata.project_path) | ||
|
||
assert len(kedro_cli.global_groups) == 2 | ||
assert kedro_cli.global_groups == [cli, create_cli] | ||
assert len(kedro_cli.project_groups) == 7 | ||
assert kedro_cli.global_groups == [cli, global_commands] | ||
assert len(kedro_cli.project_groups) == 1 | ||
assert kedro_cli.project_groups == [ | ||
catalog_cli, | ||
jupyter_cli, | ||
pipeline_cli, | ||
micropkg_cli, | ||
project_group, | ||
registry_cli, | ||
cli, | ||
project_commands, | ||
] | ||
|
||
result = CliRunner().invoke(kedro_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
Oops, something went wrong.
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.
The secret scan test was complaining about the link ^ I added the code snippet instead. I hope that's okay? @noklam
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 had this problem locally as well but it's all good now and the link seems correct and working.