Bump `aws-sdk-cpp` to 1.18.105 #1226

vnghia · 2020-12-14T10:28:57Z

This PR:

Bump aws-sdk-cpp to 1.18.105
Remove unused code for getting s3 region since it will be done automatically by aws-sdk-cpp
Enable ENABLE_CURL_LOGGING
Support more http code for TF_SetStatusFromAWSError
I am introducing a breaking change here. RecursivelyCreateDir now calls CreateDir instead of relying on TensorFlow fallback. ( Not really a breaking change though )

Part of #1183

tensorflow_io/core/plugins/s3/s3_filesystem.cc

vnghia · 2020-12-14T10:38:03Z

tensorflow_io/core/plugins/s3/s3_filesystem.cc

-    case Aws::Http::HttpResponseCode::REQUESTED_RANGE_NOT_SATISFIABLE:
-      TF_SetStatus(status, TF_OUT_OF_RANGE, "Read less bytes than requested");
-      break;


I am not sure if this is correct or not since the old code already supported REQUESTED_RANGE_NOT_SATISFIABLE = 416. WDYT @yongtang ?

io/tensorflow_io/core/plugins/s3/s3_filesystem.cc

Lines 507 to 512 in 9b5e233

// SECTION 4. Implementation for `TF_Filesystem`, the actual filesystem

// ----------------------------------------------------------------------------

// TODO: use_multi_part_download=true will cause an issue when read request

// is asking for bytes larger than object size. In that case, a status code of

// 416 is returned. However, 416 is not handled correctly and s3 thought this

// is an error - should return OUT_OF_RANGE with less bytes.

@vnvo2409 The switch is controlled in:

io/tensorflow_io/core/plugins/s3/s3_filesystem.cc

Line 519 in 9b5e233

use_multi_part_download(false), // TODO: change to true after fix

You can change the default value to true and see if the issue is resolved.

I am investigating this issue tensorflow/tensorflow#44564. Let's leave it for another PR.

@yongtang

tensorflow/tensorflow#44564 is resolved. We could enable this option

tensorflow_io/core/plugins/s3/s3_filesystem.cc

vnghia · 2020-12-16T15:07:51Z

@yongtang
I could not understand why the test keeps falling with segfault and I could not reproduce it locally. Do you have some hints why it happens ? Thank you very much !

yongtang · 2020-12-16T15:56:12Z

@vnvo2409 The crash could be caused by duplicate symbols between aws and arrow library? Though I am not quite sure. It seems to be happening on Linux only.

Could you try split the PR into two PRs with one PR for aws-sdk-cpp to 1.18.105 and another PR for other changes like Enable ENABLE_CURL_LOGGING, Support more http code for TF_SetStatusFromAWSError? This can help us locate the cause of the issue better.

In the meantime, I am trying to update the arrow library from 0.16.0 to 2.0.0 in #1231. We need to update arrow library anyway so I think it makes sense to see if updating arrow will resolve the aws+arrow issue.

vnghia · 2020-12-17T22:04:54Z

I already rebase to pickup the arrow update but it still failed. Do you have any idea what I should do next @yongtang ? Thank you very much !

yongtang · 2020-12-18T01:03:15Z

@vnvo2409 The Linux build on GitHub Actions is in:
https://github.com/tensorflow/io/blob/master/.github/workflows/build.yml#L222-L227

There are two difference in between the GitHub Actions and the normal bazel build.

GitHub Actions utilize the optimization mode with --copt=-msse4.2 --copt=-mavx --compilation_mode=opt passed to bazel
GitHub Actions utilize the docker image from tensorflow's build image which is Ubuntu16.04+ customized gcc from developer toolset.

I tends to believe optimization (1) is the factor though I cannot exclude (2) yet. Will take a look.

vnghia · 2020-12-19T17:10:30Z

0f3e1a4 fixes a ssl support problem on Mac because USE_DARWINSSL is renamed to USE_SECTRANSP https://github.com/curl/curl/blob/ccbdbe13c43f15a637afdd1ac4bf5df62c25a235/CMakeLists.txt#L365-L367

vnghia · 2020-12-20T17:33:00Z

@yongtang

I build it separately and it seems fine without any issue. I doubt that it somehow conflicts with the s3 inside TensorFlow. On my machine, sometime it works and sometime it throws segfaul 😕. I am building a custom wheel without s3 to see if the conflict is the problem.

yongtang · 2020-12-20T18:34:27Z

@vnvo2409 Ah that might explain the segment as there are actually two copies of AWS C++ sdk. One is in tensorflow and another is in tensorflow-io. And they are with different versions. If the AWS C++ SDK on both tensorflow-io and tensorflow are updated to the same version then it might be OK (as two copies with same version).. Or if we remove the one from tensorflow (so that there is only one version) then it might also be OK I guess.

