Change how kedro import datasets and raise explicit error when dependencies is missing #3272

noklam · 2023-11-06T05:14:09Z

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

Fix #2943

How are datasets can be imported?

Explicit import in Python -

from kedro_datasets.biosequence import BioSequenceDataSet

Via kedro's _load_ob -

kedro/kedro/io/core.py

Lines 381 to 389 in 38dfe6f

    
           if isinstance(class_obj, str): 
        
               if len(class_obj.strip(".")) != len(class_obj): 
        
                   raise DataSetError( 
        
                       "'type' class path does not support relative " 
        
                       "paths or paths ending with a dot." 
        
                   ) 
        
               class_paths = (prefix + class_obj for prefix in _DEFAULT_PACKAGES) 
        
               trials = (_load_obj(class_path) for class_path in class_paths)

In particular, kedro try to import datasets from a _DEFAULT_PACKAGES iteratively

kedro/kedro/io/core.py

Line 350 in 38dfe6f

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

For example, when user do catalog.load("companies"), the following happens

import from kedro.io.pandas - ModuleNotFoundError
kedro_datasets.pandas is found and exit the loop

The actual import mechanism is hidden in kedro.io.core's parse_dataset_definition which has extra try/except with _load_obj.

To test this, you can do this.

kedro new -s pandas-iris
pip uninstall pandas
kedro run

or you can directly trigger this import mechanism via the private method.

from kedro.io.core import parse_dataset_definition
config = {'type': 'pandas.CSVDataset',
           'filepath': '/01_raw/iris.csv'}


parse_dataset_definition(config)

Development notes

The main change is to differentiate two different scenarios.
Noted that since kedro_datasets moved to lazy loading, hasattr can trigger a ModuleNotFoundError.

Questions for reviewer

I remove the AttributeError in load_obj, honestly I don't think that add any more information and make the code obscure.

Previously the error was hidden because ModulenotFoundError can be two cases:

The datasets doesn't exist. i.e. `kedro_datasets.fake_dataset.x
The datasets exist, but the underlying dependencies is missing (i.e. pandas)

Developer Certificate of Origin

We need all contributions to comply with the Developer Certificate of Origin (DCO). All commits must be signed off by including a Signed-off-by line in the commit message. See our wiki for guidance.

If your PR is blocked due to unsigned commits, then you must follow the instructions under "Rebase the branch" on the GitHub Checks page for your PR. This will retroactively add the sign-off to all unsigned commits and allow the DCO check to pass.

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: Nok Chan <nok.lam.chan@quantumblack.com>

noklam · 2023-11-06T08:25:12Z

Github Action is not triggered, not sure why.

Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>

* Replace a `StopIteration` exception with for-else Signed-off-by: Nok Lam Chan <nok.lam.chan@quantumblack.com> * fix the logic Signed-off-by: Nok <nok.lam.chan@quantumblack.com> * bug fix Signed-off-by: Nok <nok.lam.chan@quantumblack.com> --------- Signed-off-by: Nok Lam Chan <nok.lam.chan@quantumblack.com> Signed-off-by: Nok <nok.lam.chan@quantumblack.com>

Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>

astrojuanlu · 2023-11-07T09:44:16Z

Could exception groups help here? https://github.com/agronholm/exceptiongroup

Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>

noklam · 2023-11-07T10:02:13Z

kedro/utils.py

-    if not hasattr(module_obj, obj_name):
-        raise AttributeError(f"Object '{obj_name}' cannot be loaded from '{obj_path}'.")


This can now create a ModuleNotFoundError due to lazy-loading. I decide to remove it because it adds more complexity but the error message itself isn't helpful at all.

Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>

noklam · 2023-11-07T10:19:37Z

When dependencies is missing.

DatasetError: No module named 'pandas' Please see the documentation on how to install relevant dependencies for kedro_datasets.pandas.CSVDataset:
https://kedro.readthedocs.io/en/stable/kedro_project_setup/dependencies.html

When we have a typo.

DatasetError: Class 'pandaxx.CSVDataset' not found or one of its dependencies has not been installed.

@astrojuanlu I'd like to get some review for the current approach. We can change the error message (or in a separate PR if we are changing all of them).

ps. All tests passed except 1 because I updated what load_obj do, currently it is only used in two place, load_obj itself is a weird function, it is the only function in kedro.utils which was created 4 years ago.

Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>

astrojuanlu · 2023-11-07T10:59:48Z

I confirm this now disambiguates both cases 👍🏽

The error messages could be:

Typo: "Class 'biosequence.BiosequenceDataSet' not found" (hence remove the "or one of its dependencies has not been installed"?)
Module not found: replace link with https://docs.kedro.org/en/stable/kedro_project_setup/dependencies.html#install-dependencies-related-to-the-data-catalog

merelcht · 2023-11-07T12:02:38Z

I confirm this now disambiguates both cases 👍🏽

I can also confirm this 🙂

