feat: Dataset API add save method

[McDonnellJoseph]

Description

We sometimes want to save some data with a REST Api (say with Django Rest Framework), most of the time issuing a POST request. So far, the APIDataSet is read only, with the _save method raising a DataSetError, we'd want to extend it to make a request.

Closes #166

Development notes

If applied, this commit will:

1. Enable save method for APIDataSet, by sending packets of size chunk_size, default is 100 to server. Default HTTP method is POST and returns a request.Response object.
2. Add, save_args dictionary to start conforming with other datasets.
3. Raise error when trying to save non-json data.
4. Raise error for save in case of HTTP error.

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

[McDonnellJoseph]

Hello, do you know if there is documentation that needs to be updated apart from the docstring of the class ?

[astrojuanlu]

Hello @McDonnellJoseph, thanks for this contribution! In this case "documentation" only refers to the docstrings, yes.

The CI is failing because some lines are not covered by tests:

kedro_datasets/api/api_dataset.py                          67      4    94%   181-185

[McDonnellJoseph]

Thanks for pointing this out, I just added full test coverage !

[McDonnellJoseph]

I haven't changed any code since last commit which passed CI.
The returned error is "ERROR: Could not build wheels for import-linter, which is required to install pyproject.toml-based projects"
but I don't see what change in my code to cause such error.

[McDonnellJoseph]

Otherwise I think this is ready for review

[astrojuanlu]

@McDonnellJoseph it's an intermittent issue with unknown root cause (see kedro-org/kedro#2570), I restarted it but I'm confident the tests will pass.

[McDonnellJoseph]

@astrojuanlu Yes that's what I thought but I don't think contributors can restart the tests. Anyway, I think the code is good for review now, got full test coverage + some better documentation for the new save feature and how it works.

[noklam]

Thank you for the PR, it looks pretty solid to me! Just thinking top of my head, does anyone ever use this for POST request? It's seem possible but I find it a bit weird. Maybe the right thing to do is GET for load and PUT DELETE POST for save?

[noklam]

I think it makes sense to only enable GET for load and POST PUT for save. In fact we have already refactor some of the arguments in #181 (It was done in kedro develop but we forgot to sync it).

So we should have load_args and save_args, which make the dataset more consistent with other datasets too.

[noklam]

Addition - I would suggest to do the refactoring after kedro-org/kedro#2570 is merged as it changes the argument a bit.

[McDonnellJoseph]

Addition - I would suggest to do the refactoring after kedro-org/kedro#2570 is merged as it changes the argument a bit.

Hello, maybe I misunderstood but don't you mean pr #184 ?

[noklam]

@McDonnellJoseph Sorry about that, you are right! #184 is merged now, please update this branch. There are quite are few changes because of the change in arguments. Let me know if you get stuck.

[McDonnellJoseph]

Hello again, I added your suggested modifications to the code and I also merged the recent modifications made to the APIDataSet with my work.
Looking forward to hearing from you

[McDonnellJoseph]

Hello, I'm still waiting for a review and would be happy to make a contribution to the project. Is something missing in the PR to get a review ?

[merelcht]

Thanks for the contribution @McDonnellJoseph ! I've left some comments. I'll be happy to approve when those are addressed 🙂

[noklam]

@McDonnellJoseph Thanks for persevering on this. I missed the Github notification, feel free to @me or find me on Slack if I haven't responded for a long time.

[McDonnellJoseph]

Hello @noklam @merelcht thank you for your comments I have addressed them in the latest version. I spent a lot of time this morning trying to rebase my branch to sign an old commit and this has generated a new problem meaning I can't pass the pipeline. Is this an easy fix for you ?

[astrojuanlu]

Looks like unsigned commit is 07eb097, which is already in main. @McDonnellJoseph let me have a quick locally at what's the right git spell to run to perform this rebase correctly and I'll post it here.

[McDonnellJoseph]

Looks like unsigned commit is 07eb097, which is already in main. @McDonnellJoseph let me have a quick locally at what's the right git spell to run to perform this rebase correctly and I'll post it here.

Thanks ! I got lost with the rebase which is the reason behind the large amount of commits for the PR

[McDonnellJoseph]

Git magic

The conflicts here are quire hairy. Initially I managed to at least linearize the history by doing

git rebase -i 3b42fae --rebase-merges

and using this rebase-todo:

label onto

# Branch main
reset onto
label main

