Catalog to config #4323

ElenaKhaustova · 2024-11-12T16:45:03Z

Description

Made on top of #4347 (review it first)

Implementation of #4329

TODO:

Exclude parameters
Add docstrings and release notes
Open related tickets:
- Discrepancy between setting save_version via catalog constructor and when passing datasets #4327, PR: Validate datasets versions #4347
- Inconsistency when setting version via versioned flag and dataset parameter #4326
- tickets for datasets required extending parent AbstractDataset.to_config(): kedro-datasets: Datasets accept non-primitive parameters in the __init__ kedro-plugins#950

Development notes

To run pytest tests/io/test_kedro_data_catalog.py::TestKedroDataCatalog::TestKedroDataCatalogFromConfig or see an example from How to test section in #4329

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]>

…catalog-from-to-prototype

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

ElenaKhaustova · 2024-11-27T14:36:44Z

Is it worth splitting the version validation PR separately? Or is it tied to the "catalog to config" prototype in any way?

#4347

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

…catalog-from-to-prototype

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

merelcht

Thanks @ElenaKhaustova, really great work! 🌟

merelcht · 2024-11-28T09:46:14Z

kedro/framework/cli/catalog.py

@@ -28,6 +28,7 @@ def _create_session(package_name: str, **kwargs: Any) -> KedroSession:


 def is_parameter(dataset_name: str) -> bool:
+    # TODO: when breaking change move it to kedro/io/core.py


Is this todo still needed?

merelcht · 2024-11-28T09:54:36Z

kedro/io/kedro_data_catalog.py

+        """Converts the `KedroDataCatalog` instance into a configuration format suitable for
+        serialization. This includes datasets, credentials, and versioning information.
+
+        This method is only applicabe to catalogs that contain datasets initialized with static, primitive


Suggested change

This method is only applicabe to catalogs that contain datasets initialized with static, primitive

This method is only applicable to catalogs that contain datasets initialized with static, primitive

idanov · 2024-11-20T10:29:49Z

kedro/io/catalog_config_resolver.py

@@ -237,8 +237,9 @@ def _extract_patterns(

        return sorted_patterns, user_default

+    @classmethod
    def _resolve_config_credentials(


Can we drop the word config from the name of the method as well as the method below? What other credentials can they be except config credentials in the context of this class?

idanov · 2024-11-28T10:35:16Z

kedro/io/core.py

+    # Declares a class-level attribute that will store the initialization
+    # arguments of an instance. Initially, it is set to None, but it will
+    # hold a dictionary of arguments after initialization.
+    _init_args: dict[str, Any] | None = None


Isn't this shared across all instances? How to make sure we're not overwriting everything with the latest instance args?

idanov · 2024-11-28T10:45:07Z

kedro/io/core.py

+            method of the instance is called with the arguments used to initialize
+            the object.
+            """
+


Suggested change

@wraps(init_func)

and you need to import it at the top as from functools import wraps

idanov · 2024-11-28T10:47:59Z

kedro/io/core.py

+        # Save the original __init__ method of the subclass
+        init_func: Callable = cls.__init__
+
+        def init_decorator(previous_init: Callable) -> Callable:


I believe you can drop the padding of an extra function, this decorator will not be used in the code as a decorator, but rather you will just assign the function it returns to be the cls.__init__, so only the new_init function is required.

idanov · 2024-11-28T10:49:13Z

kedro/io/core.py