The error messages could be:

Typo: "Class 'biosequence.BiosequenceDataSet' not found" (hence remove the "or one of its dependencies has not been installed"?)

For this one, should we add anything about checking if kedro-datasets is installed?

Module not found: replace link with https://docs.kedro.org/en/stable/kedro_project_setup/dependencies.html#install-dependencies-related-to-the-data-catalog

👍

Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>

…o-org/kedro into noklam/make-import-failures-in-2943 Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>

Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>

Signed-off-by: Nok Lam Chan <nok.lam.chan@quantumblack.com>

Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>

noklam · 2023-11-08T07:11:17Z

Typo: "Class 'biosequence.BiosequenceDataSet' not found" (hence remove the "or one of its dependencies has not been installed"?)
@astrojuanlu We still need to consider custom dataset or plugin, missing package can be a valid cause. Consider this case.

In [4]: from kedro.io.core import parse_dataset_definition
   ...: config = {'type': 'kedro_mlflow.fake.fake.CSVDataset',
   ...:            'filepath': '/01_raw/iris.csv'}
   ...: 
   ...: parse_dataset_definition(config)

DatasetError: Class 'kedro_mlflow.fake.fake.CSVDataset' not found, is this a typo?

kedro-datasets is mandatory for 0.19.0, I don't think we need to worry about this @merelcht
custom datasets / plugins always requires full path, so the missing dependencies should be a good error message already.

Cc @datajoely for some inspiration

noklam · 2023-11-08T07:12:45Z

kedro/io/core.py

+    dataset_type = config.pop("type")
+    if isinstance(dataset_type, str):
+        if len(dataset_type.strip(".")) != len(dataset_type):
            raise DatasetError(
                "'type' class path does not support relative "
                "paths or paths ending with a dot."
            )
-        class_paths = (prefix + class_obj for prefix in _DEFAULT_PACKAGES)
+        class_paths = (prefix + dataset_type for prefix in _DEFAULT_PACKAGES)


refactor the name because class_obj is overloaded and confusing. Originally it is a str but load as a class later, it's better to make it explicit.

noklam · 2023-11-08T07:13:27Z

kedro/io/core.py

+                f"{exc}. Please see the documentation on how to "
                f"install relevant dependencies for {class_path}:\n"
-                f"https://kedro.readthedocs.io/en/stable/"
-                f"kedro_project_setup/dependencies.html"
+                f"https://docs.kedro.org/en/stable/kedro_project_setup/"
+                f"dependencies.html#install-dependencies-related-to-the-data-catalog"


updated the link according to review suggestion

noklam · 2023-11-08T07:14:29Z

tests/io/test_core.py

+    def test_dataset_name_typo(self, mocker):
+        # If the module doesn't exist, it return None instead ModuleNotFoundError
+        mocker.patch("kedro.io.core.load_obj", return_value=None)
+        dataset_name = "lAmbDaDaTAsET"
+
+        with pytest.raises(
+            DatasetError, match=f"Class '{dataset_name}' not found, is this a typo?"
+        ):
+            parse_dataset_definition({"type": dataset_name})
+
+    def test_dataset_missing_dependencies(self, mocker):
+        # If the module is found but import the dataset trigger ModuleNotFoundError
+        dataset_name = "LambdaDataset"
+
+        def side_effect_function(value):
+            if "__all__" in value:
+                return [dataset_name]
+            else:
+                raise ModuleNotFoundError
+
+        mocker.patch("kedro.io.core.load_obj", side_effect=side_effect_function)
+
+        pattern = "Please see the documentation on how to install relevant dependencies"
+        with pytest.raises(DatasetError, match=pattern):
+            parse_dataset_definition({"type": dataset_name})
+


Added some tests for testing the two common scenarios.

datajoely · 2023-11-08T09:54:18Z

So two thought -

Great work- we've tried fixing this before and this is a MUCH better solution
I wonder if we could include this part in @amandakys debugging research:

from kedro.io.core import parse_dataset_definition

One of the common debugging patterns I find myself supporting users is as follows:

A user is struggling to configure a YAML definition of a catalog entry (often with something tricky like credentials)
To isolate if the error is in YAML config or DataSet class implementation I often ask people to open a notebook and try instantiating the class imperatively.
Could we do something along the lines of generating some boilerplate for people to iterate against?
Once you have a working Python implementation could we have a .to_yaml() method?

astrojuanlu

I tried breaking this in different ways and I'm satisfied with the outcome 👍🏽 Thanks @noklam!

merelcht

Great work @noklam, this is a huge improvement! 👍

noklam force-pushed the noklam/make-import-failures-in-2943 branch 2 times, most recently from face399 to 93dc1a9 Compare November 6, 2023 07:05

noklam added 2 commits November 6, 2023 07:06

release note

20ef723

Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>

Refactor - split class_obj into class_obj and dataset_type

8b3d36f

Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>

