DM-26383: Add webdav datastore tests to daf_butler #356

bgounon · 2020-08-20T09:29:21Z

Following merged PR #351, here we're adding explicit WebdavDatastore tests. This requires starting a local webdav server against which requests are run. Adds a dependency to wsgidav and cheroot packages, for testing only. Tests are skipped if dependencies cannot be met.
Also implementing ButlerURI.parent() method, which returns the parent directory of the current ButlerURI (same as dirname() for a file-like URI).

timj

This looks good. I do have some comments that should be thought about before I can merge. If you are going to do further work I will add you to the daf_butler repo as a collaborator.

timj · 2020-08-20T16:09:18Z

python/lsst/daf/butler/_butlerConfig.py

@@ -93,7 +93,7 @@ def __init__(self, other=None, searchPaths=None):
                    uri = ButlerURI(other, forceDirectory=True)
                uri.updateFile("butler.yaml")
                other = uri.geturl()
-            elif uri.scheme == "https":
+            elif uri.scheme.startswith("http"):


This change should no longer be needed since I fixed that code in #355

timj · 2020-08-20T16:14:13Z

python/lsst/daf/butler/datastores/webdavDatastore.py

@@ -181,6 +181,9 @@ def _write_in_memory_to_artifact(self, inMemoryDataset: Any, ref: DatasetRef) ->
            raise FileExistsError(f"Cannot write file for ref {ref} as "
                                  f"output file {location.uri} exists.")

+        if not location.uri.dirname().exists():


Need to consider how we deal with this in the new RemoteFileDatastore that has replaced this implementation. For now we should probably just add it and absorb the one extra S3 call.

timj · 2020-08-20T16:15:33Z

tests/config/basic/webdavDatastore.yaml

@@ -1,6 +1,6 @@
 datastore:
  cls: lsst.daf.butler.datastores.webdavDatastore.WebdavDatastore
-  root: https://some.webdav.server/butlerRoot
+  root: http://localhost:8080/butlerRoot


Maybe add a comment to the YAML here noting that this is an explicit URL for the test server rather than something random.

timj · 2020-08-20T16:19:57Z

tests/test_butler.py

+        self.tmpConfigFile = posixpath.join(rooturi, "butler.yaml")
+
+    def tearDown(self):
+        rooturi = f"http://{self.serverName}/{self.root}"


Might be safer to store this in the object in setUp so you can guarantee consistency in rooturi.

timj · 2020-08-20T16:20:37Z

tests/test_butler.py

+        self.datastoreStr = f"datastore={self.root}"
+        self.datastoreName = [f"WebdavDatastore@{rooturi}"]
+
+        Butler.makeRepo(rooturi, config=config, forceConfigRoot=False)


makeRepo can take a ButlerURI (but make sure you use forceDirectory) if that makes things easier for you.

timj · 2020-08-20T16:22:10Z

tests/test_butler.py

+
+        config = {
+            "host": "0.0.0.0",
+            "port": 8080,


Do we have to use 8080? Does it deal with people who are running local web servers there already? If you run the tests with pytest -n5 tests/test_butler.py won't it start multiple web servers, one for each subprocess? Don't they clash?

We don't necessarily have to use 8080, any port above 1024 is probably fine (we may run into permissions issues if using any port below 1024).
I've added some necessary checks : if there is another service running on the selected port, the webdav server will fail to start and all tests will raise an OSError and fail.

Running pytest with -n5 for example does not seem to be an issue and succeeds. I'm not sure if it's something related to how pytest works with multithread, or if it's just the fact that, once the first webdav server is up, subsequent webdav server threads silently fail and tests all run against the first one.

I wonder if using port 0 works (it depends on whether you can ask the server what port it has been assigned).

It seems plausible that they are sharing the same server. This seems a bit risky if the first subprocess completes before the others have finished talking to it. I imagine we are lucking out because the webdav tests are a small fraction of all the tests.

timj · 2020-08-20T16:22:56Z

tests/test_butler.py

+        """Starts a local webdav-compatible HTTP server,
+        Listening on http://localhost:8080
+        This server only runs when this test class is instantiated,
+        and then shuts down. Must be started is a separate thread.


What causes it to shut down? Can it be shutdown in tearDownClass ?

Here we use a daemon thread, which is automatically killed once the main program finishes its execution.
Another solution would be to make the thread non-daemon and explicitly ask it to terminate in tearDown(), but I think the result would be identical.

If we do kill it in tearDownClass then that might make it more obvious that the other subprocesses are using it.

timj · 2020-08-20T16:27:12Z

tests/test_uri.py

@@ -358,11 +358,16 @@ def testQuoting(self):
 class WebdavURITestCase(unittest.TestCase):

    def setUp(self):
+        os.environ["WEBDAV_AUTH_METHOD"] = "TOKEN"


I think you should use unittest.mock.patch here to patch the environment so we know we aren't contaminating other tests that might be called later. I haven't entirely thought about how this can be done without having the env set for each test separately.

Done in ebf3ebe.
@unittest.mock.patch.dict class decorator only applies to methods with test prefix in the name, which means it doesn't apply to setUp() and tearDown() for example: I had to manually decorate those methods in test_butler.py.

bgounon · 2020-08-25T13:57:35Z

After rebase, I'm having trouble with _extractIngestInfo in fileLikeDatastore not transferring files as expected, I'll update the PR when this is figured out.

edit: the issue was with using POST (privously we used PUT) to transfer a local file to the webdav repository. Moving from POST to PUT fixed it.

timj · 2020-08-25T14:44:38Z

python/lsst/daf/butler/datastores/fileLikeDatastore.py

@@ -710,6 +710,9 @@ def _extractIngestInfo(self, path: Union[str, ButlerURI], ref: DatasetRef, *,
            # Work out the name we want this ingested file to have
            # inside the datastore
            tgtLocation = self._calculate_ingested_datastore_name(srcUri, ref, formatter)
+            if not tgtLocation.uri.dirname().exists():
+                log.debug("Folder %s does not exist yet.", tgtLocation.uri.dirname().geturl())


I don't think you need the geturl() here because there is a __str__ method that does this.

bgounon · 2020-08-26T12:52:21Z

@timj I think this new version addresses all your observations.
In particular, the webdav server now dynamically chooses a free port on which to listen. This allows for multiple webdav servers to start simultaneously without interfering with each other (for example when using pytest -nX).
Also, the server is now gracefully shut down in tearDownClass instead of using daemon threads.

bgounon changed the title ~~Add webdav datastore tests to daf_butler~~ [DM-26383] Add webdav datastore tests to daf_butler Aug 20, 2020

bgounon changed the title ~~[DM-26383] Add webdav datastore tests to daf_butler~~ DM-26383: Add webdav datastore tests to daf_butler Aug 20, 2020

timj approved these changes Aug 20, 2020

View reviewed changes

bgounon force-pushed the tickets/DM-26383 branch from ebf3ebe to f8ccdab Compare August 25, 2020 14:19

timj reviewed Aug 25, 2020

View reviewed changes

bgounon force-pushed the tickets/DM-26383 branch from 7cd4a09 to 340bbfd Compare August 26, 2020 12:27

bgounon force-pushed the tickets/DM-26383 branch from 3ba4129 to b8ae901 Compare August 27, 2020 08:22

bgounon added 21 commits August 27, 2020 10:22

Run Webdav compatibility check on base URL

824ba26

Create parent directories when using mkdir

861a6fd

Implement explicit WebdavDatastore tests

1794ad9

Fix Webdav ButlerURI tests

38e0adf

Set required env variables before running tests

f9d2de2

Lint

19e7aa1

Use dirname() instead of split()

9d4d818

Skip webdav tests if dependencies not satisfied

3aa7079

Implement parent dir feature on ButlerURI

694cf3d

Add docstring

1fed2b4

Make sure parent dir is different before mkdir

317a786

Store rooturi as attribute for cleaner tearDown

bd41173

Check the webdav server has started as expected

26a4646

Add explanatory comment for webdav root config

bfeb772

Use mock.patch.dict to mock env variables

72de2d2

Use PUT instead of POST in HTTP transfer_from

a694f63

Fix string syntax in log.debug() calls

20fffe5

Don't use geturl on ButlerURIs for printing

ad71138

Dynamically select free port instead of 8080

0bda3a7

Explicitly wait for thread termination at tearDown

c37912b

Add docstrings and comments

e76e312

Move server stop to 'finally' clause

b8ae901

bgounon requested a review from timj August 27, 2020 08:29

timj merged commit 3815080 into lsst:master Aug 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-26383: Add webdav datastore tests to daf_butler #356

DM-26383: Add webdav datastore tests to daf_butler #356

bgounon commented Aug 20, 2020

timj left a comment

timj Aug 20, 2020

timj Aug 20, 2020

timj Aug 20, 2020

timj Aug 20, 2020 •

edited

timj Aug 20, 2020

timj Aug 20, 2020

bgounon Aug 25, 2020

bgounon Aug 25, 2020

timj Aug 25, 2020

timj Aug 25, 2020

timj Aug 20, 2020

bgounon Aug 25, 2020

timj Aug 25, 2020

timj Aug 20, 2020

bgounon Aug 25, 2020

bgounon commented Aug 25, 2020 •

edited

timj Aug 25, 2020

bgounon commented Aug 26, 2020 •

edited

DM-26383: Add webdav datastore tests to daf_butler #356

DM-26383: Add webdav datastore tests to daf_butler #356

Conversation

bgounon commented Aug 20, 2020

timj 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

timj Aug 20, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgounon commented Aug 25, 2020 • edited

Choose a reason for hiding this comment

bgounon commented Aug 26, 2020 • edited

timj Aug 20, 2020 •

edited

bgounon commented Aug 25, 2020 •

edited

bgounon commented Aug 26, 2020 •

edited