Use user defined default dataset factory pattern over the one from the runner #3859

ankatiyar · 2024-05-08T13:42:35Z

Description

Fix #3720

Development notes

TODO:

Wanted to get opinions on the proposed solution before proceeding with the pending tasks mentioned above^

Proposed Solution

When `DataCatalog` is created (in `from_config`)

If the last pattern in the sorted patterns list is a catch-all pattern i.e. has specificity == 0, it is popped out and stored separately as the DataCatalog._default_pattern (This would be the user-defined default pattern, not from the runner)

Note

This assumes that there is only one catch-all pattern in the user's catalog. If there are more, the last one will be popped which wouldn't ever be used anyway.❓ [Question for reviewers]: We should probably error out if the user has more than one catch-all patterns in this case, does that make sense?

When a dataset is being fetched

Try to match is against the patterns in DataCatalog._dataset_patterns OR DataCatalog._default_pattern
Resolve config again first from _dataset_patterns OR _default_pattern

When runner.run() is called

When kedro run is executed, the runner creates a shallow copy of the catalog object by calling the DataCatalog.shallow_copy()

Here the runner inserts a default dataset pattern e.g. extra_dataset_patterns = {'{default}': MemoryDataset}
IF there is already a _default_pattern - it's not needed to do anything
IF there isn't a user defined catch-all pattern - add it to the DataCatalog._dataset_patterns (this will then not emit a warning about the catch-all pattern being used over the runner default dataset)

For the catalog CLI commands

Similar logic as above has been copied for the matching of datasets to patterns

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: Ankita Katiyar <ankitakatiyar2401@gmail.com>

noklam · 2024-05-09T16:21:15Z

[Question for reviewers]: We should probably error out if the user has more than one catch-all patterns in this case, does that make sense?

Yes

ElenaKhaustova · 2024-05-09T20:12:02Z

kedro/framework/cli/catalog.py

-                    data_catalog._dataset_patterns[matched_pattern]
+                    data_catalog._dataset_patterns.get(matched_pattern)
+                    or data_catalog._default_pattern.get(matched_pattern)
+                    or {}


Is this case possible? If matches are found doesn't it mean that one of data_catalog._dataset_patterns or data_catalog._default_pattern should have it?

It wouldn't be by mypy was complaining 🫡

ElenaKhaustova

Looked through the approach suggested and it seems good to me.

Agree that we should validate if user has more than one catch-all patterns.

Left one question regarding the implementation though it's quite hard to validate it without tests. 🙂

This ticket also made me think that it would be good to have this resolution logic in one place, but that's ofc out of the scope now.

noklam · 2024-05-10T11:30:14Z

Just dumping my thought here in case I am blocking the PRs. I have some questions about the shallow_copy approach, but that was done long before this PR so I don't want to expand the discussion too much.

I think:

