Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-21429

Stop S3-backed butler tests from attempting import/export

    XMLWordPrintable

    Details

      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.

        Attachments

          Activity

          Hide
          dinob 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....

          Show
          dinob 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
          tjenness 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
          tjenness 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
          dinob 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.

          Show
          dinob 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
          jbosch 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
          jbosch 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
          tjenness Tim Jenness added a comment -

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

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

            People

            Assignee:
            jbosch Jim Bosch
            Reporter:
            jbosch Jim Bosch
            Reviewers:
            Dino Bektesevic
            Watchers:
            Dino Bektesevic, Jim Bosch, Tim Jenness
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved:

                CI Builds

                No builds found.