kvignesh1420 · 2021-02-08T16:18:14Z

@vnvo2409 the PR by @yongtang : (#1241) to exclude external libraries has been merged. Can you please check and proceed with this PR?

vnghia · 2021-02-08T17:09:20Z

Sorry. I'm still stuck at the segfault. I think I will try this PR again when aws is completely removed out of Tensorflow.

vnghia commented Dec 14, 2020

View reviewed changes

tensorflow_io/core/plugins/s3/s3_filesystem.cc Outdated Show resolved Hide resolved

vnghia commented Dec 14, 2020

View reviewed changes

yongtang reviewed Dec 14, 2020

View reviewed changes

tensorflow_io/core/plugins/s3/s3_filesystem.cc Outdated Show resolved Hide resolved

vnghia force-pushed the s3-improvements branch from 48ab23a to 5aaf40d Compare December 14, 2020 17:02

vnghia requested a review from yongtang December 14, 2020 17:11

vnghia force-pushed the s3-improvements branch from 96f2749 to 49f329b Compare December 16, 2020 13:26

vnghia force-pushed the s3-improvements branch 2 times, most recently from 7ba27f5 to ad0194c Compare December 17, 2020 00:03

vnghia force-pushed the s3-improvements branch from ad0194c to 77f3075 Compare December 19, 2020 14:34

vnghia force-pushed the s3-improvements branch from 1beaf9b to 0f3e1a4 Compare December 20, 2020 05:05

vnghia added 2 commits December 23, 2020 10:17

Bump aws-sdk-cpp to 1.8.105

25952a6

fix curl ssl BUILD on macos

f237e50

vnghia force-pushed the s3-improvements branch from 0f3e1a4 to f237e50 Compare December 23, 2020 03:18

remove libtensorflow_framework in plugins_header

e767ac1

vnghia mentioned this pull request Dec 23, 2020

S3 Improvements #1248

Merged

vnghia closed this Feb 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Bump `aws-sdk-cpp` to 1.18.105 #1226

Bump `aws-sdk-cpp` to 1.18.105 #1226

Uh oh!

vnghia commented Dec 14, 2020 •

edited

Loading

Uh oh!

Uh oh!

vnghia Dec 14, 2020

Uh oh!

yongtang Dec 14, 2020

Uh oh!

vnghia Dec 15, 2020

Uh oh!

vnghia Dec 19, 2020

Uh oh!

Uh oh!

vnghia commented Dec 16, 2020

Uh oh!

yongtang commented Dec 16, 2020

Uh oh!

vnghia commented Dec 17, 2020

Uh oh!

yongtang commented Dec 18, 2020

Uh oh!

vnghia commented Dec 19, 2020 •

edited

Loading

Uh oh!

vnghia commented Dec 20, 2020

Uh oh!

yongtang commented Dec 20, 2020

Uh oh!

kvignesh1420 commented Feb 8, 2021

Uh oh!

vnghia commented Feb 8, 2021

Uh oh!

Uh oh!

	// SECTION 4. Implementation for `TF_Filesystem`, the actual filesystem
	// ----------------------------------------------------------------------------
	// TODO: use_multi_part_download=true will cause an issue when read request
	// is asking for bytes larger than object size. In that case, a status code of
	// 416 is returned. However, 416 is not handled correctly and s3 thought this
	// is an error - should return OUT_OF_RANGE with less bytes.

Bump aws-sdk-cpp to 1.18.105 #1226

Bump aws-sdk-cpp to 1.18.105 #1226

Uh oh!

Conversation

vnghia commented Dec 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

vnghia Dec 14, 2020

Choose a reason for hiding this comment

Uh oh!

yongtang Dec 14, 2020

Choose a reason for hiding this comment

Uh oh!

vnghia Dec 15, 2020

Choose a reason for hiding this comment

Uh oh!

vnghia Dec 19, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vnghia commented Dec 16, 2020

Uh oh!

yongtang commented Dec 16, 2020

Uh oh!

vnghia commented Dec 17, 2020

Uh oh!

yongtang commented Dec 18, 2020

Uh oh!

vnghia commented Dec 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vnghia commented Dec 20, 2020

Uh oh!

yongtang commented Dec 20, 2020

Uh oh!

kvignesh1420 commented Feb 8, 2021

Uh oh!

vnghia commented Feb 8, 2021

Uh oh!

Uh oh!

Bump `aws-sdk-cpp` to 1.18.105 #1226

Bump `aws-sdk-cpp` to 1.18.105 #1226

vnghia commented Dec 14, 2020 •

edited

Loading

vnghia commented Dec 19, 2020 •

edited

Loading