Validate code snippets in datasets documentation - Part 1 #1962

AhdraMeraliQB · 2022-10-21T11:49:54Z

Description

This follows on from #2287. In this PR I have tested all the datasets that can be instantiated locally. The following datasets remain to be tested in part 2:

dask.ParquetDataSet
holoviews.HoloviewsWriter
pandas.GBQQueryDataSet
pandas.GBQTableDataSet
pandas.SQLQueryDataSet
pandas.SQLTableDataSet
redis.PickleDataSet
all Spark datasets
tensorflow.TensorFlowModelDataset

The main changes in this PR are as follows:

Some code blocks had duplicate lines for providing filepath arguments. I believe this was to illustrate both local and external (S3/GCS) filesytems, but after discussion with @MerelTheisenQB we have agreed this is a bit redundant.
Some comments rendered with weird line breaks. This was fixed.
There was a comment for a dependency to be installed within an example code block. This has been moved out to extras_require in setup.py
Added missing imports

Also warranting a flag 🚩: GraphMLDataSet and GMLDataSet appear to be identical in purpose and in implementation

Development notes

Both the YAML and Python examples have been tested under the following setup:

New conda environment
pip install kedro
pip install kedro[all]

For Python:

kedro jupyter lab
Code snippets were copied and pasted over
An additional cell was added to check <dataset name>.exists() returned True

For YAML:

kedro new -s spaceflights
The dataset was added to the data catalog in conf/base/catalog.yml and added to the pipeline arbitrarily.

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

AhdraMeraliQB · 2022-10-21T11:52:20Z

kedro/extras/datasets/json/json_dataset.py

@@ -28,17 +28,13 @@ class JSONDataSet(AbstractVersionedDataSet[Any, Any]):
        >>> json_dataset:
        >>>   type: json.JSONDataSet
        >>>   filepath: data/01_raw/location.json
-        >>>   load_args:


JSON Dataset does not support the load_args parameter: https://kedro.readthedocs.io/en/stable/kedro.extras.datasets.json.JSONDataSet.html#kedro.extras.datasets.json.JSONDataSet.__init__

Great catch!

* Update release version and release notes Signed-off-by: Nok Chan <[email protected]> * Update missing release notes Signed-off-by: Nok Chan <[email protected]> * update vresion Signed-off-by: Nok Chan <[email protected]> * update release notes Signed-off-by: Nok Chan <[email protected]> Signed-off-by: Nok Chan <[email protected]> Signed-off-by: Ahdra Merali <[email protected]>

Signed-off-by: Ahdra Merali <[email protected]>

* Fix code snippets Signed-off-by: Ahdra Merali <[email protected]> * Separate code blocks Signed-off-by: Ahdra Merali <[email protected]> * Lint Signed-off-by: Ahdra Merali <[email protected]> Signed-off-by: Ahdra Merali <[email protected]>

Signed-off-by: jstammers <[email protected]> Signed-off-by: Ahdra Merali <[email protected]>

* Update RELEASE.md * fix broken link * Update RELEASE.md Co-authored-by: Merel Theisen <[email protected]> Co-authored-by: Merel Theisen <[email protected]> Signed-off-by: Ahdra Merali <[email protected]>

* Deprecating `kedro test` and `kedro lint` Signed-off-by: Nok Chan <[email protected]> * Deprecate commands Signed-off-by: Nok Chan <[email protected]> * Make kedro looks prettier * Update Linting Signed-off-by: Nok <[email protected]> Signed-off-by: Nok Chan <[email protected]> Signed-off-by: Nok <[email protected]> Signed-off-by: Ahdra Merali <[email protected]>

Signed-off-by: Florian Gaudin-Delrieu <[email protected]> Signed-off-by: Ahdra Merali <[email protected]>

