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

[npfp]

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]

There might be a better way to do this.

[npfp]

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)

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

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]

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]

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]

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 :-) )