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

[READY] Add new notebook example to docs about adding Kedro to a notebook project #3128

Merged
merged 12 commits into from
Oct 11, 2023

Conversation

stichbury
Copy link
Contributor

@stichbury stichbury commented Oct 5, 2023

NOTE: Kedro datasets are moving from kedro.extras.datasets to a separate kedro-datasets package in
kedro-plugins repository. Any changes to the dataset implementations
should be done by opening a pull request in that repository.

Description

Answers #2855

Development notes

Introduces a new example with a markdown that mirrors a full runnable notebook in a downloadable directory.
Incrementally adds Kedro features to a notebook project for spaceflights.
Take a look at the built docs or review the markdown. PLEASE DO NOT EDIT THE NOTEBOOK!

Checklist

  • Read the contributing guidelines
  • Signed off each commit with a Developer Certificate of Origin (DCO)
  • 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
  • Checked if this change will affect Kedro-Viz, and if so, communicated that with the Viz team

Signed-off-by: Jo Stichbury <jo_stichbury@mckinsey.com>
@stichbury stichbury self-assigned this Oct 5, 2023
@stichbury stichbury added the Component: Documentation 📄 Issue/PR for markdown and API documentation label Oct 5, 2023
Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

I really like how this is shaping up, great job @stichbury !

Main comments:

  • I really like the flow. One outstanding point is how do we make the user download the code. I propose we tell them to clone a repository, or even open it on Gitpod. Could be kedro-academy, could be this one. I'm -0.5 on adding the docs/examples, but not completely opposed to it if we think it's the right thing to do.
  • I opened a PR with some proposed code changes Add new notebook tweaks #3133 including using the config loader with the current directory, so the user doesn't have to create conf/{base,local}. We can end there, or we can actually create the dirs, but in a second step IMHO.
  • Some parts at the end of the tutorial repeat a lot of code. I'm thinking whether we could assume that the user has the context and we can make the final code blocks more terse (rather than fully self-contained). There's potential of creating some confusion, but at the moment this all feels like too much text.

astrojuanlu and others added 4 commits October 6, 2023 17:10
* Minor code changes to notebook

Signed-off-by: Juan Luis Cano Rodríguez <juan_luis_cano@mckinsey.com>

* Partially apply black to code samples

Signed-off-by: Juan Luis Cano Rodríguez <juan_luis_cano@mckinsey.com>

---------

Signed-off-by: Juan Luis Cano Rodríguez <juan_luis_cano@mckinsey.com>
Signed-off-by: Jo Stichbury <jo_stichbury@mckinsey.com>
Signed-off-by: Jo Stichbury <jo_stichbury@mckinsey.com>
Signed-off-by: Jo Stichbury <jo_stichbury@mckinsey.com>
@stichbury stichbury changed the title [Draft] Add new examples folder to docs and page about a notebook example [Draft] Add new notebook example to docs about adding Kedro to a notebook project Oct 9, 2023
Signed-off-by: Jo Stichbury <jo_stichbury@mckinsey.com>
@stichbury stichbury changed the title [Draft] Add new notebook example to docs about adding Kedro to a notebook project [READY] Add new notebook example to docs about adding Kedro to a notebook project Oct 9, 2023
@stichbury stichbury marked this pull request as ready for review October 9, 2023 13:57
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.

This is really amazing work @stichbury 🤩 I left two minor comments, but other than that all good from my side.

Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

A bit long, but absolutely wonderful nonetheless 💯 Well done @stichbury !

Two final questions:

  1. do we need both the .ipynb and the .md? I reckon having these makes it easier for distracted users that don't install jupytext to just open the .ipynb and go on with their lives. The cost is having the content twice and having to sync it ourselves. Not a big deal, just wanted to make it explicit.
  2. do you plan to keep the example under docs/source/notebook-example, or is this an intermediate step towards having it on https://github.com/kedro-org/kedro-academy/ ? I think this is excellent tutorial material (welp, I've been delivering similar tutorials all year!) and would be a shame to not have it there. but if there's a rationale to keep it here, then fine.

Apart from these two, LGTM 👍🏽

@stichbury
Copy link
Contributor Author

A bit long, but absolutely wonderful nonetheless 💯 Well done @stichbury !
Thanks!

Two final questions:

  1. do we need both the .ipynb and the .md? I reckon having these makes it easier for distracted users that don't install jupytext to just open the .ipynb and go on with their lives. The cost is having the content twice and having to sync it ourselves. Not a big deal, just wanted to make it explicit.

Yes, I wrestled over this but I don't want to force people to use jupytext since this is onboarding type content. The goal is to have a notebook ready to go, but it does come at the expense of us having to understand how to work with the two files. I did write a README for this but removed it. Maybe I'll write a comment in the markdown file to explain how to make updates before I merge this.

  1. do you plan to keep the example under docs/source/notebook-example, or is this an intermediate step towards having it on https://github.com/kedro-org/kedro-academy/ ? I think this is excellent tutorial material (welp, I've been delivering similar tutorials all year!) and would be a shame to not have it there. but if there's a rationale to keep it here, then fine.

I want to keep this in the docs repo. Reason being that otherwise part 1. becomes difficult. We have to copy the markdown across repos, or the notebook, and that just gets annoying. At least with both in the same directory the code and example are side by side, and to make that happen and be able to build the docs, it has to be here. Also, I looked at the academy repo and it's not really ideal, it's a bit of a wilderness for a newbie Kedro user. I think we should probably have an examples folder in the docs and I tried it for this, but as per my comment above about putting both files together, it wasn't ideal in this instance, so I moved them to sit under the notebooks section of the docs.

Thanks for asking: making this explicit is a good decision record.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Documentation 📄 Issue/PR for markdown and API documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants