Bluemix Cloud Object Storage Support [Python] #28

bassel-zeidan · 2017-10-02T15:12:59Z

No description provided.

required credentials

gadamc · 2017-10-02T20:20:35Z

python/README.md

+The class CloudObjectStorage allows you to connect to bluemix cos. You can connect to bluemix using api keys
+as follows:
+
+```pythonw


should be python

gadamc · 2017-10-02T20:20:45Z

python/README.md

+```
+
+Alternatively, you can connect to bluemix cos using IAM token. Example:
+```pythonw


should be python

gadamc · 2017-10-02T20:22:07Z

python/README.md

+```pythonw
+import ibmos2spark
+
+# @hidden_cell


I think that if we add the # @hidden_cell we should 1) make a link to some docs to describe what it is, and 2) make a separate code snippet to encourage developers/users to separate hidden cells from the other parts of the code that doesn't need to be hidden.

We don't need that at all actually. It is a DSX thing. I took it out

gadamc · 2017-10-02T20:22:42Z

python/ibmos2spark/osconfig.py


        '''
+        This class allows you to connect to a cloud object storage (COS) instance. It also support connecting to a cos instance


gadamc · 2017-10-02T20:23:00Z

python/ibmos2spark/osconfig.py

-          * endpoint
-          * access_key
-          * secret_key
+        credentials:  a dictionary with the required keys to connect to cos. The required keys differ according


gadamc · 2017-10-02T20:23:10Z

python/ibmos2spark/osconfig.py

-          * access_key
-          * secret_key
+        credentials:  a dictionary with the required keys to connect to cos. The required keys differ according
+            to the type of cos.


gadamc · 2017-10-02T20:23:25Z

python/ibmos2spark/osconfig.py

-          * secret_key
+        credentials:  a dictionary with the required keys to connect to cos. The required keys differ according
+            to the type of cos.
+            - for cos type "softlayer_cos" the following key are required:


gadamc · 2017-10-02T20:23:32Z

python/ibmos2spark/osconfig.py

+              * endpoint
+              * access_key
+              * secret_key
+            - for cos type "bluemix_cos", here are the required/optional key:


gilv · 2017-10-03T03:47:33Z

@bassel-zeidan @gadamc
I think ( we better check ) - it need to IBM COS and not just COS

gilv · 2017-10-03T03:49:40Z

@bassel-zeidan @gadamc
https://www.ibm.com/cloud-computing/products/storage/object-storage/

bassel-zeidan · 2017-10-04T09:19:16Z

@gadamc @gilv i did some updates to the description and readme file

bassel-zeidan · 2017-10-04T09:19:24Z

Test notebook:
https://apsportal.ibm.com/analytics/notebooks/7901079b-b233-4fc4-acac-0420c1822787/view?access_token=0ab15c257179aa120283ee5a549d1a81147de66fd6b07b91a9464932b1a4d510

gilv · 2017-10-04T10:00:15Z

@bassel-zeidan @gadamc I don't think we should mention somewhere "Bluemix Cloud Object Storage Support" If it's only title - then it's fine, but if customers will see it somewhere in the code - it's wrong. We have IBM Cloud Object Storage", not "Bluemix Cloud Object Storage Support"

gadamc

I have a couple of comments on this PR, which would probably also apply to the R/Scala versions as well.

I'd like to get your comment about the named args in the README. Seems that it would be a little more helpful to include them.
What you think about the options regarding the maze of "if" statements, which I also commented on below?

Our options seem to be
a. do nothing and merge now
b. fix now and merge afterwards.
c. create a new issue to address code structure and merge now.

I'd prefer "b", but will accept answer "c".

gadamc · 2017-10-06T22:50:48Z

python/README.md

+}
+
+configuration_name = 'os_bluemix_cos_config'
+cos = ibmos2spark.CloudObjectStorage(sc, credentials, configuration_name, 'bluemix_cos')


should we use named args here instead? configuration_name=configuration_name, cos_type='bluemix_cos'

Of course having the args name would work but i think this form is quite nicer and minimised. The user who is using our lib knows Python and knows how params work in it. I think this form is fine especially because we explained in details all params and what they do.

gadamc · 2017-10-06T22:51:16Z

python/README.md

+}
+
+configuration_name = 'os_bluemix_cos_config'
+cos = ibmos2spark.CloudObjectStorage(sc, credentials, configuration_name, 'bluemix_cos', 'iam_token')


should we use named args here instead? configuration_name=configuration_name, cos_type='bluemix_cos', auth_method='iam_token'

gadamc · 2017-10-06T23:12:15Z

python/README.md

+
+bucket_name = 'bucket_name'
+object_name = 'file_name'
+data = sc.textFile(cos.url(object_name, bucket_name))


cos.url(object_name, bucket_name) is a confusing order (logically opposite order of typical folder/file hierarchy structure) and different from the container_name, object_name order of the softlayer/bluemix objects.

I know why it's there (historically that was how it was rolled out) and if we rearranged the order we'd break things...

I guess the questions are, how much would we break and how easy to repair? I guess it would break too many of the current deployments and usage of this lib in DSX accounts??

It's also really unfortunate that this is also different from the same functionality in the Scala and R versions.

Having the bucket_name at the end is due to the fact that this param is optional and in python it has to go to the end.

We could think about it if we want to change this in a next release

Right, but it's only optional because when this was first supported, we used a fixed configuration name. I was opposed to that because it didn't support multiple configurations like the other implementations, but it was pushed ahead anyways and released too quickly, in my opinion.

gadamc · 2017-10-06T23:28:33Z

python/ibmos2spark/osconfig.py

@@ -170,37 +170,50 @@ def url(self, container_name, object_name):

 class CloudObjectStorage(object):


I'm not a fan of the complicated maze of "if" statements.

Can we think of a cleaner solution that could be more easily extensible/configurable in the future?
We could subclass CloudObjectStorage and create a new class for each connection type. And/Or perhaps make a factory class/method for building the correct class type based on the input parameters.
I'm open to ideas here to simplify this. We could also consider incorporating the bluemix and softlayer classes into this code refactoring.

I think how it is right now is quite clean and clear. Honestly, i think having a new class for Bluemix COS would be confusing since it is really the same with different creds.

bassel-zeidan · 2017-10-09T21:51:18Z

@gadamc I will get the updates in. Let's collect feedback from the users with the next version and improve in case needed in next iterations.

bassel-zeidan added 11 commits September 28, 2017 14:58

Add new params for cos bluemix + write logic to validate new param and

ca8e0cc

required credentials

set up hadoop config with the new bluemix cos creds

d883a5e

Update class description

7a563e1

Add missing double points

5387cfa

Fix missing params

164b5b2

Fix bugs

aaa649f

switch to softlayer_cos naming instead of classic_cos

fd8146d

Add example for bluemix cos api keys

ff61451

Add example for connecting to bluemix cos using IAM token

50c424b

Add swift tag name for old swift os in readme file

6b26ef2

update readme

dbb7263

bassel-zeidan changed the title ~~Bluemix Cloud Object Storage Support [Python]~~ Bluemix Cloud Object Storage Support [Python] [Don't merge] Oct 2, 2017

gadamc reviewed Oct 2, 2017

View reviewed changes

bassel-zeidan added 2 commits October 4, 2017 11:09

update description and readme file

5defcb7

update description

e187184

bassel-zeidan changed the title ~~Bluemix Cloud Object Storage Support [Python] [Don't merge]~~ Bluemix Cloud Object Storage Support [Python] Oct 4, 2017

Update README.md

2b8cd15

bassel-zeidan mentioned this pull request Oct 4, 2017

Add IAM Support For the New Bluemix COS For Python, Scala and R #25

Closed

gadamc reviewed Oct 9, 2017

View reviewed changes

bassel-zeidan merged commit fc92a7f into master Oct 9, 2017

bassel-zeidan deleted the bluemix_cos_support_python branch October 9, 2017 21:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bluemix Cloud Object Storage Support [Python] #28

Bluemix Cloud Object Storage Support [Python] #28

bassel-zeidan commented Oct 2, 2017

gadamc Oct 2, 2017

gadamc Oct 2, 2017

gadamc Oct 2, 2017

bassel-zeidan Oct 4, 2017

gadamc Oct 2, 2017

gadamc Oct 2, 2017

gadamc Oct 2, 2017

gadamc Oct 2, 2017

gadamc Oct 2, 2017

gilv commented Oct 3, 2017

gilv commented Oct 3, 2017

bassel-zeidan commented Oct 4, 2017

bassel-zeidan commented Oct 4, 2017

gilv commented Oct 4, 2017

gadamc left a comment •

edited

gadamc Oct 6, 2017

bassel-zeidan Oct 9, 2017

gadamc Oct 6, 2017

bassel-zeidan Oct 9, 2017

gadamc Oct 6, 2017

bassel-zeidan Oct 9, 2017

bassel-zeidan Oct 9, 2017

gadamc Oct 9, 2017

gadamc Oct 6, 2017 •

edited

bassel-zeidan Oct 9, 2017

bassel-zeidan commented Oct 9, 2017


		'''
		This class allows you to connect to a cloud object storage (COS) instance. It also support connecting to a cos instance

		@@ -170,37 +170,50 @@ def url(self, container_name, object_name):

		class CloudObjectStorage(object):

Bluemix Cloud Object Storage Support [Python] #28

Bluemix Cloud Object Storage Support [Python] #28

Conversation

bassel-zeidan commented Oct 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gilv commented Oct 3, 2017

gilv commented Oct 3, 2017

bassel-zeidan commented Oct 4, 2017

bassel-zeidan commented Oct 4, 2017

gilv commented Oct 4, 2017

gadamc left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gadamc Oct 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bassel-zeidan commented Oct 9, 2017

gadamc left a comment •

edited

gadamc Oct 6, 2017 •

edited