…related issues better (#1881) * Update message for VersionNotFoundError Signed-off-by: Ankita Katiyar <[email protected]> * Add test for VersionNotFoundError for cloud protocols * Update test_data_catalog.py Update NoVersionFoundError test * minor linting update * update docs link + styling changes * Revert "update docs link + styling changes" This reverts commit 6088e00. * Update test with styling changes * Update RELEASE.md Signed-off-by: ankatiyar <[email protected]> Signed-off-by: Ankita Katiyar <[email protected]> Signed-off-by: ankatiyar <[email protected]> Co-authored-by: Ahdra Merali <[email protected]> Signed-off-by: Ahdra Merali <[email protected]>

Signed-off-by: Merel Theisen <[email protected]> Signed-off-by: Ahdra Merali <[email protected]>

Signed-off-by: Ahdra Merali <[email protected]>

* Add best_practices.md with introductory sections Signed-off-by: Jannic Holzer <[email protected]> * Add pytest and pytest-cov sections Signed-off-by: Jannic Holzer <[email protected]> * Add pytest-cov coverage report Signed-off-by: Jannic Holzer <[email protected]> * Add sections on pytest-cov Signed-off-by: Jannic Holzer <[email protected]> * Add automated_testing to index.rst Signed-off-by: Jannic Holzer <[email protected]> * Reformat third-party library names and clean grammar. Signed-off-by: Jannic Holzer <[email protected]> * Add link to virtual environment docs Signed-off-by: Jannic Holzer <[email protected]> * Add example of good test naming Signed-off-by: Jannic Holzer <[email protected]> * Improve link accessibility Signed-off-by: Jannic Holzer <[email protected]> * Improve pytest docs link accessibility Signed-off-by: Jannic Holzer <[email protected]> * Add reminder link to virtual environment docs Signed-off-by: Jannic Holzer <[email protected]> * Fix formatting in link to coverage docs Signed-off-by: Jannic Holzer <[email protected]> * Remove reference to /src under 'Run your tests' Signed-off-by: Jannic Holzer <[email protected]> * Modify references to <project_name> to <package_name> Signed-off-by: Jannic Holzer <[email protected]> * Fix sentence structure Signed-off-by: Jannic Holzer <[email protected]> * Fix broken databricks doc link Signed-off-by: Jannic Holzer <[email protected]> Signed-off-by: Jannic Holzer <[email protected]> Signed-off-by: Ahdra Merali <[email protected]>

* Capitalised kedro-viz Signed-off-by: yash6318 <[email protected]> * capitalised Kedro viz Signed-off-by: yash6318 <[email protected]> * Updated set_up_experiment_tracking.md Co-authored-by: Deepyaman Datta <[email protected]> Signed-off-by: yash6318 <[email protected]> Signed-off-by: yash6318 <[email protected]> Co-authored-by: Deepyaman Datta <[email protected]> Signed-off-by: Ahdra Merali <[email protected]>

Signed-off-by: Merel Theisen <[email protected]> Signed-off-by: Ahdra Merali <[email protected]>

Signed-off-by: Carla Vieira <[email protected]> Signed-off-by: Ahdra Merali <[email protected]>

* Change a file extention to match the previous article Signed-off-by: dinotuku <[email protected]> * Add a missing import Signed-off-by: dinotuku <[email protected]> * Change both preprocessed datasets to parquet files Signed-off-by: dinotuku <[email protected]> * Change data type to ParquetDataSet for parquet files Signed-off-by: dinotuku <[email protected]> * Add a note for installing seaborn if it is not installed Signed-off-by: dinotuku <[email protected]> Signed-off-by: dinotuku <[email protected]> Signed-off-by: Ahdra Merali <[email protected]>

* Add documentation for linting tools Signed-off-by: Ankita Katiyar <[email protected]> * Revert changes to commands_reference.md Signed-off-by: Ankita Katiyar <[email protected]> * Update linting docs with suggestions Signed-off-by: Ankita Katiyar <[email protected]> * Update linting doc Signed-off-by: Ankita Katiyar <[email protected]> Signed-off-by: Ankita Katiyar <[email protected]> Signed-off-by: Ahdra Merali <[email protected]>

Signed-off-by: Merel Theisen <[email protected]> Signed-off-by: Ahdra Merali <[email protected]>

* Create dependabot.yml configuration file * Update dependabot.yml Signed-off-by: SajidAlamQB <[email protected]> * add target-branch Signed-off-by: SajidAlamQB <[email protected]> * Update dependabot.yml Signed-off-by: SajidAlamQB <[email protected]> * limit dependabot to just dependency folder Signed-off-by: SajidAlamQB <[email protected]> * Update test_requirements.txt Signed-off-by: SajidAlamQB <[email protected]> * Update MANIFEST.in Signed-off-by: SajidAlamQB <[email protected]> * fix e2e Signed-off-by: SajidAlamQB <[email protected]> * Update continue_config.yml Signed-off-by: SajidAlamQB <[email protected]> * Update requirements.txt Signed-off-by: SajidAlamQB <[email protected]> * Update requirements.txt Signed-off-by: SajidAlamQB <[email protected]> * fix link Signed-off-by: SajidAlamQB <[email protected]> * revert Signed-off-by: SajidAlamQB <[email protected]> * Delete requirements.txt Signed-off-by: SajidAlamQB <[email protected]> Signed-off-by: SajidAlamQB <[email protected]> Signed-off-by: Ahdra Merali <[email protected]>

Signed-off-by: Ahdra Merali <[email protected]>

* Update dependabot.yml Signed-off-by: SajidAlamQB <[email protected]> * pin jupyterlab_services to requirments Signed-off-by: SajidAlamQB <[email protected]> * lint Signed-off-by: SajidAlamQB <[email protected]> Signed-off-by: SajidAlamQB <[email protected]> Signed-off-by: Ahdra Merali <[email protected]>

Signed-off-by: Ahdra Merali <[email protected]>

Updates the requirements on [pip-tools](https://github.com/jazzband/pip-tools) to permit the latest version. - [Release notes](https://github.com/jazzband/pip-tools/releases) - [Changelog](https://github.com/jazzband/pip-tools/blob/master/CHANGELOG.md) - [Commits](jazzband/pip-tools@6.5.0...6.9.0) --- updated-dependencies: - dependency-name: pip-tools dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Ahdra Merali <[email protected]>

Updates the requirements on [toposort]() to permit the latest version. --- updated-dependencies: - dependency-name: toposort dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Sajid Alam <[email protected]> Signed-off-by: Ahdra Merali <[email protected]>

…1953) Signed-off-by: Merel Theisen <[email protected]> Signed-off-by: Ahdra Merali <[email protected]>

* remove a redundant function call Signed-off-by: Nok Chan <[email protected]> * Remove redundant resolove_load_version & fix test Signed-off-by: Nok Chan <[email protected]> * Fix HoloviewWriter tests with more specific error message pattern & Lint Signed-off-by: Nok Chan <[email protected]> * Rename tests Signed-off-by: Nok Chan <[email protected]> Signed-off-by: Nok Chan <[email protected]> Signed-off-by: Ahdra Merali <[email protected]>

Signed-off-by: Ahdra Merali <[email protected]>

merelcht · 2022-10-21T12:33:52Z

I found the PR where the GraphMLDataSet and GMLDataSet were added: #881 It looks like both of them were added because "Included GraphML in addition to GML. Some tools prefer GraphML as it's able to perfectly preserve internal property maps (ref)."

So let's just keep them both.

merelcht

Great work on going through all these examples and testing them! 👍 ⭐

merelcht · 2022-10-21T12:36:50Z

kedro/extras/datasets/json/json_dataset.py

@@ -28,17 +28,13 @@ class JSONDataSet(AbstractVersionedDataSet[Any, Any]):
        >>> json_dataset:
        >>>   type: json.JSONDataSet
        >>>   filepath: data/01_raw/location.json
-        >>>   load_args:


Great catch!

merelcht · 2022-10-21T12:38:11Z

setup.py

@@ -78,6 +78,7 @@ def _collect_requirements(requires):
    "pandas.XMLDataSet": [PANDAS, "lxml~=4.6"],
    "pandas.GenericDataSet": [PANDAS],
 }
