build(datasets): upgrade s3fs to newer calver #348

MatthiasRoels · 2023-09-26T17:18:33Z

Description

s3fs version is already 3+ years old. The aim of this PR is to upgrade it to their new calver versioning approach.

Fixes #337

Development notes

When s3fs switched to calver versioning, they switched from using botocore to aiobotocore, which is an async client for AWS services. As a result it became much more difficult to use moto for unit testing as many people faced issues like the one described here aio-libs/aiobotocore#755. To fix it, we had to switch to using moto in server mode.

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

Signed-off-by: Matthias Roels <matthias.roels21@gmail.com>

Since s3fs is switched to aiobotocore, tests started to fail with message `'MockRawResponse' object has no attribute 'raw_headers'`. This is a bug in aiobotocore. Using moto server does seem to resolve the issues Signed-off-by: Matthias Roels <matthias.roels21@gmail.com>

Signed-off-by: Matthias Roels <matthias.roels21@gmail.com>

noklam · 2023-09-26T17:41:39Z

It's interesting that you need moto-server to resolve the issue. Did you get some weird authentication error? This is something I encountered in the past, with newer moto version the issue seems to resolve itself automatically.

Does it fails on all version on specific Python Version?

MatthiasRoels · 2023-09-26T17:58:16Z

@noklam: No I didn't encounter any weird authn errors. If you don't use moto in server mode, you always run into these kind of errors:

AttributeError: 'AWSResponse' object has no attribute 'raw_headers'

For some reason, it occurs only in some test files. The tests for PandasCSVDataset run fine for example (although it's just one test involving S3). When I google the error, 2 fixes consistently appear; either you patch some boto function calls or you use moto in sever mode. The later looked like the most elegant one to fix the failing tests.

As a side note: I noticed there is a lot of inconsistency in the types of tests between different dataset classes. Is that intentional?

noklam · 2023-09-26T18:00:27Z

@MatthiasRoels that's fine, I have research this in the past I think server mode is the right way.

Signed-off-by: Matthias Roels <matthias.roels21@gmail.com>

sbrugman · 2023-09-29T10:33:20Z

kedro-datasets/tests/video/test_video_dataset.py

-            Key=S3_KEY_PATH + "/" + video_name,
-            Filename=str(tmp_file),
-        )
+        with open(tmp_file, "wb") as file:


nit: we could do tmp_file.open("wb") since it's already a pathlib object

I didn't knew that was possible :-). Do you want me to change it?

kedro-datasets/setup.py

noklam · 2023-09-29T11:19:31Z

kedro-datasets/tests/conftest.py

+def credentials():
+    return {
+        "key": "fake_access_key",
+        "secret": "fake_secret_key",
+    }


Why there are 2 sets of credentials?

Do we need key and secret or the AWS_XXXXX?

One is for credentials file, the other one are env vars. The env vars are required for bucket setup etc while the credentials are used in the Dataset classes.

Signed-off-by: Matthias Roels <matthias.roels21@gmail.com>

MatthiasRoels · 2023-10-12T06:40:42Z

Seems like the unit tests failed on (windows-latest, 3.8). Unfortunately, I have no access to a Windows machine for debugging and testing. And to be honest, I have never written software on a Windows OS before... So it would be nice if someone can help here. Thanks!

Signed-off-by: Matthias Roels <matthias.roels21@gmail.com> Co-authored-by: Nok Lam Chan <nok.lam.chan@quantumblack.com>

merelcht · 2023-10-25T17:15:23Z

@MatthiasRoels I've tried running this on Gitpod (ubuntu) and the tests also fail there, so it's not just a windows problem. You can also see that in the builds: https://github.com/kedro-org/kedro-plugins/actions/runs/6557080300/job/17809502926?pr=348 I guess it was just the windows 3.8 that failed first.

noklam · 2023-11-20T08:54:39Z

Update branch to see if the problem persist

Signed-off-by: Merel Theisen <49397448+merelcht@users.noreply.github.com>

Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>

…k tests Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>

Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>

