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

test-upstream #5267

Merged
merged 8 commits into from Aug 15, 2019

Conversation

@TomAugspurger
Copy link
Member

commented Aug 13, 2019

No description provided.

@TomAugspurger

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

@martindurant the following test has changed between fsspec 0.4.1 and master

$  pytest dask/bytes/tests/test_http.py::test_errors
================================================================================================================ FAILURES =================================================================================================================
_______________________________________________________________________________________________________________ test_errors _______________________________________________________________________________________________________________

dir_server = '/var/folders/hz/f43khqfn7b1b1g8z_z6y3bsw0000gp/T/tmp5_o8megr'

    def test_errors(dir_server):
        f = open_files("http://localhost:8999/doesnotexist")[0]
        with pytest.raises(requests.exceptions.RequestException):
            with f as f:
                f.read()
        f = open_files("http://nohost/")[0]
        with pytest.raises(requests.exceptions.RequestException):
>           with f as f:

dask/bytes/tests/test_http.py:123:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
../../miniconda3/envs/dask-upstream-dev/lib/python3.7/site-packages/fsspec/core.py:65: in __enter__
    f = self.fs.open(self.path, mode=mode)
../../miniconda3/envs/dask-upstream-dev/lib/python3.7/site-packages/fsspec/spec.py:684: in open
    autocommit=ac, **kwargs)
../../miniconda3/envs/dask-upstream-dev/lib/python3.7/site-packages/fsspec/implementations/http.py:133: in _open
    return HTTPFile(self, url, self.session, block_size, **kw)
../../miniconda3/envs/dask-upstream-dev/lib/python3.7/site-packages/fsspec/implementations/http.py:204: in __init__
    cache_type=cache_type, **kwargs)
../../miniconda3/envs/dask-upstream-dev/lib/python3.7/site-packages/fsspec/spec.py:879: in __init__
    self.details = fs.info(path)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <fsspec.implementations.http.HTTPFileSystem object at 0x1141600b8>, url = 'http://nohost/', kwargs = {}, size = False, policy = 'get'

    def info(self, url, **kwargs):
        """Get info of URL

        Tries to access location via HEAD, and then GET methods, but does
        not fetch the data.

        It is possible that the server does not supply any size information, in
        which case size will be given as None (and certain operations on the
        corresponding file will not work).
        """
        size = False
        for policy in ['head', 'get']:
            try:
                size = file_size(url, self.session, policy, **self.kwargs)
                if size is not None:
                    break
            except Exception:
                pass
        else:
            # get failed, so conclude URL does not exist
            if size is False:
>               raise FileNotFoundError(url)
E               FileNotFoundError: http://nohost/

../../miniconda3/envs/dask-upstream-dev/lib/python3.7/site-packages/fsspec/implementations/http.py:166: FileNotFoundError

Is that a deliberate change (possibly from intake/filesystem_spec#93)? Will we need to use fsspec's http server for this test?

@TomAugspurger

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

Err sorry, I should look more closely at the test. We should just expect a FileNotFoundError, right?

@martindurant

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

FileNotFound is, I think, the right thing to expect here

@TomAugspurger

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

OK, I think the next failure looks more real.

$ pytest dask/bytes/tests/test_s3.py::test_compression[bz2-None] 

It seems like read_bytes('s3://foo/*', compression='bz2', blocksize=None) ends up trying to pass a local file path named 's3://foo/...' to bs2.Bz2File, which fails.

@TomAugspurger

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

intake/filesystem_spec#104 for the orc failure.

@TomAugspurger

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

Ah, it turns out the test_s3.py::test_compression is failing due to intake/filesystem_spec#104 as well.

@TomAugspurger

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

@mrocklin 8a732ae fixes some upstream failures from distributed's stricter(?) checking of lingering threads / processes like

dask/tests/test_distributed.py:61: 
1685_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
1686../../../miniconda/envs/test-environment/lib/python3.7/contextlib.py:119: in __exit__
1687    next(self.gen)
1688../../../miniconda/envs/test-environment/lib/python3.7/site-packages/distributed/utils_test.py:721: in cluster
1689    client.close()
1690../../../miniconda/envs/test-environment/lib/python3.7/contextlib.py:119: in __exit__
1691    next(self.gen)
1692../../../miniconda/envs/test-environment/lib/python3.7/site-packages/distributed/utils_test.py:1544: in clean
1693    del thread_state.on_event_loop_thread
1694../../../miniconda/envs/test-environment/lib/python3.7/contextlib.py:119: in __exit__
1695    next(self.gen)
1696_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
1697
1698check = True
1699
1700    @contextmanager
1701    def check_process_leak(check=True):
1702        for proc in mp_context.active_children():
1703            proc.terminate()
1704    
1705        yield
1706    
1707        if check:
1708            for i in range(100):
1709                if not set(mp_context.active_children()):
1710                    break
1711                else:
1712                    sleep(0.2)
1713            else:
1714>               assert not mp_context.active_children()
1715E               assert not [<SpawnProcess(SpawnPoolWorker-68, started daemon)>, <SpawnProcess(SpawnPoolWorker-70, started daemon)>, <SpawnProcess... daemon)>, <SpawnProcess(SpawnPoolWorker-69, started daemon)>, <SpawnProcess(SpawnPoolWorker-71, started daemon)>, ...]
1716E                +  where [<SpawnProcess(SpawnPoolWorker-68, started daemon)>, <SpawnProcess(SpawnPoolWorker-70, started daemon)>, <SpawnProcess... daemon)>, <SpawnProcess(SpawnPoolWorker-69, started daemon)>, <SpawnProcess(SpawnPoolWorker-71, started daemon)>, ...] = <function active_children at 0x7fd9935bdbf8>()
1717E                +    where <function active_children at 0x7fd9935bdbf8> = <multiprocessing.context.ForkServerContext object at 0x7fd993546da0>.active_children
1718
1719../../../miniconda/envs/test-environment/lib/python3.7/site-packages/distributed/utils_test.py:1463: AssertionError
1720

The failing tests weren't the ones that needed updating though. It was the pool from a different test that needed closing, which made debugging difficult. Do you think it's possible for check_process_leak to only check for new processes / threads? Or is that hopeless?

@jakirkham

This comment has been minimized.

Copy link
Member

commented Aug 15, 2019

@mrocklin, any thoughts on Tom's question above?

@mrocklin

This comment has been minimized.

Copy link
Member

commented Aug 15, 2019

Do you think it's possible for check_process_leak to only check for new processes

Currently we close all processes when entering the context manager. We assume that people are using the mp_context from within utils.py

We don't do this for threads, but I'm not sure that we can.

@TomAugspurger

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2019

FYI: there's still one upstream dev failure I need to debug: https://travis-ci.org/dask/dask/jobs/571906205#L1208

@TomAugspurger

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2019

All passing now.

@jrbourbeau

This comment has been minimized.

Copy link
Member

commented Aug 15, 2019

Thanks for handling this @TomAugspurger. Merging

@jrbourbeau jrbourbeau merged commit 2445933 into dask:master Aug 15, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.