feat(telemetry): Run telemetry outside of kedro projects #775

DimedS · 2024-07-22T08:37:50Z

Description

Closes #729,

Telemetry will be executed if a CLI command is run outside of a Kedro project (including the kedro new command). In this case, the same information will be sent, except for the project-related data.

Checklist

Opened this PR as a 'Draft Pull Request' if it is work-in-progress
Updated the documentation to reflect the code changes
Added a description of this change in the relevant RELEASE.md file
Added tests to cover my changes

Signed-off-by: Dmitry Sorokin <[email protected]>

…nds-outside-kedro-project-folder

noklam · 2024-07-23T10:17:49Z

kedro-telemetry/kedro_telemetry/plugin.py

@@ -168,26 +168,23 @@ def before_command_run(
    ):
        """Hook implementation to send command run data to Heap"""

-        if not project_metadata:  # in package mode


Does that mean we also track packaged mode now?

I believe that line isn't related to package mode:
#729 (comment)

As I understand it, in package mode, execution will occur without the CLI. Am I correct?

A packaged project will use the __main__.py as entrypoint. It will then look for the run method to execute.

I think you're right this line here in telemetry actually isn't related to package mode.. It seems to only be reached if you're not in a kedro project.

Packaged mode will execute the CLI through the entrypoint. I believe the commend about package mode because it was assumed that it is the only situation that project_metadata will be missing. This is not exactly true now because it would be either:

packaged mode

kedro command outside of project (i.e. kedro new)

I believe bot situations should be handled.

Thank you, @noklam. Could you please provide more details about packaged mode? From what I see in the code, if I package a Kedro project, it will have an entry point project_name.__main__:main. When I execute project_name in the terminal, the kedro/framework/cli/project.py:run() function will be executed directly, without calling the before_command_run() hook. This is because that hook is only called in the KedroCLI() class during Kedro CLI execution. I'm trying to understand what we need to handle in the telemetry hooks code concerning packaging mode.

noklam · 2024-07-23T10:20:32Z

kedro-telemetry/kedro_telemetry/plugin.py

        if not self._consent:
            self._opt_out_notification()
            return

        # get KedroCLI and its structure from actual project root
-        cli = KedroCLI(project_path=project_metadata.project_path)
+        cli = KedroCLI(project_path=project_path if project_path else Path.cwd())


I guess my question is, does that project_path here actually matters? Where is it used as the command_args are coming from hook and KedroCLI which was created in kedro framework.

I am also trying to understand why KedroCLI is dependent on a specific Kedro project. I believe the CLI structure should be the same for all projects. Do you think we can just use Path.cwd() here?

I don't think the project_path matters but we should add a comment to explain this. I have questions about the _recurse_cli used by the function, does it takes care of the LazyCommand we added lately?

I have questions about the _recurse_cli used by the function, does it takes care of the LazyCommand we added lately?

I checked the Lazy Loading PR, looks like _recurse_cli() func should work as previously. Also I tested current PR manually with latest kedro main branch on fresh virtual env, everything works well from the 1st command.

I dig into this a bit and I think project_path is not an arbitrary value here and it matters.

The output of recurse_cli change when I provide different value of project_path (mlflow etc is missing), they are not important here but I am not sure if this will trigger the imports.

When project_path is set as root correctly:

I am also concerned would this revert the effort of LazyCommand since this seems to try to load the CLI group (hopefully not the dependencies). How could we test this properly?

I think Nok is right, the recursion through the CLI definitely structure triggers the loading of lazy the CLI command groups -
It uses get_command() :

kedro-plugins/kedro-telemetry/kedro_telemetry/masking.py

Line 44 in 63a0c91

cli_element.get_command(ctx, command_name),

Which if you look at the implementation of the LazyGroup, triggers the lazy load:
https://github.com/kedro-org/kedro/blob/8bbbfb6c7257005bef841143690308f05d4b829b/kedro/framework/cli/utils.py#L504-L509

I believe we need to find a way to do the masking without loading relying on KedroCLI to not defeat the purpose of introducing lazy loading

kedro-telemetry/kedro_telemetry/plugin.py

merelcht · 2024-07-23T13:55:48Z

kedro-telemetry/kedro_telemetry/plugin.py

+    if project_path:
+        pyproject_path = project_path / PYPROJECT_CONFIG_NAME


Instead of overwriting the supplied project_path here I'd just create a new variable to make it easier to follow what happens and debug if something is wrong.

I believe it's already a new variable?