There should be only 1 default dataset, validation should happen when DataCatalog is initialised. (I think this is consistent as the shallow_copy will create a new DataCatalog, this is where I find the shallow_copy weird as it does not seem to be a reference but completely new object
multiple dataset pattern by Runner is fine. As long as only one of them is default datasets. For example, you could have different default for different namespaces which control by runner.

The most important question here is "should runner dataset patterns override DataCatalog"? In framework setting, the way to override patterns is catalog.yml. However, the order of how framework work is different.

DataCatalog read catalog.yml first
Runner override default patterns (memory dataset)

This suggests that one should only use either catalog.yml or runner. Should it fails or just ignore the dataset pattern from runner? For me, I think I will go for raising an error instead of ignoring the pattern sliently.

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>

merelcht

I left some comments, but generally the approach looks good 👍

kedro/io/data_catalog.py

merelcht · 2024-05-14T10:05:38Z

kedro/io/data_catalog.py

+        ]
+        if len(catch_all) > 1:
+            raise DatasetError(
+                f"Multiple catch-all patterns found in the catalog: {', '.join(catch_all)}"


It might be helpful to add here how to resolve the problem by making sure only one catch-all exists in the catalog.

tests/io/test_data_catalog.py

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>

merelcht

LGTM 👍

ElenaKhaustova

Left one minor comment, the rest LGTM! 🚀

ElenaKhaustova · 2024-05-14T11:31:00Z

docs/source/data/kedro_dataset_factories.md

@@ -223,19 +223,14 @@ The matches are ranked according to the following criteria:
 You can use dataset factories to define a catch-all pattern which will overwrite the default [`MemoryDataset`](/api/kedro.io.MemoryDataset) creation.

 ```yaml
-"{a_default_dataset}":
+"{default_dataset}":


Cannot comment on the target line as it wasn't changed but we need to update the filepath as well

Good catch! Updated!

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>

ankatiyar added 3 commits May 8, 2024 14:39

Use user default pattern over the ones from the runner

adbcd3f

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>

Update catalog CLI commands

4e4b7e0

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>

Fix lint

38f45c2

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>

ankatiyar requested review from noklam, ElenaKhaustova and idanov May 8, 2024 15:02

ElenaKhaustova reviewed May 9, 2024

View reviewed changes

ankatiyar added 2 commits May 13, 2024 09:21

Fix tests and add error for multiple catch-all patterns

16bd4df

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>

Remove note from docs

d2191cd

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>

ankatiyar marked this pull request as ready for review May 13, 2024 08:34

ankatiyar requested review from yetudada, astrojuanlu and merelcht as code owners May 13, 2024 08:34

ankatiyar requested a review from ElenaKhaustova May 13, 2024 15:54

merelcht reviewed May 14, 2024

View reviewed changes

ankatiyar and others added 2 commits May 14, 2024 11:49

Suggestions from code review, release notes

62c57a7

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>

Merge branch 'main' into dataset-factories-default

1a61878

merelcht approved these changes May 14, 2024

View reviewed changes

ElenaKhaustova approved these changes May 14, 2024

View reviewed changes

ankatiyar and others added 2 commits May 14, 2024 13:23

Docs changes

e2438e4

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>

Merge branch 'main' into dataset-factories-default

6c1f666

ankatiyar enabled auto-merge (squash) May 14, 2024 12:25

ankatiyar merged commit 9369227 into main May 14, 2024
41 checks passed

ankatiyar deleted the dataset-factories-default branch May 14, 2024 12:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use user defined default dataset factory pattern over the one from the runner #3859

Use user defined default dataset factory pattern over the one from the runner #3859

ankatiyar commented May 8, 2024 •

edited

noklam commented May 9, 2024

ElenaKhaustova May 9, 2024

ankatiyar May 9, 2024

ElenaKhaustova left a comment

noklam commented May 10, 2024

merelcht left a comment

merelcht May 14, 2024

ankatiyar May 14, 2024

merelcht left a comment

ElenaKhaustova left a comment

ElenaKhaustova May 14, 2024

ankatiyar May 14, 2024

Use user defined default dataset factory pattern over the one from the runner #3859

Use user defined default dataset factory pattern over the one from the runner #3859

Conversation

ankatiyar commented May 8, 2024 • edited

Description

Development notes

TODO:

Proposed Solution

When DataCatalog is created (in from_config)

When a dataset is being fetched

When runner.run() is called

For the catalog CLI commands

Checklist

noklam commented May 9, 2024

ElenaKhaustova May 9, 2024

Choose a reason for hiding this comment

ankatiyar May 9, 2024

Choose a reason for hiding this comment

ElenaKhaustova left a comment

Choose a reason for hiding this comment

noklam commented May 10, 2024

merelcht left a comment

Choose a reason for hiding this comment

merelcht May 14, 2024

Choose a reason for hiding this comment

ankatiyar May 14, 2024

Choose a reason for hiding this comment

merelcht left a comment

Choose a reason for hiding this comment

ElenaKhaustova left a comment

Choose a reason for hiding this comment

ElenaKhaustova May 14, 2024

Choose a reason for hiding this comment

ankatiyar May 14, 2024

Choose a reason for hiding this comment

ankatiyar commented May 8, 2024 •

edited

When `DataCatalog` is created (in `from_config`)