+pickle_require = {"pickle.PickleDataSet": ["compress-pickle~=2.1.0"]}


The original comment said: "# Add "compress_pickle[lz4]" to requirements.txt" Do we need to specify [lz4] for this requirement?

Good catch - this is what the pip list showed after manually installing compress_pickle[lz4], I'll make the change to follow the dask example further up

AhdraMeraliQB · 2022-10-21T12:43:56Z

@MerelTheisenQB

I found the PR where the GraphMLDataSet and GMLDataSet were added: #881 It looks like both of them were added because "Included GraphML in addition to GML. Some tools prefer GraphML as it's able to perfectly preserve internal property maps (ref)."

So let's just keep them both.

I took a look at the ref but couldn't see the preference; is it the name that they prefer? As the implementation is carbon copies 😬

ankatiyar

Looks good to me!

merelcht · 2022-10-24T16:01:46Z

I took a look at the ref but couldn't see the preference; is it the name that they prefer? As the implementation is carbon copies 😬

The difference between GraphML and GML (apparently) is that GraphML preserves internal property maps, but GML doesn't. See this comment in the docs: "The only file formats which are capable of perfectly preserving the internal property maps are “gt” and “graphml”. Because of this, they should be preferred over the other formats whenever possible." https://graph-tool.skewed.de/static/doc/graph_tool.html#graph_tool.load_graph