Signed-off-by: Dmitry Sorokin <[email protected]>

…nds-outside-kedro-project-folder

noklam

Approved with notes. #775 (comment), upon some investigation, I think there are issues to follow up:

project_path is not arbitary as it affects the result of recursive call
The recursive call undo the effort of LazyCommand as it eagerly import all the commands.

I will open a new issue and I suggest we fix both problem together (1. may not needed if we end up throwing away the recursive call)

DimedS · 2024-07-31T17:42:38Z

Thanks for your review, @ankatiyar and @noklam. I agree with you; recurse_cli() in telemetry is currently undermining the efforts made by Lazy Load, and we should prioritise fixing that. However, this issue is somewhat loosely connected to the current PR. Therefore, it's better to proceed with this PR before the release to fully complete the opt-out telemetry modernisation.

ElenaKhaustova

Thank you, @DimedS!

Agree that the issue found #794 should be addressed asap but it is outside of the scope of this PR.

…nds-outside-kedro-project-folder

* Run telemetry outside of kedro projects --------- Signed-off-by: Dmitry Sorokin <[email protected]> Co-authored-by: Nok Lam Chan <[email protected]> Signed-off-by: Merel Theisen <[email protected]>

Run telemetry outside of kedro projects

2f92844

Signed-off-by: Dmitry Sorokin <[email protected]>

DimedS linked an issue Jul 22, 2024 that may be closed by this pull request

kedro-telemetry: Spike how to enable telemetry for "kedro new" and other commands executed outside of kedro project folder #729

Closed

DimedS changed the title ~~Run telemetry outside of kedro projects~~ feat(telemetry): Run telemetry outside of kedro projects Jul 22, 2024

DimedS requested review from noklam, merelcht and ankatiyar July 22, 2024 08:42

Merge branch 'main' into 729-enable-telemetry-for-kedro-new-and-comma…

6fb1930

…nds-outside-kedro-project-folder

noklam reviewed Jul 23, 2024

View reviewed changes

merelcht reviewed Jul 23, 2024

View reviewed changes

Fix type hint

de3abe8

Signed-off-by: Dmitry Sorokin <[email protected]>

DimedS requested review from noklam, merelcht, lrcouto and ElenaKhaustova July 29, 2024 11:05

DimedS marked this pull request as ready for review July 31, 2024 15:42

Merge branch 'main' into 729-enable-telemetry-for-kedro-new-and-comma…

63a0c91

…nds-outside-kedro-project-folder

noklam approved these changes Jul 31, 2024

View reviewed changes

noklam mentioned this pull request Jul 31, 2024

kedro-telemetry: remove recursive logic to avoid eagerly load commands #794

Closed

ElenaKhaustova approved these changes Aug 1, 2024

View reviewed changes

Merge branch 'main' into 729-enable-telemetry-for-kedro-new-and-comma…

1b63f74

…nds-outside-kedro-project-folder

DimedS merged commit 05a794f into main Aug 1, 2024
10 checks passed

DimedS deleted the 729-enable-telemetry-for-kedro-new-and-commands-outside-kedro-project-folder branch August 1, 2024 09:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(telemetry): Run telemetry outside of kedro projects #775

feat(telemetry): Run telemetry outside of kedro projects #775

DimedS commented Jul 22, 2024

noklam Jul 23, 2024

DimedS Jul 23, 2024

merelcht Jul 23, 2024 •

edited

Loading

noklam Jul 30, 2024

DimedS Jul 30, 2024

noklam Jul 23, 2024

DimedS Jul 23, 2024

noklam Jul 30, 2024

DimedS Jul 31, 2024

noklam Jul 31, 2024 •

edited

Loading

ankatiyar Jul 31, 2024

merelcht Jul 23, 2024

DimedS Jul 29, 2024

noklam left a comment

DimedS commented Jul 31, 2024

ElenaKhaustova left a comment

		if project_path:
		pyproject_path = project_path / PYPROJECT_CONFIG_NAME

feat(telemetry): Run telemetry outside of kedro projects #775

feat(telemetry): Run telemetry outside of kedro projects #775

Conversation

DimedS commented Jul 22, 2024

Description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merelcht Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noklam Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noklam left a comment

Choose a reason for hiding this comment

DimedS commented Jul 31, 2024

ElenaKhaustova left a comment

Choose a reason for hiding this comment

merelcht Jul 23, 2024 •

edited

Loading

noklam Jul 31, 2024 •

edited

Loading