feat(datasets): add wrapped dataset to store the response from a POST/PUT request #905

npfp · 2024-10-23T12:44:23Z

Proof of concept to start discussions about design on how to store and use the response object.

Description

To pursue discussions on how we could store the response object from a POST or PUT api request.

Relates to #748

Development notes

Only poc code
Tests to be implemented

Checklist

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 relevant RELEASE.md file
Added tests to cover my changes
Received approvals from at least half of the TSC (required for adding a new, non-experimental dataset)

npfp · 2024-10-23T12:46:51Z

kedro-datasets/kedro_datasets/api/api_dataset.py

+    @property
+    def _nested_dataset_type(
+        self,
+    ) -> Type[JSONDataset | PickleDataset | MemoryDataset]:


There might be a better way to do this.

npfp · 2024-10-23T12:50:04Z

kedro-datasets/kedro_datasets/api/api_dataset.py

+            if self._extension == "json":
+                self.wrapped_dataset.save(response.json()) #TODO(npfp): expose json loads arguments
+            elif self._extension == "text":
+                self.wrapped_dataset.save(response.text)
+            elif self._extension:
+                self.wrapped_dataset.save(response)


Might be better to check on the self.wrapped_dataset type:

Suggested change

if self._extension == "json":

self.wrapped_dataset.save(response.json()) #TODO(npfp): expose json loads arguments

elif self._extension == "text":

self.wrapped_dataset.save(response.text)

elif self._extension:

self.wrapped_dataset.save(response)

elif isinstance(self.wrapped_dataset, JSONDataset):

self.wrapped_dataset.save(response.json()) #TODO(npfp): expose json loads arguments

elif isinstance(self.wrapped_dataset, TextDataset)

self.wrapped_dataset.save(response.text)

elif self._extension:

self.wrapped_dataset.save(response)

…/PUT request Proof of concept to start discussions about design on how to store and use response. Signed-off-by: Nicolas Perrin <[email protected]>

astrojuanlu

Hi @npfp , sorry for the very long delay here. Thanks a lot for sending this pull request.

Is your idea to make APIDataset wrap another dataset? (Similar to what you mentioned in #748 (comment))

I'm assuming that this is a breaking change on the API dataset right?

npfp · 2025-01-14T18:19:59Z

No worries. Thx for getting back to me!

Yes, I chose the wrapped dataset mainly because it would avoid maintenance burden by relying on the implementation of the wrapped dataset rather than duplicating the logic there.

Of course, it's a subjective opinion, so feel free to challenge this, if it doesn't follow kedro's guidelines.

npfp · 2025-01-14T18:21:37Z

I don't think it's a breaking change of the API dataset: the wrapped dataset args are optionals.

If not provided, the response is not saved as it's already the case.

merelcht · 2025-01-16T13:33:14Z

It's been a while since I looked at the issue this PR is for, but if I understand correctly the problem with APIDataset is that it only returns the response, but doesn't actually save it? And this PR aims to add a wrapper so a user can indicate what format to save the response in?

On the original issue I said I didn't like the idea of importing other datasets, which I still agree with, but at the same time it does make sense now I see it in code 😜 I'll have to think about this a bit more to review it properly.

npfp · 2025-01-17T17:57:54Z

It's been a while since I looked at the issue this PR is for, but if I understand correctly the problem with APIDataset is that it only returns the response, but doesn't actually save it? And this PR aims to add a wrapper so a user can indicate what format to save the response in?

On the original issue I said I didn't like the idea of importing other datasets, which I still agree with, but at the same time it does make sense now I see it in code 😜 I'll have to think about this a bit more to review it properly.

Yes that's right:

some APIs would return a response that might be important to store somewhere,
yes exactly.

Good to know, thx! (I'm a bit biased because we almost always use wrapped dataset for our custom datasets: we're lazy and like relying on a maintained classe :-) )

merelcht

On second review I think this solution is a pretty good solution. I have left some comments and asked for a second opinion. If we all agree on approach you can polish this and we can do the proper review process 🙂

merelcht · 2025-02-03T17:10:47Z

kedro-datasets/kedro_datasets/api/api_dataset.py

+        self._extension = extension
+        self._wrapped_dataset_args = wrapped_dataset
+        self._wrapped_dataset = None


I'm a little confused on the need and difference between all these arguments. Can you explain them? Would it make sense to rename the wrapped_dataset argument in __init__ to wrapped_dataset_args?

Ah yes, you're right the naming is confusing. I'm changing this.

merelcht · 2025-02-03T17:11:24Z

kedro-datasets/kedro_datasets/api/api_dataset.py

+        """The wrapped dataset where response data is stored."""
+        if self._wrapped_dataset is None and self._extension is not None:
+            self._wrapped_dataset = self._nested_dataset_type(
+                **self._wrapped_dataset_args


How is **self._wrapped_dataset_args actually used in _nested_dataset_type?

I'm not sure if I understood your question, but as _nested_dataset_type returns a dataset class,

self._nested_dataset_type( **self._wrapped_dataset_args )

will create the instance of the wrapped dataset:

Also I should change the name of the property _nested_dataset_type to _wrapped_dataset_type to be consistent...

ElenaKhaustova · 2025-02-04T19:03:20Z

The idea of having such a dataset makes sense to me. Implementation-wise, I would expect to see something similar to https://docs.kedro.org/projects/kedro-datasets/en/kedro-datasets-6.0.0/_modules/kedro_datasets/partitions/partitioned_dataset.html#PartitionedDataset, so logic of requesting data is separated from saving it via dataset and the extension is derived from dataset set rather than vice a versa.

npfp · 2025-02-13T08:06:05Z

kedro-datasets/kedro_datasets/api/api_dataset.py

+            else:
+                response: requests.Response = self._execute_save_request(json_data=data)
+
+            if self._wrapped_dataset is None:


Suggested change

if self._wrapped_dataset is None:

if self.wrapped_dataset is None:

npfp marked this pull request as draft October 23, 2024 12:44

npfp mentioned this pull request Oct 23, 2024

Make the APIDataset.save stores the response received #748

Open

npfp commented Oct 23, 2024

View reviewed changes

npfp force-pushed the feature/save-response-in-api-dataset branch from 2978b3c to dc433d1 Compare October 23, 2024 12:54

npfp changed the title ~~feat(api_dataset): add wrapped dataset to store the response from a POST/PUT request~~ feat(dataset): add wrapped dataset to store the response from a POST/PUT request Oct 23, 2024

npfp changed the title ~~feat(dataset): add wrapped dataset to store the response from a POST/PUT request~~ feat(datasets): add wrapped dataset to store the response from a POST/PUT request Oct 23, 2024

feat(datasets): add wrapped dataset to store the response from a POST…

e9980af

…/PUT request Proof of concept to start discussions about design on how to store and use response. Signed-off-by: Nicolas Perrin <[email protected]>

npfp force-pushed the feature/save-response-in-api-dataset branch from dc433d1 to e9980af Compare October 23, 2024 13:14

astrojuanlu reviewed Jan 14, 2025

View reviewed changes

merelcht reviewed Feb 4, 2025

View reviewed changes

ElenaKhaustova self-requested a review February 4, 2025 19:03

npfp commented Feb 13, 2025

View reviewed changes

npfp requested review from merelcht and astrojuanlu February 13, 2025 08:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(datasets): add wrapped dataset to store the response from a POST/PUT request #905

feat(datasets): add wrapped dataset to store the response from a POST/PUT request #905

npfp commented Oct 23, 2024

npfp Oct 23, 2024

npfp Oct 23, 2024

astrojuanlu left a comment

npfp commented Jan 14, 2025

npfp commented Jan 14, 2025

merelcht commented Jan 16, 2025

npfp commented Jan 17, 2025

merelcht left a comment

merelcht Feb 3, 2025

npfp Feb 13, 2025

merelcht Feb 3, 2025

npfp Feb 13, 2025

ElenaKhaustova commented Feb 4, 2025

npfp Feb 13, 2025

	if self._wrapped_dataset is None:
	if self.wrapped_dataset is None:

feat(datasets): add wrapped dataset to store the response from a POST/PUT request #905

Are you sure you want to change the base?

feat(datasets): add wrapped dataset to store the response from a POST/PUT request #905

Conversation

npfp commented Oct 23, 2024

Description

Development notes

Checklist

npfp Oct 23, 2024

Choose a reason for hiding this comment

npfp Oct 23, 2024

Choose a reason for hiding this comment

astrojuanlu left a comment

Choose a reason for hiding this comment

npfp commented Jan 14, 2025

npfp commented Jan 14, 2025

merelcht commented Jan 16, 2025

npfp commented Jan 17, 2025

merelcht left a comment

Choose a reason for hiding this comment

merelcht Feb 3, 2025

Choose a reason for hiding this comment

npfp Feb 13, 2025

Choose a reason for hiding this comment

merelcht Feb 3, 2025

Choose a reason for hiding this comment

npfp Feb 13, 2025

Choose a reason for hiding this comment

ElenaKhaustova commented Feb 4, 2025

npfp Feb 13, 2025

Choose a reason for hiding this comment