noklam changed the base branch from main to develop November 6, 2023 07:15

bug fix

d4bff90

Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>

noklam linked an issue Nov 6, 2023 that may be closed by this pull request

Make import failures in kedro-datasets clearer #2943

Closed

Merge branch 'develop' into noklam/make-import-failures-in-2943

922d353

noklam self-assigned this Nov 6, 2023

noklam added 2 commits November 7, 2023 08:39

If module exists, raise error even it is ModuleNotFoundError

0e475c0

Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>

simplify load_obj

645a2ee

Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>

noklam commented Nov 7, 2023

View reviewed changes

minor bug fix, check if class exist

b92c5db

Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>

noklam added 2 commits November 7, 2023 10:26

remove obsolete test

d9ca8d1

Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>

object is a bad type hint

6e2a638

Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>

noklam requested review from merelcht and astrojuanlu November 7, 2023 10:32

noklam marked this pull request as ready for review November 7, 2023 10:32

noklam added 6 commits November 7, 2023 17:57

Merge branch 'develop' into noklam/make-import-failures-in-2943

96050ac

update error message link to installation guide

5d33a29

Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>

Merge branch 'noklam/make-import-failures-in-2943' of github.com:kedr…

5116411

…o-org/kedro into noklam/make-import-failures-in-2943 Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>

add tests

5d78809

Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>

Change error message

2450178

Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>

Merge branch 'develop' into noklam/make-import-failures-in-2943

10d0d0f

Signed-off-by: Nok Lam Chan <nok.lam.chan@quantumblack.com>

Update docstring

2a4e605

Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>

noklam changed the title ~~[DRAFT] - Change how kedro import datasets and raise explicit error when dependencies is missing~~ Change how kedro import datasets and raise explicit error when dependencies is missing Nov 8, 2023

fix lint

2121fe7

Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>

noklam commented Nov 8, 2023

View reviewed changes

astrojuanlu approved these changes Nov 8, 2023

View reviewed changes

merelcht approved these changes Nov 8, 2023

View reviewed changes

noklam added 2 commits November 9, 2023 10:30

Merge branch 'develop' into noklam/make-import-failures-in-2943

dff2973

Merge branch 'develop' into noklam/make-import-failures-in-2943

2dd537f

noklam merged commit 45bfddb into develop Nov 9, 2023
53 of 58 checks passed

noklam deleted the noklam/make-import-failures-in-2943 branch November 9, 2023 23:46

merelcht mentioned this pull request Nov 30, 2023

Seems like the rename from class_obj to dataset_type wasn't complete #3369

Closed

7 tasks

noklam mentioned this pull request Dec 7, 2023

Bug fix for parse dataest definition #3400

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change how kedro import datasets and raise explicit error when dependencies is missing #3272

Change how kedro import datasets and raise explicit error when dependencies is missing #3272

noklam commented Nov 6, 2023 •

edited

Loading

noklam commented Nov 6, 2023

astrojuanlu commented Nov 7, 2023

noklam Nov 7, 2023

noklam commented Nov 7, 2023

astrojuanlu commented Nov 7, 2023

merelcht commented Nov 7, 2023

noklam commented Nov 8, 2023

noklam Nov 8, 2023

noklam Nov 8, 2023

noklam Nov 8, 2023

datajoely commented Nov 8, 2023

astrojuanlu left a comment

merelcht left a comment

	if isinstance(class_obj, str):
	if len(class_obj.strip(".")) != len(class_obj):
	raise DataSetError(
	"'type' class path does not support relative "
	"paths or paths ending with a dot."
	)
	class_paths = (prefix + class_obj for prefix in _DEFAULT_PACKAGES)

	trials = (_load_obj(class_path) for class_path in class_paths)

		if not hasattr(module_obj, obj_name):
		raise AttributeError(f"Object '{obj_name}' cannot be loaded from '{obj_path}'.")

Change how kedro import datasets and raise explicit error when dependencies is missing #3272

Change how kedro import datasets and raise explicit error when dependencies is missing #3272

Conversation

noklam commented Nov 6, 2023 • edited Loading

Description

How are datasets can be imported?

Development notes

Questions for reviewer

Developer Certificate of Origin

Checklist

noklam commented Nov 6, 2023

astrojuanlu commented Nov 7, 2023

noklam Nov 7, 2023

Choose a reason for hiding this comment

noklam commented Nov 7, 2023

astrojuanlu commented Nov 7, 2023

merelcht commented Nov 7, 2023

noklam commented Nov 8, 2023

noklam Nov 8, 2023

Choose a reason for hiding this comment

noklam Nov 8, 2023

Choose a reason for hiding this comment

noklam Nov 8, 2023

Choose a reason for hiding this comment

datajoely commented Nov 8, 2023

astrojuanlu left a comment

Choose a reason for hiding this comment

merelcht left a comment

Choose a reason for hiding this comment

noklam commented Nov 6, 2023 •

edited

Loading