Perhaps the question is whether our users make use of both the datasets or not. It's not an extreme maintenance burden so I'd just keep them both.

Signed-off-by: Merel Theisen <[email protected]>

AhdraMeraliQB requested a review from idanov as a code owner October 21, 2022 11:49

AhdraMeraliQB requested review from merelcht and ankatiyar October 21, 2022 11:50

AhdraMeraliQB commented Oct 21, 2022

View reviewed changes

noklam and others added 26 commits October 21, 2022 13:19

Remove comment from code example

570c574

Signed-off-by: Ahdra Merali <[email protected]>

Remove more comments

981dcfe

Signed-off-by: Ahdra Merali <[email protected]>

Add YAML formatting

c60aef6

Signed-off-by: Ahdra Merali <[email protected]>

Add missing import

07159ff

Signed-off-by: Ahdra Merali <[email protected]>

Remove even more comments

09b0d46

Signed-off-by: Ahdra Merali <[email protected]>

Remove more even more comments

48ba471

Signed-off-by: Ahdra Merali <[email protected]>

Add pickle requirement to extras_require

770e3b9

Signed-off-by: Ahdra Merali <[email protected]>

Try fix YAML docs

66efe0d

Signed-off-by: Ahdra Merali <[email protected]>

Try fix YAML docs pt 2

80151f8

Signed-off-by: Ahdra Merali <[email protected]>

