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.

Sign up for GitHub

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

Jump to bottom

fix(telemetry): Avoid eager loading the whole KedroCLI for masking #824

Merged
merged 6 commits into from
Sep 9, 2024
Merged

fix(telemetry): Avoid eager loading the whole KedroCLI for masking #824

merged 6 commits into from
Sep 9, 2024

Conversation

ankatiyar
Copy link
Contributor

@ankatiyar ankatiyar commented Sep 4, 2024 •
edited
Loading

Description

Resolve #794

Development notes

  • Instead of recursively loading all the commands from KedroCLI (which are lazy now) to generate a dict CLI structure, only load the structure for the command that is called to perform masking.
  • I've also updated the test to actually check masking with real Kedro commands since _mask_kedro_cli needs the actual KedroCLI object.
  • Removed the _recursive_items() function and associated unit tests since it wasn't being used anywhere.

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

ankatiyar and others added 5 commits September 4, 2024 16:59
@ankatiyar
First pass: only load command that was called
200baac 
Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>
@ankatiyar
Try to make it work with help and invalud commands
f9247d8 
Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>
@ankatiyar
Fix tests
15e2700 
Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>
@ankatiyar
Fix masking tests
f3af62b 
Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>
@ankatiyar
Merge branch 'main' into telemetry-masking-CLI
cc6adeb
@ankatiyar ankatiyar marked this pull request as ready for review September 5, 2024 16:52
@ankatiyar ankatiyar requested review from DimedS, noklam, limdauto and merelcht and removed request for limdauto September 5, 2024 16:52
@ankatiyar
Remove unused function and add argument name and type
e97a73a 
Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>
merelcht
merelcht reviewed Sep 6, 2024
View reviewed changes
Copy link
Member

@merelcht merelcht left a comment

Choose a reason for hiding this comment

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

The implementation looks good! 👍

Any way to test it end to end?

@ankatiyar
Copy link
Contributor Author

The implementation looks good! 👍

Any way to test it end to end?

Hmm, nothing straight-forward. You can print the result of _get_cli_structure() to see that the whole CLI dict is not loaded and the result of _mask_kedro_cli() if the masking is happening as expected.

@ElenaKhaustova
Copy link
Contributor

The implementation looks good! 👍
Any way to test it end to end?

Hmm, nothing straight-forward. You can print the result of _get_cli_structure() to see that the whole CLI dict is not loaded and the result of _mask_kedro_cli() if the masking is happening as expected.

Tested locally and compared outputs from _get_cli_structure() and _mask_kedro_cli() for the current branch and main branch - works as expected 🚀

ElenaKhaustova
ElenaKhaustova approved these changes Sep 6, 2024
View reviewed changes
Copy link
Contributor

@ElenaKhaustova ElenaKhaustova left a comment

Choose a reason for hiding this comment

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

LGTM!

DimedS
DimedS approved these changes Sep 9, 2024
View reviewed changes
Copy link
Contributor

@DimedS DimedS left a comment

Choose a reason for hiding this comment

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

Works well—tested with HeapAnalytics. Thanks, @ankatiyar, great job! I left one minor comment.

kedro-telemetry/kedro_telemetry/masking.py Show resolved Hide resolved
@ankatiyar ankatiyar merged commit b766f45 into main Sep 9, 2024
11 checks passed
@ankatiyar ankatiyar deleted the telemetry-masking-CLI branch September 9, 2024 12:41
harm-matthias-harms pushed a commit to harm-matthias-harms/kedro-plugins that referenced this pull request Oct 1, 2024
@ankatiyar @harm-matthias-harms
fix(telemetry): Avoid eager loading the whole KedroCLI for masking (k…
dc9fd3d 
…edro-org#824)

* First pass: only load command that was called

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>

* Try to make it work with help and invalud commands

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>

* Fix tests

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>

* Fix masking tests

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>

* Remove unused function and add argument name and type

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>

---------

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>
Signed-off-by: Harm Matthias Harms <matthias.harms@quis.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DimedS DimedS DimedS approved these changes

@ElenaKhaustova ElenaKhaustova ElenaKhaustova approved these changes

@merelcht merelcht Awaiting requested review from merelcht

@noklam noklam Awaiting requested review from noklam

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

kedro-telemetry: remove recursive logic to avoid eagerly load commands
5 participants
@ankatiyar @ElenaKhaustova @noklam @DimedS @merelcht