+                """
+
+                # Call the original __init__ method
+                previous_init(self, *args, **kwargs)


Suggested change

previous_init(self, *args, **kwargs)

init_func(self, *args, **kwargs)

idanov · 2024-11-28T10:51:01Z

kedro/io/core.py

+                if type(self) is cls:
+                    # Capture and process the arguments passed to the original __init__
+                    call_args = getcallargs(init_func, self, *args, **kwargs)
+                    # Call the custom post-initialization method to save captured arguments


I believe this might not be needed, if you setup the function as suggested.

idanov · 2024-11-28T10:51:39Z

kedro/io/core.py

+    # hold a dictionary of arguments after initialization.
+    _init_args: dict[str, Any] | None = None
+
+    def __post_init__(self, call_args: dict[str, Any]) -> None:


Suggested change

def __post_init__(self, call_args: dict[str, Any]) -> None:

def __post_init__(self, *args, **kwargs) -> None:

Btw, do we even need this method at all? Can't all of this be done by the decorator itself instead of delegating it to a separate method?

idanov · 2024-11-28T10:53:49Z

kedro/io/core.py

@@ -484,14 +602,14 @@ def parse_dataset_definition(
    config = copy.deepcopy(config)

    # TODO: remove when removing old catalog as moved to KedroDataCatalog
-    if "type" not in config:
+    if TYPE_KEY not in config:


idanov · 2024-11-28T10:55:02Z

kedro/io/kedro_data_catalog.py

+        load_versions: dict[str, str | None] = {}
+
+        for ds_name, ds in self._lazy_datasets.items():
+            if _is_memory_dataset(ds.config.get("type", "")):


Suggested change

if _is_memory_dataset(ds.config.get("type", "")):

if _is_memory_dataset(ds.config.get(TYPE_KEY, "")):

...maybe?

idanov · 2024-11-28T10:57:47Z

kedro/io/core.py

+
+def _is_memory_dataset(ds_or_type: AbstractDataset | str) -> bool:
+    """Check if dataset or str type provided is a MemoryDataset."""
+    if isinstance(ds_or_type, AbstractDataset):


Can't we check directly for a MemoryDataset instance here or this is to avoid circular dependencies?

ElenaKhaustova added 21 commits November 7, 2024 12:26

Captured init arguments

ae5384a

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

Implemented unresoloving credentials

fcdf357

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

Added some comments

b147374

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

Put type in first place for dataset config

0ed0c1e

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

Handled version key

3c839a9

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

Added lazy dataset to_config

29bb714

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

Removed data key from MemoryDataset

49858b6

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

Added TODOs

0d0ba91

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

Saved call args

8413f58

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

Saved only set credentials

a89db7e

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

Processed CachedDataset case

b081c65

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

Updated TODOs

18c0ad6

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

Tested with PartitionedDataset

2751ea8

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

Popped metadata

fc576ff

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

Fixed versioning when load

1d6454c

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

Fixed linter

8c31237

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

Tested datasets factories

e035881

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

Tested transcoding

edcdc38

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

Removed TODOs

15a1e72

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

Removed debug output

d4e4534

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

Removed debug output

e7e8af5

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

ElenaKhaustova changed the title ~~Catalog from/to dict prototype~~ Catalog to config prototype Nov 13, 2024

ElenaKhaustova mentioned this pull request Nov 13, 2024

[DataCatalog]: Catalog to config #4329

Open

ElenaKhaustova added 7 commits November 14, 2024 11:50

Added logic to set VERSIONED_FLAG_KEY

1b6be8e

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

Merge branch 'main' into feature/3932-catalog-from-to-prototype

db77436

Updated version set up

c6dc380

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

Added TODO for versioning

54b0793

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

Added tests for unresolve_config_credentials

f183e60

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

Implemented test_to_config

0d9d241

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

Added test with MemoryDataset

763e635

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

ElenaKhaustova added 9 commits November 27, 2024 10:13

Updated release notes

ca2ac6c

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

Added tests for CachedDataset use case

615d135

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

Merge branch 'fix/4327-validate-datasets-versions' into feature/3932-…

8dd1084

…catalog-from-to-prototype

Updated unit test after version validation is applied

8a01881

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

Removed MemoryDatasets

eb44a30

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

Removed _is_parameter

ba3d04e

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

Pop metadata from cached dataset configuration

35953a9

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

Fixed lint

d56793b

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

Fixed unit test

ebf1483

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

ElenaKhaustova marked this pull request as draft November 27, 2024 15:47

ElenaKhaustova added 9 commits November 27, 2024 15:55

Added docstrings for AbstractDataset.to_config()

f5468c9

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

Updated docstrings

edee597

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

Fixed typos

4454970

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

Updated TODOs

5d6bd3c

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

Merge branch 'main' into fix/4327-validate-datasets-versions

f1ace7c

Merge branch 'fix/4327-validate-datasets-versions' into feature/3932-…

95fc260

…catalog-from-to-prototype

Added docstring for KedroDataCatalog.to_config

86e25e9

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

Added docstrinbgs for unresolve_credentials

c8fd99e

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

Updated release notes

5b2f21f

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

ElenaKhaustova changed the title ~~Catalog to config prototype~~ Catalog to_config() Nov 27, 2024

ElenaKhaustova changed the title ~~Catalog to_config()~~ Catalog to config Nov 27, 2024

ElenaKhaustova added 4 commits November 27, 2024 19:34

Fixed indentation

35dc102

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

Fixed to_config() example

2853fda

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

Fixed indentation

8f0fe4f

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

Fixed indentation

0db9b46

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

ElenaKhaustova marked this pull request as ready for review November 27, 2024 20:36

Added a note about to_config() constraints

2f72e23

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

merelcht approved these changes Nov 28, 2024

View reviewed changes

idanov reviewed Nov 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Catalog to config #4323

Catalog to config #4323

ElenaKhaustova commented Nov 12, 2024 •

edited

Loading

ElenaKhaustova commented Nov 27, 2024

merelcht left a comment

merelcht Nov 28, 2024

merelcht Nov 28, 2024

idanov Nov 20, 2024

idanov Nov 28, 2024

idanov Nov 28, 2024

idanov Nov 28, 2024

idanov Nov 28, 2024

idanov Nov 28, 2024

idanov Nov 28, 2024

idanov Nov 28, 2024

idanov Nov 28, 2024

idanov Nov 28, 2024

		@@ -28,6 +28,7 @@ def _create_session(package_name: str, **kwargs: Any) -> KedroSession:


		def is_parameter(dataset_name: str) -> bool:
		# TODO: when breaking change move it to kedro/io/core.py

	This method is only applicabe to catalogs that contain datasets initialized with static, primitive
	This method is only applicable to catalogs that contain datasets initialized with static, primitive

	previous_init(self, args, *kwargs)
	init_func(self, args, *kwargs)

	def __post_init__(self, call_args: dict[str, Any]) -> None:
	def __post_init__(self, args, *kwargs) -> None:

	if _is_memory_dataset(ds.config.get("type", "")):
	if _is_memory_dataset(ds.config.get(TYPE_KEY, "")):

Catalog to config #4323

Are you sure you want to change the base?

Catalog to config #4323

Conversation

ElenaKhaustova commented Nov 12, 2024 • edited Loading

Description

TODO:

Development notes

Developer Certificate of Origin

Checklist

ElenaKhaustova commented Nov 27, 2024

merelcht left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElenaKhaustova commented Nov 12, 2024 •

edited

Loading