Signed-off-by: Merel Theisen <49397448+merelcht@users.noreply.github.com>

Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>

…com:MatthiasRoels/kedro-plugins into build/datasets-upgrade-s3fs-to-newer-calver

Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu>

MatthiasRoels · 2023-11-30T19:38:01Z

Thanks @merelcht for taking over, I wanted to work on this for a while, but I’m in the middle of a cloud migration so I’m way to busy 🙁

merelcht · 2023-12-01T09:39:30Z

Hi @MatthiasRoels no worries. I've been trying to fix the issues described by people in aio-libs/aiobotocore#755. It's not straightforward though, so I'm using another branch to experiment and not pollute this PR too much.

merelcht · 2023-12-05T17:15:47Z

@MatthiasRoels I opened #463 where I've got most tests passing now and marked the two failing ones with @pytest.mark.xfail

merelcht · 2023-12-07T12:02:15Z

Thanks again for opening this PR @MatthiasRoels #463 is now merged, so I'll close this PR. After our 2.0.0 release on datasets we will review our mocking approach and perhaps we can use the server approach you tried.

MatthiasRoels added 3 commits September 21, 2023 15:41

build(datasets): upgrade to newer, CalVered s3fs

dcef71d

Signed-off-by: Matthias Roels <matthias.roels21@gmail.com>

fix(datasets): unit tests for dask pq dataset now leverage moto server

dd5e433

Signed-off-by: Matthias Roels <matthias.roels21@gmail.com>

MatthiasRoels changed the title ~~Build/datasets upgrade s3fs to newer calver~~ Build(datasets) upgrade s3fs to newer calver Sep 26, 2023

noklam self-assigned this Sep 26, 2023

noklam added the Community Issue/PR opened by the open-source community label Sep 26, 2023

MatthiasRoels added 3 commits September 27, 2023 07:46

build(datasets): bump moto version for server mode

73a5ad2

Signed-off-by: Matthias Roels <matthias.roels21@gmail.com>

fix(datasets): apply linting to test files

f08fce6

Signed-off-by: Matthias Roels <matthias.roels21@gmail.com>

fix(datasets): unit tests for video dataset now leverage moto server

5109fb5

Signed-off-by: Matthias Roels <matthias.roels21@gmail.com>

MatthiasRoels marked this pull request as ready for review September 27, 2023 08:02

sbrugman reviewed Sep 29, 2023

View reviewed changes

noklam reviewed Sep 29, 2023

View reviewed changes

MatthiasRoels changed the title ~~Build(datasets) upgrade s3fs to newer calver~~ build: kedro-datasets - upgrade s3fs to newer calver Oct 2, 2023

Fix(datasets): linting errors

a5bfd4c

Signed-off-by: Matthias Roels <matthias.roels21@gmail.com>

MatthiasRoels changed the title ~~build: kedro-datasets - upgrade s3fs to newer calver~~ build(datasets): upgrade s3fs to newer calver Oct 2, 2023

MatthiasRoels added 2 commits October 10, 2023 18:25

Merge branch 'main' into build/datasets-upgrade-s3fs-to-newer-calver

51859ad

Signed-off-by: Matthias Roels <matthias.roels21@gmail.com>

Merge branch 'main' into build/datasets-upgrade-s3fs-to-newer-calver

e578ad3

MatthiasRoels and others added 3 commits October 12, 2023 21:18

fix(datasets): set correct CalVer for s3fs in setup.py

308fe19

Signed-off-by: Matthias Roels <matthias.roels21@gmail.com> Co-authored-by: Nok Lam Chan <nok.lam.chan@quantumblack.com>

Merge branch 'main' into build/datasets-upgrade-s3fs-to-newer-calver

4fb1b02

Merge branch 'main' into build/datasets-upgrade-s3fs-to-newer-calver

3e43613

astrojuanlu mentioned this pull request Oct 30, 2023

Bump s3fs version number kedro-org/kedro#1995

Closed

Merge branch 'main' into build/datasets-upgrade-s3fs-to-newer-calver

31510e8

merelcht mentioned this pull request Nov 20, 2023

