Improve `add_feed_dict` docstrings #4009

ElenaKhaustova · 2024-07-15T14:53:39Z

Description

Development notes

Updated the docstrings for DataCatalog's add_feed_dict method and extended them with an additional example.
Created form_dict alias for add_feed_dict method and added a note to the docstring that the method will be renamed in the 0.20.0 release.

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

ElenaKhaustova · 2024-07-15T16:27:53Z

I would also suggest replacing io with catalog in the rest of the DataCatalog docstrings, as the latter feels more intuitive. But in a separate PR.

astrojuanlu · 2024-07-16T06:15:02Z

kedro/io/data_catalog.py

@@ -681,29 +681,41 @@ def add_all(
            self.add(name, dataset, replace)

    def add_feed_dict(self, feed_dict: dict[str, Any], replace: bool = False) -> None:
-        """Adds instances of ``MemoryDataset``, containing the data provided
-        through feed_dict.
+        """This function adds datasets to the ``DataCatalog`` using the data provided through the `feed_dict`.


I don't think we have a style guide for docstrings yet? In any case, following Pydocstyle D404 and D401,

Suggested change

"""This function adds datasets to the ``DataCatalog`` using the data provided through the `feed_dict`.

"""Add datasets to the ``DataCatalog`` using the data provided through the `feed_dict`.

Add additional ?

Suggested change

"""This function adds datasets to the ``DataCatalog`` using the data provided through the `feed_dict`.

"""Add additional datasets to the ``DataCatalog`` using the data provided through the `feed_dict`.

I didn't fully understand why we need additional here, as we can add them to the empty catalog as well. So, it looks like we do not need to specify any adding conditions.

astrojuanlu · 2024-07-16T06:18:40Z

kedro/io/data_catalog.py

@@ -713,6 +725,9 @@ def add_feed_dict(self, feed_dict: dict[str, Any], replace: bool = False) -> Non

            self.add(dataset_name, dataset, replace)

+    # Alias for add_feed_dict method
+    from_dict = add_feed_dict


Pedantic nitpick, but when I read from_dict I assume it's a classmethod, like

class DataCatalog: @classmethod def from_dict(cls, data_dict: dict[str, AbstractDataset]): ... return cls

So not sure if I'd add this alias.

(I know this comes from #3612 (comment) and that I asserted, but my response there is incomplete)

This is actually quite a significant change. I think we should discuss first what a good alternative is, before just adding an alias.

This has always been an undiscovered part of the API, I agree with @merelcht that we should think really hard / research what a better name should be.

I removed an alias until we are sure about adding it and agree on its name.

To @astrojuanlu's point, if we decide to keep an alias, we can use the add_from_dict name instead.

Or, as an alternative, we can add a new implementation (add_from_dict / add_datasets) and use it inside add_feed_dict with a deprecation warning.

astrojuanlu · 2024-07-16T06:20:58Z

kedro/io/data_catalog.py

+        `feed_dict` dictionary key is used as a dataset name, and a value is used to create an instance of
+        ``MemoryDataset`` before adding to the ``DataCatalog`` for all the value types except of ``AbstractDataset``.
+        In the last case, the ``AbstractDataset`` is added as it is.


What about

Suggested change

`feed_dict` dictionary key is used as a dataset name, and a value is used to create an instance of

``MemoryDataset`` before adding to the ``DataCatalog`` for all the value types except of ``AbstractDataset``.

In the last case, the ``AbstractDataset`` is added as it is.

`feed_dict` keys are used as dataset names, and values can either be raw data or instances of ``AbstractDataset``.

In the former case, instances of ``MemoryDataset`` are automatically created before adding to the ``DataCatalog``.

?

I actually don't think mentioning AbstractDataset is useful here, we shouldn't really have instances of an interface right? Does the user gain anything for understanding this implementation detail?

That's a good point. I rephrased it, but I still think it's useful to distinguish between the two cases when adding raw data and a dataset.

merelcht · 2024-07-16T08:56:29Z

kedro/io/data_catalog.py

@@ -713,6 +725,9 @@ def add_feed_dict(self, feed_dict: dict[str, Any], replace: bool = False) -> Non

            self.add(dataset_name, dataset, replace)

+    # Alias for add_feed_dict method
+    from_dict = add_feed_dict


This is actually quite a significant change. I think we should discuss first what a good alternative is, before just adding an alias.

kedro/io/data_catalog.py

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

astrojuanlu

LGTM!

ankatiyar

much clearer!

merelcht

LGTM! ⭐

ElenaKhaustova added 4 commits July 15, 2024 15:23

Updated docstring text

cbd73ee

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

Updated example

0e6e60b

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

Added alias

d0420f1

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

Merge branch 'main' into feature/3612-add-feed-dict-docstrings

232b1b1

ElenaKhaustova marked this pull request as ready for review July 15, 2024 16:59

ElenaKhaustova requested a review from merelcht as a code owner July 15, 2024 16:59

ElenaKhaustova requested review from astrojuanlu, datajoely and ankatiyar July 15, 2024 16:59

astrojuanlu reviewed Jul 16, 2024

View reviewed changes

merelcht reviewed Jul 16, 2024

View reviewed changes

ElenaKhaustova added 3 commits July 16, 2024 10:26

Removed alias

ee563c1

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

Applied suggested changes

dada3d4

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

Updated docstring based on comments

390dfd1

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

astrojuanlu approved these changes Jul 16, 2024

View reviewed changes

ankatiyar approved these changes Jul 16, 2024

View reviewed changes

merelcht approved these changes Jul 17, 2024

View reviewed changes

Merge branch 'main' into feature/3612-add-feed-dict-docstrings

da0eda8

ElenaKhaustova enabled auto-merge (squash) July 17, 2024 15:45

ElenaKhaustova merged commit ea54d60 into main Jul 17, 2024
34 checks passed

ElenaKhaustova deleted the feature/3612-add-feed-dict-docstrings branch July 17, 2024 16:04

ElenaKhaustova mentioned this pull request Jul 19, 2024

Improve docstring for add_feed_dict #3612

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve `add_feed_dict` docstrings #4009

Improve `add_feed_dict` docstrings #4009

ElenaKhaustova commented Jul 15, 2024 •

edited

Loading

ElenaKhaustova commented Jul 15, 2024 •

edited

Loading

astrojuanlu Jul 16, 2024

datajoely Jul 16, 2024

ElenaKhaustova Jul 16, 2024

astrojuanlu Jul 16, 2024

merelcht Jul 16, 2024

datajoely Jul 16, 2024

ElenaKhaustova Jul 16, 2024

astrojuanlu Jul 16, 2024

datajoely Jul 16, 2024

ElenaKhaustova Jul 16, 2024

merelcht Jul 16, 2024

astrojuanlu left a comment

ankatiyar left a comment

merelcht left a comment

	"""This function adds datasets to the ``DataCatalog`` using the data provided through the `feed_dict`.
	"""Add datasets to the ``DataCatalog`` using the data provided through the `feed_dict`.

	"""This function adds datasets to the ``DataCatalog`` using the data provided through the `feed_dict`.
	"""Add additional datasets to the ``DataCatalog`` using the data provided through the `feed_dict`.

Improve add_feed_dict docstrings #4009

Improve add_feed_dict docstrings #4009

Conversation

ElenaKhaustova commented Jul 15, 2024 • edited Loading

Description

Development notes

Developer Certificate of Origin

Checklist

ElenaKhaustova commented Jul 15, 2024 • edited Loading

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

astrojuanlu left a comment

Choose a reason for hiding this comment

ankatiyar left a comment

Choose a reason for hiding this comment

merelcht left a comment

Choose a reason for hiding this comment

Improve `add_feed_dict` docstrings #4009

Improve `add_feed_dict` docstrings #4009

ElenaKhaustova commented Jul 15, 2024 •

edited

Loading

ElenaKhaustova commented Jul 15, 2024 •

edited

Loading