daf_persistence Tickets/dm 7468 #51

n8pease · 2017-03-20T19:57:31Z

No description provided.

andy-slac

Looks OK, small bunch of minor comments.

andy-slac · 2017-03-22T19:29:30Z

include/lsst/daf/persistence/StorageFormatter.h

 * see <http://www.lsstcorp.org/LegalNotices/>.
 */
- 
+
 #ifndef LSST_MWI_PERSISTENCE_STORAGE_H
 #define LSST_MWI_PERSISTENCE_STORAGE_H


Change to LSST_MWI_PERSISTENCE_STORAGEFORMATTER_H? BTW, why LSST_MWI_PERSISTENCE... and not LSST_DAF_PERSISTENCE... (this applies to many other headers too).

andy-slac · 2017-03-22T19:30:56Z

include/lsst/daf/persistence/StorageFormatter.h

-    Storage& operator=(Storage const&);
+    // Do not copy or assign a StorageFormatter instance.
+    StorageFormatter(StorageFormatter const&);
+    StorageFormatter& operator=(StorageFormatter const&);


Use = delete instead of making them private?

andy-slac · 2017-03-22T20:01:11Z

tests/testSwiftStorage.py

+        user = os.getenv('SWIFT_USERNAME')
+        if user is None:
+            raise unittest.SkipTest(
+                'Can not run test: OS_USERNAME not in environment')


SWIFT_USERNAME

andy-slac · 2017-03-22T20:01:30Z

tests/testSwiftStorage.py

+        key = os.getenv('SWIFT_PASSWORD')
+        if key is None:
+            raise unittest.SkipTest(
+                'Can not run test: OS_PASSWORD not in environment')


SWIFT_PASSWORD

andy-slac · 2017-03-22T20:32:31Z

python/lsst/daf/persistence/posixStorage.py

        """
        p = os.path.join(self.root, path)
        if os.path.exists(p):
-            return p
+            return open(p)


Nitpick: in general I prefer to open() and catch an exception if it fails, as checking that file exist and opening it in two separate steps is non-atomic and my lead to some interesting errors. But I'm not sure you have to worry about this in your storage classes as they are probably the once managing lifetime of the files.

andy-slac · 2017-03-22T21:40:15Z

python/lsst/daf/persistence/swiftStorage.py

+        try:
+            f = self._getLocalFile(location)
+            self._log.info(
+                "...getting file took {} seconds.".format(time.time() - start))


Some people may not like too much nose at INFO log level.

Would debug be better, or what would you suggest?
For the time being I find it very useful for profiling Butler behavior, although I agree it will end up being noisy. I'd like to leave something that will print during unit test execution, but I don't need it to be 'info'.

I'd say that this was at the trace level, not even debug

I believe we log at INFO by default, so if you want to see this output from unit test at DEBUG/TRACE level you need to configure logging with non-default values.

andy-slac · 2017-03-22T21:42:35Z

python/lsst/daf/persistence/swiftStorage.py

+        """
+        # Now search for the path in the root or its parents
+        # Strip off any cfitsio bracketed extension if present
+        strippedPath, pathStripped = self.getFitsHeaderStrippedPath(path)


strippedPath, pathStripped is not going to confuse anyone? 😸

It confuses me! Actually it's pretty descriptive if you sit on your zen cushion for a minute. In any case, it's moved here from code before my time, and I don't want to mess with it right now.

OK, I'll practice zen, it'll just take longer to review if I need to focus for a minute on that line 😸

andy-slac · 2017-03-22T21:43:44Z

python/lsst/daf/persistence/swiftStorage.py

+        except self.swift.ClientException as err:
+            raise RuntimeError("Container \'{0}\' not found: {1}".format(
+                self._containerName, err))
+        import fnmatch


Should probably go to the top of the file.

andy-slac · 2017-03-22T21:46:28Z

python/lsst/daf/persistence/swiftStorage.py

+            # Assuming the file does not exist, not some other kind of error
+            # (how can we do more precise handling of this expcetion?)
+            return None
+        with open(localFile.name, mode='r') as f:


localFile is already a file object, can you read from it directly?

andy-slac · 2017-03-22T21:50:21Z

tests/testSwiftStorage.py

+import sys
+import uuid
+
+packageDir = os.path.join(getPackageDir('daf_persistence'))


Is os.path.join doing anything here?

airnandez

Please consider the proposed modifications.

airnandez · 2017-04-20T10:15:12Z

python/lsst/daf/persistence/swiftStorage.py

+    SWIFT_USERNAME : string
+        The username to use when authorizing the connection.
+    SWIFT_PASSWORD : string
+        The password to use when authorizing the connection.


In would suggest to use the environment variables OS_USERNAME, OS_PASSWORD and OS_TENANT_NAME for authentication information. These are the variables that the official Swift python command line interface uses for authentication, so I see value in keeping the same convention for the Butlet.

airnandez · 2017-04-20T10:22:42Z

python/lsst/daf/persistence/swiftStorage.py

+        A URI to connect to a swift storage location. The form of the URI is
+        `swift://[URL without 'http://']/[Object API Version]/[tenant name (account)]/[container]`
+        For example:
+        `swift://nebula.ncsa.illinois.edu:5000/v2.0/lsst/my_container`


An URL such as swift://nebula.ncsa.illinois.edu:5000/v2.0/lsst/my_container does not tell what scheme the Swift authentication server uses. For instance, NCSA's instance of Swift uses http:// but CC-IN2P3's uses https:// for both the authentication service and the storage server. In other words, it is not robust to infer that a swift:// URI implies http://.