Release kedro-datasets 2.0.0 #421

Closed

4 tasks

merelcht removed the Community Issue/PR opened by the open-source community label Nov 27, 2023

merelcht and others added 9 commits November 27, 2023 11:58

Merge branch 'main' into build/datasets-upgrade-s3fs-to-newer-calver

3bfbfdc

Signed-off-by: Merel Theisen <49397448+merelcht@users.noreply.github.com>

Merge branch 'main' into build/datasets-upgrade-s3fs-to-newer-calver

66931e8

Signed-off-by: Merel Theisen <49397448+merelcht@users.noreply.github.com>

Add patching for AWSResponse to make moto and aiobotocore work

619213c

Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>

Fix lint

cee9b80

Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>

Add AWS mocking to lazy polars dataset test + temporarily ignore spar…

dabd773

…k tests Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>

Try use same moto for all python versions

3bc77e0

Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>

Merge branch 'main' into build/datasets-upgrade-s3fs-to-newer-calver

e5b551d

Signed-off-by: Merel Theisen <49397448+merelcht@users.noreply.github.com>

Undo polars changes

cf32731

Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>

Merge branch 'build/datasets-upgrade-s3fs-to-newer-calver' of github.…

32e0e31

…com:MatthiasRoels/kedro-plugins into build/datasets-upgrade-s3fs-to-newer-calver

merelcht self-assigned this Nov 30, 2023

merelcht mentioned this pull request Nov 30, 2023

build: Experiment aws patching #458

Closed

4 tasks

chore(datasets): fix accidental reference to NumPy (kedro-org#450)

402439e

Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu>

merelcht force-pushed the build/datasets-upgrade-s3fs-to-newer-calver branch from 750f31a to 402439e Compare November 30, 2023 16:20

Merge branch 'main' into build/datasets-upgrade-s3fs-to-newer-calver

1857887

This was referenced Dec 4, 2023

build: Debug bump s3fs #459

Closed

build(datasets): Bump s3fs #463

Merged

merelcht closed this Dec 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build(datasets): upgrade s3fs to newer calver #348

build(datasets): upgrade s3fs to newer calver #348

MatthiasRoels commented Sep 26, 2023 •

edited

Loading

noklam commented Sep 26, 2023 •

edited

Loading

MatthiasRoels commented Sep 26, 2023

noklam commented Sep 26, 2023

sbrugman Sep 29, 2023

MatthiasRoels Oct 2, 2023

noklam Sep 29, 2023

MatthiasRoels Oct 2, 2023

MatthiasRoels commented Oct 12, 2023 •

edited

Loading

merelcht commented Oct 25, 2023

noklam commented Nov 20, 2023

MatthiasRoels commented Nov 30, 2023

merelcht commented Dec 1, 2023

merelcht commented Dec 5, 2023

merelcht commented Dec 7, 2023

build(datasets): upgrade s3fs to newer calver #348

build(datasets): upgrade s3fs to newer calver #348

Conversation

MatthiasRoels commented Sep 26, 2023 • edited Loading

Description

Development notes

Checklist

noklam commented Sep 26, 2023 • edited Loading

MatthiasRoels commented Sep 26, 2023

noklam commented Sep 26, 2023

sbrugman Sep 29, 2023

Choose a reason for hiding this comment

MatthiasRoels Oct 2, 2023

Choose a reason for hiding this comment

noklam Sep 29, 2023

Choose a reason for hiding this comment

MatthiasRoels Oct 2, 2023

Choose a reason for hiding this comment

MatthiasRoels commented Oct 12, 2023 • edited Loading

merelcht commented Oct 25, 2023

noklam commented Nov 20, 2023

MatthiasRoels commented Nov 30, 2023

merelcht commented Dec 1, 2023

merelcht commented Dec 5, 2023

merelcht commented Dec 7, 2023

MatthiasRoels commented Sep 26, 2023 •

edited

Loading

noklam commented Sep 26, 2023 •

edited

Loading

MatthiasRoels commented Oct 12, 2023 •

edited

Loading