Make kedro-telemetry a core dependency
#3976
Conversation
Signed-off-by: Elena Khaustova <ymax70rus@gmail.com>
Signed-off-by: Elena Khaustova <ymax70rus@gmail.com>
Signed-off-by: Elena Khaustova <ymax70rus@gmail.com>
|
Unit tests fail if |
Signed-off-by: Elena Khaustova <ymax70rus@gmail.com>
Signed-off-by: Elena Khaustova <ymax70rus@gmail.com>
Signed-off-by: Elena Khaustova <ymax70rus@gmail.com>
|
Key question from me: does this create a circular dependency? kedro depending on kedro-telemetry and viceversa |
Yes, that's what we obviously missed 🙈 @DimedS shall we consider moving |
|
I would be happy to incorporate the kedro-telemetry code into Kedro, and I believe it should be done. However, I thought that at this stage, it would be a significant change, so I decided to start with a minor step. As a temporary solution, I don't see any major issues with this type of circular dependency; it essentially means that the code is part of one package. I don't anticipate any usage problems with this approach. Nevertheless, I would be happy to discuss whether we are ready to integrate the telemetry code into Kedro, as this is definitely the correct long-term solution. |
|
As far as I understand, pip indeed can deal with circular dependencies as long as they are both not build dependencies of each other - which is our case. But from the code organisation perspective, it feels wrong to me unless we have reasonable arguments for keeping telemetry in a separate package. Now (with recent changes of making telemetry opt-out), we expect the @DimedS, I agree with your points on long-term solution and am curious what others think. |
|
I also tested by creating circular dependencies in two libraries and uploading them to PyPI. Everything worked well, with no warnings during the upload to PyPI or while installing with pip. Based on this, I believe that for the next Kedro release, it would be better to integrate kedro-telemetry as a dependency of Kedro. For the subsequent major release, we can consider integrating the telemetry directly into Kedro, possibly in conjunction with splitting Kedro into different modules. |
kedro/framework/cli/cli.py
Outdated
| ) | ||
| else: | ||
| click.echo("No plugins installed") | ||
| # plugin_versions now always include kedro-telemetry as it was moved to core kedro dependency |
There was a problem hiding this comment.
I believe it is better to leave the code as it is now because it's more generic and users still can manually uninstall the kedro-telemetry which will produce "No plugins installed", and I believe in some time in future we will integrate kedro-telemetry inside of kedro and will need to change that back.
Fantastic, thanks 👍🏼
I 100 % agree. |
Signed-off-by: Elena Khaustova <ymax70rus@gmail.com>
Signed-off-by: Elena Khaustova <ymax70rus@gmail.com>
There was a problem hiding this comment.
Thank you for the PR, @ElenaKhaustova , tested it, everything works well!
pyproject.toml
Outdated
| @@ -22,6 +22,7 @@ dependencies = [ | |||
| "gitpython>=3.0", | |||
| "importlib-metadata>=3.6,<9.0", | |||
| "importlib_resources>=1.3,<7.0", # The `files()` API was introduced in `importlib_resources` 1.3 and Python 3.9. | |||
| "kedro-telemetry>=0.3.1", | |||
There was a problem hiding this comment.
Is 0.3.1 an acceptable version or should we just up this to 0.5.0 so we get the latest with all improvements on identifying users and projects?
There was a problem hiding this comment.
IMO makes sense to update, indeed. @DimedS, wdyt?
There was a problem hiding this comment.
Yes, I believe it makes sense to update it now, but it's more important to remember to update it with a new kedro-telemetry version after the next release of kedro-telemetry. This should be done after we complete all opt-out work, but before the Kedro release.
It's not a breaking change the only thing which is blocking it is some additional code restructuring that needs to be done before which @DimedS mentioned above #3976 (comment). Otherwise, we can proceed with moving. |
Signed-off-by: Elena Khaustova <ymax70rus@gmail.com>
There was a problem hiding this comment.
As I seen it in the discussion, it seems like circular dependencies wasn't a big problem, so I don't have problem with this approach.
I test it with uv to see what happens if I try to force an incompatible dependencies:
(kedro) gitpod /workspace/kedro (feature/telemetry-core) $ uv pip install -e . kedro-tele
metry==0.3.1
× No solution found when resolving dependencies:
╰─▶ Because only kedro==0.19.6 is available and kedro==0.19.6 depends on
kedro-telemetry>=0.5.0, we can conclude that all versions of kedro depend on
kedro-telemetry>=0.5.0.
And because you require kedro-telemetry==0.3.
- Do we have an issue to track the long term solution? Is it a breaking change or something that we can already implement in 0.19.x?
It's not a breaking change and we agreed to make it before the rest of the modularisation. Here is an issue created: #3999 |
Signed-off-by: Elena Khaustova <ymax70rus@gmail.com>
Description
Partly solves kedro-org/kedro-plugins#726
Development notes
kedro-telemetrydependency from therequirements.txtfile inKedroproject template.pyproject.tomlfile to includekedro-telemetryin the dependencies section.Developer Certificate of Origin
We need all contributions to comply with the Developer Certificate of Origin (DCO). All commits must be signed off by including a
Signed-off-byline in the commit message. See our wiki for guidance.If your PR is blocked due to unsigned commits, then you must follow the instructions under "Rebase the branch" on the GitHub Checks page for your PR. This will retroactively add the sign-off to all unsigned commits and allow the DCO check to pass.
Checklist
RELEASE.mdfile