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

Improve DataSet API documentation with YAML examples #1844

Merged
merged 8 commits into from
Nov 10, 2022
Merged

Improve DataSet API documentation with YAML examples #1844

merged 8 commits into from
Nov 10, 2022

Conversation

levimjoseph
Copy link
Contributor

@levimjoseph levimjoseph commented Sep 8, 2022

Description

Adding YAML documentation
In response to: #1762

Development notes

I changed the following files:

  • kedro/extras/datasets/api
  • kedro/extras/datasets/dask
  • kedro/extras/datasets/pandas
  • kedro/extras/datasets/tensorflow
  • kedro/extras/datasets/tracking
  • kedro/extras/datasets/tracking
  • kedro/extras/datasets/yaml

I ran the test suite.

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

@levimjoseph
Copy link
Contributor Author

@noklam this is my draft pr. First PR, so any advice/input is welcome

@noklam noklam changed the title testing new env Improve DataSet API documentation with YAML examples Sep 9, 2022
@noklam noklam added the Community Issue/PR opened by the open-source community label Sep 9, 2022
Copy link
Contributor

@noklam noklam left a comment

Choose a reason for hiding this comment

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

Thanks @levimjoseph for the PR and sorry for the really late review.

Just to check if you have run some of the code to check if the YAML API works?

I notice there are some inconsistencies (some of them are inherit from our docs). In general, the Python API and YAML API are the describing the same dataset, which I like because it demonstrate how to map these arguments clearly.

In some cases the original example are too simple, so I think it's fine even if the new YAML API are not exactly the same.

There are way too many s3, gcs paths that are too complicated as an exampe, but we will address this later. For API docs I think it's nice to have at least 1 example that is easy to copy & paste without too many setups.

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.

Thanks for taking this on @levimjoseph 👍 I've left some comments.

kedro/extras/datasets/api/api_dataset.py Outdated Show resolved Hide resolved
kedro/extras/datasets/dask/parquet_dataset.py Outdated Show resolved Hide resolved
kedro/extras/datasets/tracking/json_dataset.py Outdated Show resolved Hide resolved
kedro/extras/datasets/tracking/metrics_dataset.py Outdated Show resolved Hide resolved
@merelcht
Copy link
Member

Hi @levimjoseph , do you need some help finishing this PR?

@merelcht merelcht mentioned this pull request Nov 7, 2022
10 tasks
@merelcht
Copy link
Member

merelcht commented Nov 7, 2022

Hi @levimjoseph do you still want to complete this PR, or should someone from the team take-over? We'd like to get all PRs related to datasets to be merged soon now we're moving our datasets code to a different package (see our medium blog post for more details)

merelcht and others added 3 commits November 9, 2022 11:51
Co-authored-by: Nok Lam Chan <nok_lam_chan@mckinsey.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
merelcht and others added 3 commits November 9, 2022 11:57
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
@merelcht merelcht requested review from noklam and removed request for AhdraMeraliQB, idanov and jmholzer November 9, 2022 13:58
Copy link
Contributor

@noklam noklam left a comment

Choose a reason for hiding this comment

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

Looks good ⭐️ Thank you @levimjoseph and @merelcht

@merelcht merelcht self-assigned this Nov 9, 2022
@merelcht merelcht merged commit bc6f3fe into kedro-org:main Nov 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community Issue/PR opened by the open-source community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants