[DataCatalog2.0]: `Protocol` abstraction for `DataCatalog` #4160

ElenaKhaustova · 2024-09-12T21:49:27Z

Description

This introduces the Protocol abstraction for the current DataCatalog, which will be used to add new catalog implementations. Currently, it fully relies on the existing DataCatalog implementation to avoid breaking changes.

Development notes

Protocol class was excluded from test coverage since its method placeholders were marked as untested, see the related issue: nedbat/coveragepy#1616

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: Elena Khaustova <[email protected]>

datajoely · 2024-09-13T13:39:30Z

HUGE!

idanov

So neat and self-contained and best of all - non-breaking! If all references to DataCatalog are replaced with CatalogProtocol, I think it looks ready to merge for me.

kedro/runner/runner.py

noklam

My original understanding this is going to be a separate Catalog, but I see this PR has a lot of changes to existing API's typehint so I am slightly confused.

Does type hint suppose to work here? As it seems like it doesn't understand the catalog at all as all the IDE features seems broken.

noklam · 2024-09-13T14:48:10Z

kedro/framework/context/context.py

@@ -178,13 +178,13 @@ class KedroContext:
    )

    @property
-    def catalog(self) -> DataCatalog:
-        """Read-only property referring to Kedro's ``DataCatalog`` for this context.
+    def catalog(self) -> CatalogProtocol:


Question: Is this how type hint supposed to be done with Protocol? I roughly understand how Protocol works like traits/interface but I haven't seen much in a real codebase.

Yep, cause this is type in the end, see examples here: https://peps.python.org/pep-0544/#protocol-members

Signed-off-by: Elena Khaustova <[email protected]>

ElenaKhaustova · 2024-09-13T16:25:12Z

@noklam

Hmm, it seems working well in PyCharm and mypy does not complain as well.

The new catalog is added in the following PR. But first we need this to fit two implementations together. Here is a context regarding this decision: #4151 (comment)

noklam

Overall looks good to me. I find CatalogProtocol slightly weird, I don't know what's the convention is for Python, I found a list here: python/typeshed#4174

noklam

approved as non-blocking commenet

merelcht

This mostly looks good, but I left a comment about the methods added here that will be removed in the future.

kedro/io/core.py

Signed-off-by: Elena Khaustova <[email protected]>

merelcht

Don't forget to add this to the release notes ✍️

Otherwise, looks all good to me!

Signed-off-by: Elena Khaustova <[email protected]>

ElenaKhaustova added 30 commits July 31, 2024 18:16

Added a skeleton for AbstractDataCatalog and KedroDataCatalog

a8f4fb3

Signed-off-by: Elena Khaustova <[email protected]>

Removed from_config method

7d56818

Signed-off-by: Elena Khaustova <[email protected]>

Merge branch 'main' into refactor-pattern-logic

787e121

Implemented _init_datasets method

0b80f23

Signed-off-by: Elena Khaustova <[email protected]>

Implemented get dataset

5c727df

Signed-off-by: Elena Khaustova <[email protected]>

Started resolve_patterns implementation

05c9171

Signed-off-by: Elena Khaustova <[email protected]>

Implemented resolve_patterns

5c804d6

Signed-off-by: Elena Khaustova <[email protected]>

Merge branch 'main' into refactor-pattern-logic

e9ba5c4

Fixed credentials resolving

530f7d6

Signed-off-by: Elena Khaustova <[email protected]>

Updated match pattern

64be83c

Signed-off-by: Elena Khaustova <[email protected]>

Implemented add from dict method

c29828a

Signed-off-by: Elena Khaustova <[email protected]>

Updated io __init__

957403a

Signed-off-by: Elena Khaustova <[email protected]>

Added list method

14908ff

Signed-off-by: Elena Khaustova <[email protected]>

Implemented _validate_missing_keys

c5e925b

Signed-off-by: Elena Khaustova <[email protected]>

Added datasets access logic

b9a92b0

Signed-off-by: Elena Khaustova <[email protected]>

Merge branch 'main' into refactor-pattern-logic

2cb794f

Added __contains__ and comments on lazy loading

2f32593

Signed-off-by: Elena Khaustova <[email protected]>

Renamed dataset_name to ds_name

d1ea64e

Signed-off-by: Elena Khaustova <[email protected]>

Updated some docstrings

fb89fca

Signed-off-by: Elena Khaustova <[email protected]>

Merge branch 'main' into refactor-pattern-logic

4486939

Fixed _update_ds_configs

c667645

Signed-off-by: Elena Khaustova <[email protected]>

Fixed _init_datasets

be8e929

Signed-off-by: Elena Khaustova <[email protected]>

Implemented add_runtime_patterns

ec7ac39

Signed-off-by: Elena Khaustova <[email protected]>

Fixed runtime patterns usage

8e23450

Signed-off-by: Elena Khaustova <[email protected]>

Merge branch 'main' into refactor-pattern-logic

529e61a

Merge branch 'main' into refactor-pattern-logic

e4cb21c

Moved pattern logic out of data catalog, implemented KedroDataCatalog

50bc816

Signed-off-by: Elena Khaustova <[email protected]>

Merge branch 'main' into 4110-move-pattern-resolution-logic

6dfbcb0

KedroDataCatalog updates

9346f08

Signed-off-by: Elena Khaustova <[email protected]>

Added property to return config

9568e29

Signed-off-by: Elena Khaustova <[email protected]>

ElenaKhaustova added 2 commits September 12, 2024 23:00

Fixed docs

0833a84

Signed-off-by: Elena Khaustova <[email protected]>

Removed reference to DataCatalog in docstrings

0ec1f23

Signed-off-by: Elena Khaustova <[email protected]>

ElenaKhaustova changed the title ~~Protocol abstraction for DataCatalog~~ [DataCatalog2.0]: Protocol abstraction for DataCatalog Sep 13, 2024

ElenaKhaustova mentioned this pull request Sep 13, 2024

[DataCatalog2.0]: KedroDataCatalog #4151

Open

7 tasks

Removed add_all from protocol

741b682

Signed-off-by: Elena Khaustova <[email protected]>

ElenaKhaustova self-assigned this Sep 13, 2024

ElenaKhaustova marked this pull request as ready for review September 13, 2024 13:21

ElenaKhaustova requested review from yetudada, astrojuanlu and merelcht as code owners September 13, 2024 13:21

ElenaKhaustova requested review from idanov and ankatiyar September 13, 2024 13:21

ElenaKhaustova requested a review from noklam September 13, 2024 14:17

idanov approved these changes Sep 13, 2024

View reviewed changes

kedro/runner/runner.py Outdated Show resolved Hide resolved

noklam reviewed Sep 13, 2024

View reviewed changes

ElenaKhaustova added 2 commits September 13, 2024 17:04

Updated docstrings

78feb51

Signed-off-by: Elena Khaustova <[email protected]>

Updated docstrings

6bf912c

Signed-off-by: Elena Khaustova <[email protected]>

noklam reviewed Sep 13, 2024

View reviewed changes

noklam self-requested a review September 13, 2024 23:56

noklam approved these changes Sep 13, 2024

View reviewed changes

merelcht reviewed Sep 16, 2024

View reviewed changes

kedro/io/core.py Show resolved Hide resolved

kedro/io/core.py Outdated Show resolved Hide resolved

kedro/io/core.py Show resolved Hide resolved

ElenaKhaustova added 2 commits September 16, 2024 16:30

Fixed docstrings

c9c7c9a

Signed-off-by: Elena Khaustova <[email protected]>

Merge branch 'main' into 4138-catalog-protocol

c66df33

merelcht approved these changes Sep 17, 2024

View reviewed changes

Updated RELEASE.md

4d92c33

Signed-off-by: Elena Khaustova <[email protected]>

ElenaKhaustova enabled auto-merge (squash) September 17, 2024 14:48

ElenaKhaustova merged commit 6bf29f9 into main Sep 17, 2024
41 checks passed

ElenaKhaustova deleted the 4138-catalog-protocol branch September 17, 2024 15:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DataCatalog2.0]: `Protocol` abstraction for `DataCatalog` #4160

[DataCatalog2.0]: `Protocol` abstraction for `DataCatalog` #4160

ElenaKhaustova commented Sep 12, 2024 •

edited

Loading

datajoely commented Sep 13, 2024

idanov left a comment

noklam left a comment

noklam Sep 13, 2024

ElenaKhaustova Sep 13, 2024

ElenaKhaustova commented Sep 13, 2024 •

edited

Loading

noklam left a comment

noklam left a comment

merelcht left a comment

merelcht left a comment

[DataCatalog2.0]: Protocol abstraction for DataCatalog #4160

[DataCatalog2.0]: Protocol abstraction for DataCatalog #4160

Conversation

ElenaKhaustova commented Sep 12, 2024 • edited Loading

Description

Development notes

Developer Certificate of Origin

Checklist

datajoely commented Sep 13, 2024

idanov left a comment

Choose a reason for hiding this comment

noklam left a comment

Choose a reason for hiding this comment

noklam Sep 13, 2024

Choose a reason for hiding this comment

ElenaKhaustova Sep 13, 2024

Choose a reason for hiding this comment

ElenaKhaustova commented Sep 13, 2024 • edited Loading

noklam left a comment

Choose a reason for hiding this comment

noklam left a comment

Choose a reason for hiding this comment

merelcht left a comment

Choose a reason for hiding this comment

merelcht left a comment

Choose a reason for hiding this comment

[DataCatalog2.0]: `Protocol` abstraction for `DataCatalog` #4160

[DataCatalog2.0]: `Protocol` abstraction for `DataCatalog` #4160

ElenaKhaustova commented Sep 12, 2024 •

edited

Loading

ElenaKhaustova commented Sep 13, 2024 •

edited

Loading