Fix code snippets in docs (#1876)

6e52ba6

* Fix code snippets Signed-off-by: Ahdra Merali <[email protected]> * Separate code blocks Signed-off-by: Ahdra Merali <[email protected]> * Lint Signed-off-by: Ahdra Merali <[email protected]> Signed-off-by: Ahdra Merali <[email protected]>

Fix issue with specifying format for SparkHiveDataSet (#1857)

4e3e7b4

Signed-off-by: jstammers <[email protected]> Signed-off-by: Ahdra Merali <[email protected]>

Update RELEASE.md (#1883)

d955135

* Update RELEASE.md * fix broken link * Update RELEASE.md Co-authored-by: Merel Theisen <[email protected]> Co-authored-by: Merel Theisen <[email protected]> Signed-off-by: Ahdra Merali <[email protected]>

Fix micro package pull from PyPI (#1848)

57384cc

Signed-off-by: Florian Gaudin-Delrieu <[email protected]> Signed-off-by: Ahdra Merali <[email protected]>

Update experiment tracking documentation with working examples (#1893)

ce070f5

Signed-off-by: Merel Theisen <[email protected]> Signed-off-by: Ahdra Merali <[email protected]>

Add NHS AI Lab and ReSpo.Vision to companies list (#1878)

4f45905

Signed-off-by: Ahdra Merali <[email protected]>

Fix linting on autmated test page (#1906)

6fa2048

Signed-off-by: Merel Theisen <[email protected]> Signed-off-by: Ahdra Merali <[email protected]>

Add _SINGLE_PROCESS property to CachedDataSet (#1905)

ed06e8d

Signed-off-by: Carla Vieira <[email protected]> Signed-off-by: Ahdra Merali <[email protected]>

Make core config accessible in dict get way (#1870)

72d9b96

Signed-off-by: Merel Theisen <[email protected]> Signed-off-by: Ahdra Merali <[email protected]>

SajidAlamQB and others added 9 commits October 21, 2022 13:19

Update robots.txt (#1929)

81cf5a4

Signed-off-by: Ahdra Merali <[email protected]>

fix broken link (#1950)

0803e74

Signed-off-by: Ahdra Merali <[email protected]>

Update setup.py Jinja2 dependencies (#1954)

3ce5dcb

Signed-off-by: Ahdra Merali <[email protected]>

Add deprecation warning to package_name argument in session create() (#…

0760011

…1953) Signed-off-by: Merel Theisen <[email protected]> Signed-off-by: Ahdra Merali <[email protected]>

Make docstring in test starter match real starters (#1916)

011b5bb

Signed-off-by: Ahdra Merali <[email protected]>

AhdraMeraliQB force-pushed the fix/check-code-snippets branch from 37dc351 to 011b5bb Compare October 21, 2022 12:19

AhdraMeraliQB requested a review from yetudada as a code owner October 21, 2022 12:19

Merge branch 'main' into fix/check-code-snippets

14fbc85

merelcht approved these changes Oct 21, 2022

View reviewed changes

Merge branch 'main' into fix/check-code-snippets

46d1c30

ankatiyar approved these changes Oct 21, 2022

View reviewed changes

Merge branch 'main' into fix/check-code-snippets

571866d

This was referenced Nov 7, 2022

Release Kedro 0.18.4 #2004

Closed

Add generic HDF5 dataset #1964

Closed

Merge branch 'main' into fix/check-code-snippets

9b1a9e4

merelcht mentioned this pull request Nov 7, 2022

Add SVMLight DataSet #1992

Merged

5 tasks

AhdraMeraliQB self-assigned this Nov 8, 2022

merelcht and others added 3 commits November 8, 2022 13:41

Try to fix formatting error

2ec5c20

Signed-off-by: Merel Theisen <[email protected]>

Merge branch 'main' into fix/check-code-snippets

84ce260

Specify pickle import

d0fa348

AhdraMeraliQB merged commit f52249b into main Nov 9, 2022

AhdraMeraliQB deleted the fix/check-code-snippets branch November 9, 2022 08:03

merelcht mentioned this pull request Nov 9, 2022

Make sure all example code blocks for datasets are runnable #2287

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate code snippets in datasets documentation - Part 1 #1962

Validate code snippets in datasets documentation - Part 1 #1962

AhdraMeraliQB commented Oct 21, 2022 •

edited

Loading

AhdraMeraliQB Oct 21, 2022

merelcht Oct 21, 2022

merelcht commented Oct 21, 2022

merelcht left a comment

merelcht Oct 21, 2022

merelcht Oct 21, 2022

AhdraMeraliQB Nov 8, 2022

AhdraMeraliQB commented Oct 21, 2022

ankatiyar left a comment

merelcht commented Oct 24, 2022

Validate code snippets in datasets documentation - Part 1 #1962

Validate code snippets in datasets documentation - Part 1 #1962

Conversation

AhdraMeraliQB commented Oct 21, 2022 • edited Loading

Description

Development notes

Both the YAML and Python examples have been tested under the following setup:

For Python:

For YAML:

Checklist

AhdraMeraliQB Oct 21, 2022

Choose a reason for hiding this comment

merelcht Oct 21, 2022

Choose a reason for hiding this comment

merelcht commented Oct 21, 2022

merelcht left a comment

Choose a reason for hiding this comment

merelcht Oct 21, 2022

Choose a reason for hiding this comment

merelcht Oct 21, 2022

Choose a reason for hiding this comment

AhdraMeraliQB Nov 8, 2022

Choose a reason for hiding this comment

AhdraMeraliQB commented Oct 21, 2022

ankatiyar left a comment

Choose a reason for hiding this comment

merelcht commented Oct 24, 2022

AhdraMeraliQB commented Oct 21, 2022 •

edited

Loading