reset onto
pick 5d374cd [FEAT] add save method to APIDataset
pick 09e627e [ENH] create save_args parameter for api_dataset
pick f6147a0 [ENH] add tests for socket + http errors
pick 9580c38 [ENH] check save data is json
pick 440d249 [FIX] clean code
pick 1ae242c [ENH] handle different data types
pick ec8af04 [FIX] test coverage for exceptions
pick cb51b97 [ENH] add examples in APIDataSet docstring
fixup aa7c1fc make git happy'
pick 07eb097 sync APIDataSet  from kedro's `develop` (#184)
pick 3977607 [FIX] remove support for delete method
fixup 7374b2d [FEAT] add save method to APIDataset
drop f90c20f [FEAT] add save method to APIDataset
pick 318b90e [FIX] lint files
drop a9246d1 [FIX] fix conflicts
drop cc5a368 [FEAT] add save method to APIDataset
drop 30dfc59 [FIX] remove support for delete method
drop 9654232 [FEAT] add save method to APIDataset
pick af518cf [FIX] fix conflicts  # remove _convert_type
drop 58d217c [FIX] lint files # empty
pick d99d28c [FIX] remove fail save test
pick dab7584 [ENH] review suggestions
merge -C cf25924 main # Merge branch 'main' into dataset-api-add_save_method
pick 2ade519 [ENH] fix tests

# Rebase 3b42fae..2ade519 onto 3b42fae (41 commands)
#
# Commands:
# p, pick <commit> = use commit
# r, reword <commit> = use commit, but edit the commit message
# e, edit <commit> = use commit, but stop for amending
# s, squash <commit> = use commit, but meld into previous commit
# f, fixup [-C | -c] <commit> = like "squash" but keep only the previous
#                    commit's log message, unless -C is used, in which case
#                    keep only this commit's message; -c is same as -C but
#                    opens the editor
# x, exec <command> = run command (the rest of the line) using shell
# b, break = stop here (continue rebase later with 'git rebase --continue')
# d, drop <commit> = remove commit
# l, label <label> = label current HEAD with a name
# t, reset <label> = reset HEAD to a label
# m, merge [-C <commit> | -c <commit>] <label> [# <oneline>]
#         create a merge commit using the original merge commit's
#         message (or the oneline, if no original merge commit was
#         specified); use -c <commit> to reword the commit message
# u, update-ref <ref> = track a placeholder for the <ref> to be updated
#                       to this position in the new commits. The <ref> is
#                       updated at the end of the rebase
#
# These lines can be re-ordered; they are executed from top to bottom.
#
# If you remove a line here THAT COMMIT WILL BE LOST.
#
# However, if you remove everything, the rebase will be aborted.
#

After that, most of the repeated commits are dropped. However, doing a git rebase main or git merge main after that yields a lot of conflicts. I'm not sure if it's worth going through the hassle of reviewing them or if it's better to squash the result of the first interactive into one commit and just push that.

Git magic worked ! Thanks a lot, won't the commits be squashed once the branch is merged ?

[astrojuanlu]

👽 git magic is indeed miraculous!

Yes, the commits will be squashed on merge. But to achieve that, even with this nice linearized history, first you'll need to fix the current conflicts.

[astrojuanlu]

Well done @McDonnellJoseph, thanks for persevering! 🙌🏽 I think this is ready for another round of reviews.

[noklam]

Thank you, the PR is looking better now.

If the DCO fails, I suggested just leave it until everything is reviewed, as this is a little bit hard to review now since all previous review comments are gone.

In general it's good to have documentation consistent with other dataset, I would suggest to have a look at the popular one (i.e. CSVDataSet).

[noklam]

Thank you, the PR is looking better now.

If the DCO fails, I suggested just leave it until everything is reviewed, as this is a little bit hard to review now since all previous review comments are gone.

In general it's good to have documentation consistent with other dataset, I would suggest to have a look at the popular one (i.e. CSVDataSet).

[McDonnellJoseph]

Thank you, the PR is looking better now.

If the DCO fails, I suggested just leave it until everything is reviewed, as this is a little bit hard to review now since all previous review comments are gone.

In general it's good to have documentation consistent with other dataset, I would suggest to have a look at the popular one (i.e. CSVDataSet).

Do you think there is somewhere more appropriate in the file for the explanation paragraph on how the save method works ? Or should I just dump it altogether ? @noklam

[noklam]

I think it's fine to leave it within the docstring, @stichbury for comments

[astrojuanlu]

Well done @McDonnellJoseph!

[astrojuanlu]

Congrats @McDonnellJoseph and thank you for your contribution! 🙌🏽