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.

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

Deprecate kedro.extras.datasets and add top-level docs for kedro_datasets #2546

Merged
merged 6 commits into from
Apr 28, 2023

Conversation

astrojuanlu
Copy link
Member

@astrojuanlu astrojuanlu commented Apr 27, 2023

Description

Fix gh-1501.

  • Add admonition to kedro/extras/datasets/README.md, visible on GitHub (link):

image

  • Add admonotion to kedro/extras/datasets/__init__.py, visible in the docs:

Screenshot 2023-04-27 at 19 41 51

  • Emit a DeprecationWarning whenever kedro.extras.datasets is imported:

Screenshot 2023-04-27 at 18 55 15

  • Hide kedro.extras.datasets from the kedro.extras toctree (not from the navigation bar because that's not possible - an alternative is to declare it as :orphan:)

Screenshot 2023-04-27 at 19 43 26

  • And... 🥁 added top-level documentation for kedro_datasets + replace misleading kedro.datasets references that didn't exist + simplify the documentation builds by removing the special script!

Screenshot 2023-04-27 at 19 44 28

Development notes

The special handling of kedro_datasets was introduced in gh-2006.

Checklist

  • Read the contributing guidelines
  • 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 RELEASE.md file
  • Added tests to cover my changes

Signed-off-by: Juan Luis Cano Rodríguez <[email protected]>
Signed-off-by: Juan Luis Cano Rodríguez <[email protected]>
Signed-off-by: Juan Luis Cano Rodríguez <[email protected]>
Signed-off-by: Juan Luis Cano Rodríguez <[email protected]>
@astrojuanlu astrojuanlu force-pushed the deprecate-kedro-extras-datasets branch from 73921f8 to 698164e Compare April 27, 2023 17:47
@astrojuanlu astrojuanlu marked this pull request as ready for review April 27, 2023 18:06
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.

How do the kedro-datasets docs get build without the script? They're not part of the repo so how can sphinx build them?

Comment on lines +6 to +14
.. automodule:: kedro_datasets

.. rubric:: Classes

.. autosummary::
:toctree:
:template: autosummary/class.rst

kedro_datasets.api.APIDataSet
Copy link
Member Author

Choose a reason for hiding this comment

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

@merelcht here's the magic. autodoc and sphinx.ext.autosummary only need to be able to import the objects, they don't even have to be downloaded physically!

Copy link
Member

Choose a reason for hiding this comment

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

So you need to have kedro-datasets installed? And what if you have an older version, would it then build old docs?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's correct, it will load whatever version is installed. Since the docs extras-requires is getting installed, kedro-datasets and all its dependencies were already there:

kedro/Makefile

Lines 36 to 37 in a95ce7a

build-docs:
pip install -e ".[docs]"

kedro/setup.py

Line 109 in a95ce7a

"kedro-datasets[api,biosequence,dask,geopandas,matplotlib,holoviews,networkx,pandas,pillow,polars,video,plotly,redis,spark,svmlight,yaml]==1.1.1",

Copy link
Member

Choose a reason for hiding this comment

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

Alright, so I guess in that case we need to make sure to bump the pin here whenever we do a kedro-datasets release to make sure docs are the most recent.

Copy link
Member

Choose a reason for hiding this comment

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

I've added checking to bump the kedro-datasets doc requirements as a step in the release notes.

Copy link
Contributor

@stichbury stichbury left a comment

Choose a reason for hiding this comment

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

Looks great to me! Thank you 🌟

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.

Let's add this to the release notes as well so we remember when we added the deprecation warnings.

Signed-off-by: Merel Theisen <[email protected]>
@merelcht merelcht enabled auto-merge (squash) April 28, 2023 10:15
@astrojuanlu
Copy link
Member Author

I see you went ahead and added it, thanks @merelcht 🙏🏽

@merelcht merelcht merged commit a7d0e7d into main Apr 28, 2023
@merelcht merelcht deleted the deprecate-kedro-extras-datasets branch April 28, 2023 11:08
astrojuanlu added a commit that referenced this pull request May 5, 2023
* Deprecate `kedro.extras.datasets` and add top-level docs for `kedro_datasets` (#2546)

Signed-off-by: Juan Luis Cano Rodríguez <[email protected]>

* Delete reference to removed kedro.extras subpackages

Signed-off-by: Juan Luis Cano Rodríguez <[email protected]>

---------

Signed-off-by: Juan Luis Cano Rodríguez <[email protected]>
Co-authored-by: Juan Luis Cano Rodríguez <[email protected]>
@zedrdave
Copy link

Sorry if I missed something, but shouldn't _DEFAULT_PACKAGES also be modified from:
_DEFAULT_PACKAGES = ["kedro.io.", "kedro_datasets.", "kedro.extras.datasets.", ""]

to:
_DEFAULT_PACKAGES = ["kedro.io.", "kedro_datasets.", "", "kedro.extras.datasets."]

Currently, the DeprecationWarning will be fired whenever using a custom class string.

@astrojuanlu
Copy link
Member Author

Hey @zedrdave , possibly - could you open a new issue so it has more visibility?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add deprecation warnings to all the kedro.extras.datasets to explain they're moving
4 participants