Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

changed url for rasterio network test #3162

Merged
merged 2 commits into from
Jul 31, 2019
Merged

Conversation

scottyhq
Copy link
Contributor

fix failing rasterio network test by simplifying test and using same image url as rasterio library test suite

@max-sixty
Copy link
Collaborator

Thanks @scottyhq !

Did we disable some tests / builds before? If so, shall we re-enable them as part of this PR? (I remember seeing something but wasn't involved directly)

@fmaussion
Copy link
Member

@scottyhq
Copy link
Contributor Author

scottyhq commented Jul 26, 2019

but seems like the tests are now run with each build on Azure? https://dev.azure.com/xarray/xarray/_build/results?buildId=389&view=ms.vss-test-web.build-test-results-tab

the new test currently still fails - I think due to rasterio/rasterio#1709

cc @shoyer who created the issue linked in the first post.

@max-sixty
Copy link
Collaborator

It looks like the flakey test failed but that Azure counts it as passing: https://dev.azure.com/xarray/xarray/_build/results?buildId=389&view=logs

...similar to an allow_failures

@max-sixty
Copy link
Collaborator

@scottyhq would you prefer to merge this as-is as an improvement, or wait until the test is fixed?

@scottyhq
Copy link
Contributor Author

I'm in no rush to merge this (just wanted to help get rid of the red on Azure :). I think it's important to have network tests since it's increasingly common to be streaming data from s3, gcs, etc rather than reading locally. But I'm not sure of best practices for setting such tests up, so any suggestions for modifications are welcome.

@max-sixty
Copy link
Collaborator

OK great. Let's at least try and get this passing and turn it green.

Do you have a better idea of why it's still broken? Is it an issue with rasterio (re your comment above)? Is it fixed on master? If not we should open an issue there. If so we could install a later version on that build

@shoyer
Copy link
Member

shoyer commented Jul 26, 2019

The build is already green because this is marked as an allowed failure, but it shows up in the list of failing tests anyways. It's a little confusing but that was the best way I found to make the result of these tests noticeable without counting as a build failure.

If you click on "Checks" at the top of this PR and then click on the top level "pydata.xarray" section of Azure, you see the full details on why the test is failing still:

self = <xarray.tests.test_backends.TestRasterio object at 0x7fb4b5cfa358>

    @network
    def test_rasterio_vrt_network(self):
        # Make sure loading w/ rasterio give same results as xarray
        import rasterio
        # use same url that rasterio package uses in tests
        prefix = "https://landsat-pds.s3.amazonaws.com/L8/139/045/"
        image = "LC81390452014295LGN00/LC81390452014295LGN00_B1.TIF"
        httpstif = prefix + image
>       with rasterio.Env(aws_unsigned=True):

xarray/tests/test_backends.py:3797: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
/usr/share/miniconda/envs/xarray-tests/lib/python3.6/site-packages/rasterio/env.py:193: in __init__
    aws_unsigned=aws_unsigned)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <rasterio.session.AWSSession object at 0x7fb4b4b10da0>, session = None
aws_unsigned = True, aws_access_key_id = None, aws_secret_access_key = None
aws_session_token = None, region_name = None, profile_name = None
requester_pays = False

    def __init__(
            self, session=None, aws_unsigned=False, aws_access_key_id=None,
            aws_secret_access_key=None, aws_session_token=None,
            region_name=None, profile_name=None, requester_pays=False):
        """Create a new boto3 session
    
        Parameters
        ----------
        session : optional
            A boto3 session object.
        aws_unsigned : bool, optional (default: False)
            If True, requests will be unsigned.
        aws_access_key_id : str, optional
            An access key id, as per boto3.
        aws_secret_access_key : str, optional
            A secret access key, as per boto3.
        aws_session_token : str, optional
            A session token, as per boto3.
        region_name : str, optional
            A region name, as per boto3.
        profile_name : str, optional
            A shared credentials profile name, as per boto3.
        requester_pays : bool, optional
            True if the requester agrees to pay transfer costs (default:
            False)
        """
>       import boto3
E       ModuleNotFoundError: No module named 'boto3'

It looks like using the aws_unsigned argument means that boto3 is required, but we don't have that installed in our CI currently. It would be straightforward to add it whenever we install rasterio -- put it in the lists in ci/requirements/

@scottyhq
Copy link
Contributor Author

scottyhq commented Jul 30, 2019

ok. as noted rasterio/rasterio#1709 the aws_unsigned=True should bypass importing boto3 in upcoming rasterio 1.0.25. But I've gone ahead and added boto3 to ci requirements.yml files in case people want to add other tests reading from s3, gcs, etc. in the future

@shoyer shoyer merged commit 86799b7 into pydata:master Jul 31, 2019
@shoyer
Copy link
Member

shoyer commented Jul 31, 2019

Thanks @scottyhq !

dcherian added a commit to dcherian/xarray that referenced this pull request Aug 1, 2019
* master:
  More annotations in Dataset (pydata#3112)
  Hotfix for case of combining identical non-monotonic coords (pydata#3151)
  changed url for rasterio network test (pydata#3162)
dcherian added a commit to yohai/xarray that referenced this pull request Aug 3, 2019
* master: (68 commits)
  enable sphinx.ext.napoleon (pydata#3180)
  remove type annotations from autodoc method signatures (pydata#3179)
  Fix regression: IndexVariable.copy(deep=True) casts dtype=U to object (pydata#3095)
  Fix distributed.Client.compute applied to DataArray (pydata#3173)
  More annotations in Dataset (pydata#3112)
  Hotfix for case of combining identical non-monotonic coords (pydata#3151)
  changed url for rasterio network test (pydata#3162)
  to_zarr(append_dim='dim0') doesn't need mode='a' (pydata#3123)
  BUG: fix+test groupby on empty DataArray raises StopIteration (pydata#3156)
  Temporarily remove pynio from py36 CI build (pydata#3157)
  missing 'about' field (pydata#3146)
  Fix h5py version printing (pydata#3145)
  Remove the matplotlib=3.0 constraint from py36.yml (pydata#3143)
  disable codecov comments (pydata#3140)
  Merge broadcast_like docstrings, analyze implementation problem (pydata#3130)
  Update whats-new for pydata#3125 and pydata#2334 (pydata#3135)
  Fix tests on big-endian systems (pydata#3125)
  XFAIL tests failing on ARM (pydata#2334)
  Add broadcast_like. (pydata#3086)
  Better docs and errors about expand_dims() view (pydata#3114)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test_rasterio_vrt_network is failing in continuous integration tests
4 participants