-
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
kedro-telemetry
: Improve performance by switching to after_command_run
#4014
Changes from all commits
1e67775
9fca028
29b627f
8e9e5d4
22977d5
3142ef8
6ee73b8
a97a012
45013cf
bbab82c
66136bd
f5fe4cc
67dd0ff
184c35d
7b592fe
c5c22eb
155a30d
de64ef7
f6f657b
b66c0fd
43054c5
a31f0d4
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 |
---|---|---|
|
@@ -5,6 +5,7 @@ | |
from __future__ import annotations | ||
|
||
import importlib | ||
import logging | ||
import sys | ||
import traceback | ||
from collections import defaultdict | ||
|
@@ -38,6 +39,9 @@ | |
v{version} | ||
""" | ||
|
||
logger = logging.getLogger(__name__) | ||
logger.addHandler(logging.StreamHandler(sys.stderr)) | ||
|
||
|
||
@click.group(context_settings=CONTEXT_SETTINGS, name="Kedro") | ||
@click.version_option(version, "--version", "-V", help="Show version and exit") | ||
|
@@ -154,6 +158,9 @@ def main( | |
self._cli_hook_manager.hook.before_command_run( | ||
project_metadata=self._metadata, command_args=args | ||
) | ||
|
||
hook_called = False | ||
|
||
try: | ||
super().main( | ||
args=args, | ||
|
@@ -169,6 +176,8 @@ def main( | |
self._cli_hook_manager.hook.after_command_run( | ||
project_metadata=self._metadata, command_args=args, exit_code=exc.code | ||
) | ||
hook_called = True | ||
|
||
# When CLI is run outside of a project, project_groups are not registered | ||
catch_exception = "click.exceptions.UsageError: No such command" | ||
# click convert exception handles to error message | ||
|
@@ -198,7 +207,19 @@ def main( | |
) | ||
click.echo(message) | ||
click.echo(hint) | ||
sys.exit(exc.code) | ||
sys.exit(exc.code) | ||
except Exception as error: | ||
logger.error(f"An error has occurred: {error}") | ||
self._cli_hook_manager.hook.after_command_run( | ||
project_metadata=self._metadata, command_args=args, exit_code=1 | ||
) | ||
ankatiyar marked this conversation as resolved.
Show resolved
Hide resolved
|
||
hook_called = True | ||
sys.exit(1) | ||
finally: | ||
if not hook_called: | ||
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. Thank you for the PR, @lrcouto! Could you please explain how this is possible? It looks like 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. I guess that's in case of no exception 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. Yes, this finally block is there to solve that issue where the hook was called twice when there was not an exception. |
||
self._cli_hook_manager.hook.after_command_run( | ||
project_metadata=self._metadata, command_args=args, exit_code=0 | ||
) | ||
|
||
@property | ||
def global_groups(self) -> Sequence[click.MultiCommand]: | ||
|
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.
With that setting, will every logging message be sent to
stderr
? If theINFO
logging level is configured in Kedro, willINFO
messages also go tostderr
, or are they not connected to Kedro's logging settings?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.
It should affect only the messages generated from this instance of the logger, which is just the one error message on the main function.