-
Notifications
You must be signed in to change notification settings - Fork 904
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
Make sure all example code blocks for datasets are runnable #2287
Comments
Part of this task was completed in #1962 Remaining datasets are:
|
Hi, I wanted to start contributing to Kedro, it's such an amazing tool, may I take this one? |
Hi @JoaoAreias, Welcome to the Kedro community 😄 That would be wonderful! This work will need to be done in |
Thank you! Will do! |
In addition, I think we should make sure the example are from |
It would be nice to actually use doctest for these. (Rather than verifying once manually.) |
Agreed @deepyaman! Putting the examples into a test script and including as part of ongoing CI would be awesome. |
I would like to take this one |
Hi @Gundalai-Batkhuu, feel free to take #2287, #2604, #2643, and if @ggermade does not reappear in a few days, #2008. Just bear in mind that we are not assigning all the issues at once, focus on one of them and feel free to open a PR when you're ready. Other people might take them, so be quick! And rather than asking us whether you can start working on them, either open a PR directly, or if there's something you don't understand about the issue or the scope, ask for clarification in the relevant ticket. This will increase your chances of success. Good luck! |
kedro-org/kedro-plugins#416 is a first attempt at validating using doctest. Example run: https://github.com/kedro-org/kedro-plugins/actions/runs/6634484238/job/18023981594?pr=416 As @merelcht mentioned, some of the tests reference S3, or data files that don't exist; many of these can probably be updated. There are some where the issue is just that the correct output isn't reflected. Certain cases, the doctests are catching legitimate mistakes it seems (e.g. missing arguments). Want to take a pause and check, before investing more time on this--are we aligned on/okay with using doctest? |
Failure details
|
I am more than happy with using doctest if it means we end up with solid code that works for readers. I'm somewhat overwhelmed by the issues listed above though: do we need to ticket all these? |
Hi @deepyaman, I would like to contribute to updating the docstrings at Do you have any suggestions about which ones I could look at? |
@laizaparizotto Sorry for not responding to you earlier! Somebody just brought it to my attention this morning. Would love your help. If you see https://github.com/kedro-org/kedro-plugins/blob/main/Makefile#L30, all these files have examples that are failing in one way or another; feel free to take any of them (ideally, that don't have a PR already). I've just raised one for Let me know if you have any questions, or run into issues! |
Fixes tried, but no success and might be too complicated:
|
All fixable dataset docstrings are now fixed. The remaining examples all require complicated cloud/database client setup, which is overkill for the examples. I'll close this as completed. |
Description
Some of the code examples we provide in the API docs for datasets (https://docs.kedro.org/en/stable/kedro_datasets.html#module-kedro_datasets) aren't actually runnable. Some datasets have easy and straightforward examples that can be copy-pasted and run straight away, others reference setup including S3, but it's not clear these snippets won't be runnable.
Implementation
Update all code snippets on in the dataset API docs to basic examples that can be run. And in case a simpler example doesn't make sense, clarify that this snippet can't be run as is and what additional setup would be needed.
Please also make sure the example refer to
kedro-datasets
but notkedro
The text was updated successfully, but these errors were encountered: