# Stop S3-backed butler tests from attempting import/export

XMLWordPrintable

#### Details

• Type: Story
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
• Story Points:
0.5
• Team:
Data Release Production

#### Description

DM-17023's addition of import/export functionality included a test that is intended to be run only on PosixDatastore-backed butlers, because that's the only datastore that implements export at present.  However, the S30-backed butler test class inherits from the PosixDatastore-backed test class, so when the (optional) moto/boto3 dependencies are present, the import/export test is attempted there, too, and fails.

The right fix appears to be changing the inheritance relationship so that S3 and PosixDatastore butler tests both inherit from a common base class, mirroring the inheritance relationship of the datastore classes themselves.

#### Activity

Hide
Dino Bektesevic added a comment - - edited

I was looking at the code and, while there could be more work that I missed, most of the repoTransfer seemed pliable to change. Mostly it's stuff like:

that would have to be made to look more like:
https://github.com/lsst/daf_butler/blob/master/python/lsst/daf/butler/core/config.py#L106

and the actual file transfer would, in a perfect world, already work, since it just "punts" the work off to Butler.ingest functionality:
https://github.com/lsst/daf_butler/blob/feec7ef31987ef15e2eab2d36e336c8f6c074054/python/lsst/daf/butler/core/repoTransfers.py#L393

I would have to test and see what happens in S3 edge cases of "in-place" exports when users don't have permission to the entire bucket, but if I just assume permissions are there for both parties then targeting buckets for import/export can be a thing.

EDIT: I will run it as soon as I manage, it's just that you caught me in the middle of updating my stack....

Show
Dino Bektesevic added a comment - - edited I was looking at the code and, while there could be more work that I missed, most of the repoTransfer seemed pliable to change. Mostly it's stuff like: https://github.com/lsst/daf_butler/blob/feec7ef31987ef15e2eab2d36e336c8f6c074054/python/lsst/daf/butler/core/repoTransfers.py#L335 https://github.com/lsst/daf_butler/blob/feec7ef31987ef15e2eab2d36e336c8f6c074054/python/lsst/daf/butler/core/repoTransfers.py#L361 that would have to be made to look more like: https://github.com/lsst/daf_butler/blob/master/python/lsst/daf/butler/core/config.py#L106 and the actual file transfer would, in a perfect world, already work, since it just "punts" the work off to Butler.ingest functionality: https://github.com/lsst/daf_butler/blob/feec7ef31987ef15e2eab2d36e336c8f6c074054/python/lsst/daf/butler/core/repoTransfers.py#L393 I would have to test and see what happens in S3 edge cases of "in-place" exports when users don't have permission to the entire bucket, but if I just assume permissions are there for both parties then targeting buckets for import/export can be a thing. EDIT: I will run it as soon as I manage, it's just that you caught me in the middle of updating my stack....
Hide
Tim Jenness added a comment -

Jim Bosch I was thinking of adding a simple test to the S3 subclass along the lines of:

 @unittest.expectedFailure def testImportExport(self):  # call the import export test that should fail 

The "x" has the advantage that it looks different and will be a continual reminder. This does mean the import export test moves into the parent test class though.

Show
Tim Jenness added a comment - Jim Bosch I was thinking of adding a simple test to the S3 subclass along the lines of: @unittest .expectedFailure def testImportExport( self ): # call the import export test that should fail The "x" has the advantage that it looks different and will be a continual reminder. This does mean the import export test moves into the parent test class though.
Hide
Dino Bektesevic added a comment -

Jim Bosch I ran scons on your branch and everything passed short of the two warnings bellow:

 ============================================================================================== warnings summary ============================================================================================== tests/test_butlerFits.py::PosixDatastoreButlerTestCase::testExposureCompositePutGetConcrete  /home/dinob/uni/lsstspark/w_2019_38/python/miniconda3-4.5.12/envs/lsst-scipipe-1172c30/lib/python3.7/importlib/_bootstrap.py:219: RuntimeWarning: numpy.ufunc size changed, may indicate binary incompatibility. Expected 216, got 192  return f(*args, **kwds)  /home/dinob/uni/lsstspark/w_2019_38/python/miniconda3-4.5.12/envs/lsst-scipipe-1172c30/lib/python3.7/importlib/_bootstrap.py:219: RuntimeWarning: numpy.ufunc size changed, may indicate binary incompatibility. Expected 216, got 192  return f(*args, **kwds)  /home/dinob/uni/lsstspark/w_2019_38/python/miniconda3-4.5.12/envs/lsst-scipipe-1172c30/lib/python3.7/importlib/_bootstrap.py:219: ImportWarning: can't resolve package from __spec__ or __package__, falling back on __name__ and __path__  return f(*args, **kwds)   -- Docs: http://doc.pytest.org/en/latest/warnings.html ================================================================================== 403 passed, 3 warnings in 87.94 seconds =================================================================================== sys:1: ResourceWarning: unclosed  Global pytest run completed successfully  

The socket warning is known from before DM-21018 and I believe the binary mismatch is a numpy issue that is also known and harmless. Pending the decission, following the above conversation, this looks like it can be merged.

Show
Dino Bektesevic added a comment - Jim Bosch I ran scons on your branch and everything passed short of the two warnings bellow: ============================================================================================== warnings summary ============================================================================================== tests /test_butlerFits .py::PosixDatastoreButlerTestCase::testExposureCompositePutGetConcrete /home/dinob/uni/lsstspark/w_2019_38/python/miniconda3-4 .5.12 /envs/lsst-scipipe-1172c30/lib/python3 .7 /importlib/_bootstrap .py:219: RuntimeWarning: numpy.ufunc size changed, may indicate binary incompatibility. Expected 216, got 192 return f(*args, **kwds) /home/dinob/uni/lsstspark/w_2019_38/python/miniconda3-4 .5.12 /envs/lsst-scipipe-1172c30/lib/python3 .7 /importlib/_bootstrap .py:219: RuntimeWarning: numpy.ufunc size changed, may indicate binary incompatibility. Expected 216, got 192 return f(*args, **kwds) /home/dinob/uni/lsstspark/w_2019_38/python/miniconda3-4 .5.12 /envs/lsst-scipipe-1172c30/lib/python3 .7 /importlib/_bootstrap .py:219: ImportWarning: can't resolve package from __spec__ or __package__, falling back on __name__ and __path__ return f(*args, **kwds)   -- Docs: http: //doc .pytest.org /en/latest/warnings .html ================================================================================== 403 passed, 3 warnings in 87.94 seconds =================================================================================== sys:1: ResourceWarning: unclosed <socket.socket fd=12, family=AddressFamily.AF_INET, type =SocketKind.SOCK_DGRAM, proto=0, laddr=( '0.0.0.0' , 0)> Global pytest run completed successfully The socket warning is known from before DM-21018 and I believe the binary mismatch is a numpy issue that is also known and harmless. Pending the decission, following the above conversation, this looks like it can be merged.
Hide
Jim Bosch added a comment -

I've done as Tim Jenness suggested with the expected test failure, and tested that it works after doing a conda install of moto (which brought in moto3, too).  Will merge after Jenkins finishes.

Show
Jim Bosch added a comment - I've done as Tim Jenness suggested with the expected test failure, and tested that it works after doing a conda install of moto (which brought in moto3, too).  Will merge after Jenkins finishes.
Hide
Tim Jenness added a comment -

Looks good to me. I've also tested it locally and approved on GitHub.

Show
Tim Jenness added a comment - Looks good to me. I've also tested it locally and approved on GitHub.

#### People

Assignee:
Jim Bosch
Reporter:
Jim Bosch
Reviewers:
Dino Bektesevic
Watchers:
Dino Bektesevic, Jim Bosch, Tim Jenness