An alternative way to create an object of the SwiftStorage class would be to pass as an argument a Python dictionary with the required fields for authentication, i.e.:

user name (or the value of OS_USERNAME if not specified)

password (or the value of OS_PASSWORD if not specified)

tenant name (or the value of OS_TENANT_NAME if not specified)

authentication URL (or the value of OS_AUTH_URL if not specified)

This mechanism would allow for accessing Butler repositories stored in several Swift instances (e.g. at NCSA and CC-IN2P3) for which the authentication information (auth url, user name, tenant name, password) is different.

airnandez · 2017-04-20T10:23:46Z

python/lsst/daf/persistence/swiftStorage.py

+            self._version, \
+            self._tenantName, \
+            self._containerName = self._parseURI(uri)
+        self._url = "http://" + os.path.join(self._url, self._version)


See my comment above. A Swift endpoint may speak http:// or https:// depending on how it is configured.

airnandez · 2017-04-20T10:26:33Z

python/lsst/daf/persistence/swiftStorage.py

+            The object representing the connection. The object may or may not
+            actually be connected.
+        """
+        user = os.getenv('SWIFT_USERNAME')


See above comment. I think OS_USERNAME would be a better variable for compatibility purposes.

airnandez · 2017-04-20T10:27:06Z

python/lsst/daf/persistence/swiftStorage.py

+        if user is None:
+            raise RuntimeError(
+                'SwiftStorage could not find SWIFT_USERNAME in environment')
+        key = os.getenv('SWIFT_PASSWORD')


See above comment. I think OS_PASSWORD would be a better variable for compatibility purposes.

airnandez · 2017-04-20T12:11:18Z

python/lsst/daf/persistence/swiftStorage.py

+        return storage.instanceSearch(path) if storage else None
+
+    def copyFile(self, fromLocation, toLocation):
+        """Copy a file from one location to another on the local filesystem.


From the implementation of this method I would say that the purpose here is to copy one Swift object into another Swift object, therefore in the same container, not in the local filesystem. If that's the intention, this comment needs to be updated.

When a dataset is downloaded from a remote resource, SwiftStorage puts the dataset into a NamedTemporaryFile, which will be deleted when the file object is deleted (or closed explicitly). This guarantees the file will exist as long as the object exists (in case the object refers internally to the file).

…ists

n8pease force-pushed the tickets/DM-7468 branch from c00f085 to 92a7133 Compare March 20, 2017 21:50

andy-slac approved these changes Mar 22, 2017

View reviewed changes

n8pease force-pushed the tickets/DM-7468 branch 7 times, most recently from 790f537 to 7ca1c3a Compare March 28, 2017 22:12

n8pease force-pushed the tickets/DM-7468 branch from 07e6cec to 7ca1c3a Compare April 6, 2017 16:18

airnandez reviewed Apr 20, 2017

View reviewed changes

n8pease force-pushed the tickets/DM-7468 branch from 7ca1c3a to 89875c5 Compare April 28, 2017 20:50

n8pease force-pushed the tickets/DM-7468 branch from 89875c5 to 4e1518f Compare May 23, 2017 23:51

n8pease and others added 17 commits May 31, 2017 15:24

create abstract StorageInterface class

5397383

add SwiftStorage and tests

93fcbc1

reorder includes to fix linter warnings

3afc030

correct getLocalFile docstring

91584b4

make PosixStorage inherit from StorageInterface abc

f3529e7

cache repoCfgs in Storage

0102300

rename the C++ Storage class to StorageFormatter

7ca102e

add a test that gets an sqlite registry from SwiftStorage

0011ba5

add generic _put method to SwiftStorage

1b0a679

move fnmatch import to top of file

8ce6b01

don't reopen file object

fba5d43

fix getLocalFile docstrings

9fc3847

make PosixStorage.getLocalFile handle exception instead of testing ex…

51aa682

…ists

remove file handle cache monkeypatch

3dbedcd

rename StorageFormatter to FormatterStorage

b12bbba

storage init takes optional 'create' arg

69dc820

move SwiftStorage to daf_fmt_swift package

f2c0865

n8pease force-pushed the tickets/DM-7468 branch from 4e1518f to f2c0865 Compare May 31, 2017 22:26

n8pease merged commit f2c0865 into master May 31, 2017

ktlim deleted the tickets/DM-7468 branch August 25, 2018 06:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

daf_persistence Tickets/dm 7468 #51

daf_persistence Tickets/dm 7468 #51

n8pease commented Mar 20, 2017

andy-slac left a comment

andy-slac Mar 22, 2017

andy-slac Mar 22, 2017

andy-slac Mar 22, 2017

andy-slac Mar 22, 2017

andy-slac Mar 22, 2017

andy-slac Mar 22, 2017

n8pease Mar 27, 2017

RobertLuptonTheGood Mar 27, 2017

andy-slac Mar 27, 2017

andy-slac Mar 22, 2017

n8pease Mar 27, 2017

andy-slac Mar 27, 2017

andy-slac Mar 22, 2017

andy-slac Mar 22, 2017

andy-slac Mar 22, 2017

airnandez left a comment

airnandez Apr 20, 2017

airnandez Apr 20, 2017

airnandez Apr 20, 2017

airnandez Apr 20, 2017

airnandez Apr 20, 2017

airnandez Apr 20, 2017

daf_persistence Tickets/dm 7468 #51

daf_persistence Tickets/dm 7468 #51

Conversation

n8pease commented Mar 20, 2017

andy-slac left a 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

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

airnandez left a 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