Migrate to `boto3`. Fix #43 #164

mpenkov · 2017-12-22T13:32:43Z

Creating a new pull request to get around travis' secure data issues.

menshikh-iv

Looks good

menshikh-iv · 2017-12-25T06:42:06Z

smart_open/tests/test_smart_open.py

-        conn = boto.connect_s3()
-        conn.create_bucket("mybucket")
+        s3 = boto3.resource('s3')
+        s3.create_bucket(Bucket='mybucket')


Maybe, better to use "global" bucket /key (as in https://github.com/RaRe-Technologies/smart_open/blob/master/smart_open/tests/test_s3.py#L16) + same maybe_mock_s3?

menshikh-iv · 2017-12-25T06:43:05Z

smart_open/tests/test_smart_open.py

@@ -968,6 +975,9 @@ class S3OpenTest(unittest.TestCase):
    @mock_s3
    def test_r(self):
        """Reading a UTF string should work."""
+        #


What's your plan to do with this type of test (with FIXME)?

mpenkov · 2018-03-16T07:54:51Z

@piskvorky @menshikh-iv Moving all tests across to boto3 requires moving some of our core functionality from boto to boto3. This affects two features:

smart_open accepts boto keys. If we switch to boto3, we could either drop this feature, or silently substitute incoming boto objects with boto3 ones.
s3_iter_bucket accepts a boto Bucket, and yields boto Keys. If we switch to boto3, we will have to do something about this feature, e.g. always accept and return boto3. We can also write two versions of the function, one for boto2 and one for boto3, but this is labor-intensive.

I can see three ways going forward. In my opinion, in order of decreasing subjective goodness:

Stop using boto/boto3 in the interface, and use native datatypes only, i.e. strings, to identify resources. This decouples our API from boto and boto3 - which particular library we use to do the work becomes purely an implementation detail.
Use boto3 only.
Use boto3, maintain boto functionality as separate.
Keep things as is: use boto in the API, use boto3 under the covers.

I think 1) is a bit extreme, because it will surprise (anger) some people. 3) is poor, because it makes us do additional work to support a deprecated library, but marginally better than 4). I think 2) is a good compromise, because it isn't difficult to switch between boto and boto3 in application code.

What do you guys think?

menshikh-iv · 2018-03-16T09:19:46Z

Thanks for detailed description @mpenkov, I'm +1 for (2) because we want to migrate to boto3 a long time ago, see #43, I see no reasons to support boto2 anymore (more than that, this project doesn't develop actively now https://github.com/boto/boto (they send us to boto3)).

piskvorky · 2018-03-16T15:47:24Z

I agree with @menshikh-iv .

menshikh-iv · 2018-03-18T05:56:32Z

.travis.yml

@@ -30,6 +30,8 @@ matrix:

 install:
  - pip install .[test]
+  - pip uninstall --yes botocore boto3


better to edit setup.py as we discussed, I think this will work

menshikh-iv · 2018-03-18T06:00:07Z

smart_open/s3.py

+_MULTIPROCESSING = False
+try:
+    import multiprocessing.pool
+    _MULTIPROCESSING = True


why can multiprocessing be unavailable?

I trust the above comment. It may be unavailable in certain environments, but I've never been in one.

menshikh-iv · 2018-03-18T06:03:16Z

smart_open/tests/test_smart_open.py

@@ -672,168 +615,6 @@ def write_callback(request):
        assert responses.calls[3].request.url == "http://127.0.0.1:8440/file"


-class S3IterBucketTest(unittest.TestCase):


These tests have no more sense, am I right?

Yes. Most of them moved to the s3 module. The others have lost their meaning with the rewrite.

menshikh-iv · 2018-04-02T08:09:29Z

@mpenkov what's missed here (especially what's about cases when this is impossible to replace old boto)? Is current PR ready to merge?

mpenkov · 2018-04-02T09:05:50Z

@menshikh-iv Yes, I think it's ready.

menshikh-iv · 2018-04-02T10:06:09Z

Great work @mpenkov 🔥

use boto3 in tests wherever possible

110024c

menshikh-iv reviewed Dec 25, 2017

View reviewed changes

get rid of unused unsecured mode code

488ce43

mpenkov added 4 commits March 17, 2018 17:54

rewrote s3_iter_bucket using boto3

27d68dc

disabled slow unit test

ea2aba0

attempting to work around ContextualVersionConflict under Py3.3

5ea054f

suppress prompt from pip uninstall

e4f6ce7

menshikh-iv reviewed Mar 18, 2018

View reviewed changes

inksink mentioned this pull request Mar 30, 2018

Migrate to boto3 #43

Closed

Merge branch 'master' into boto3

ae3b3bd

mpenkov added 2 commits April 2, 2018 18:43

adding unit test

fab9951

update comment

f4072a8

menshikh-iv changed the title ~~use boto3 in tests wherever possible~~ Migrate to boto3. Fix #43 Apr 2, 2018

menshikh-iv merged commit fd227d5 into master Apr 2, 2018

piskvorky deleted the boto3 branch April 2, 2018 15:34

macks22 mentioned this pull request Jul 15, 2018

Support for 's3a://' URIs #210

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to `boto3`. Fix #43 #164

Migrate to `boto3`. Fix #43 #164

mpenkov commented Dec 22, 2017

menshikh-iv left a comment

menshikh-iv Dec 25, 2017

mpenkov Apr 2, 2018

menshikh-iv Dec 25, 2017

mpenkov commented Mar 16, 2018 •

edited

Loading

menshikh-iv commented Mar 16, 2018

piskvorky commented Mar 16, 2018

menshikh-iv Mar 18, 2018

menshikh-iv Mar 18, 2018

mpenkov Mar 30, 2018

menshikh-iv Mar 18, 2018

mpenkov Mar 18, 2018 •

edited

Loading

menshikh-iv commented Apr 2, 2018

mpenkov commented Apr 2, 2018

menshikh-iv commented Apr 2, 2018

		@@ -672,168 +615,6 @@ def write_callback(request):
		assert responses.calls[3].request.url == "http://127.0.0.1:8440/file"


		class S3IterBucketTest(unittest.TestCase):

Migrate to boto3. Fix #43 #164

Migrate to boto3. Fix #43 #164

Conversation

mpenkov commented Dec 22, 2017

menshikh-iv left a comment

Choose a reason for hiding this comment

menshikh-iv Dec 25, 2017

Choose a reason for hiding this comment

mpenkov Apr 2, 2018

Choose a reason for hiding this comment

menshikh-iv Dec 25, 2017

Choose a reason for hiding this comment

mpenkov commented Mar 16, 2018 • edited Loading

menshikh-iv commented Mar 16, 2018

piskvorky commented Mar 16, 2018

menshikh-iv Mar 18, 2018

Choose a reason for hiding this comment

menshikh-iv Mar 18, 2018

Choose a reason for hiding this comment

mpenkov Mar 30, 2018

Choose a reason for hiding this comment

menshikh-iv Mar 18, 2018

Choose a reason for hiding this comment

mpenkov Mar 18, 2018 • edited Loading

Choose a reason for hiding this comment

menshikh-iv commented Apr 2, 2018

mpenkov commented Apr 2, 2018

menshikh-iv commented Apr 2, 2018

Migrate to `boto3`. Fix #43 #164

Migrate to `boto3`. Fix #43 #164

mpenkov commented Mar 16, 2018 •

edited

Loading

mpenkov Mar 18, 2018 •

edited

Loading