scikit-hep / pyhf

pure-Python HistFactory implementation with tensors and autodiff
https://pyhf.readthedocs.io/
Apache License 2.0
279 stars 83 forks source link

fail.org exists and is for sale #1451

Closed kratsg closed 2 years ago

kratsg commented 3 years ago

Previously had an SSL error, now exists which is somewhat problematic. We should use https://httpstat.us/ or similar.

kratsg commented 3 years ago

The other option is to mock out the underlying requests to return the expected error, and only check that the rest of the code behaves as expected.

matthewfeickert commented 3 years ago

@kratsg This error isn't from what you say it is, given that if I rerun the CI on master right now everything passes. So as it is passing with

https://github.com/scikit-hep/pyhf/blob/671ed56483bc602876f611cf10ff9add7f4a2137/tests/test_scripts.py#L563-L568

then I don't think it is related to https://www.fail.org/ existing or not.

Here's an example as well that is just doing what pyhf.contrib.download is doing

Example:

$ docker run --rm -ti python:3.8 /bin/bash
# python -m pip install --upgrade --quiet pip setuptools wheel
# python -m pip install requests
# python
Python 3.8.10 (default, May  4 2021, 18:50:59) 
[GCC 8.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import requests
>>> requests.get("https://www.fail.org/record/resource/1234567")
Traceback (most recent call last):
  File "/usr/local/lib/python3.8/site-packages/urllib3/connectionpool.py", line 699, in urlopen
    httplib_response = self._make_request(
  File "/usr/local/lib/python3.8/site-packages/urllib3/connectionpool.py", line 382, in _make_request
    self._validate_conn(conn)
  File "/usr/local/lib/python3.8/site-packages/urllib3/connectionpool.py", line 1010, in _validate_conn
    conn.connect()
  File "/usr/local/lib/python3.8/site-packages/urllib3/connection.py", line 464, in connect
    _match_hostname(cert, self.assert_hostname or server_hostname)
  File "/usr/local/lib/python3.8/site-packages/urllib3/connection.py", line 512, in _match_hostname
    match_hostname(cert, asserted_hostname)
  File "/usr/local/lib/python3.8/ssl.py", line 416, in match_hostname
    raise CertificateError("hostname %r "
ssl.SSLCertVerificationError: ("hostname 'www.fail.org' doesn't match either of 'shortener.secureserver.net', 'www.shortener.secureserver.net'",)

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/local/lib/python3.8/site-packages/requests/adapters.py", line 439, in send
    resp = conn.urlopen(
  File "/usr/local/lib/python3.8/site-packages/urllib3/connectionpool.py", line 755, in urlopen
    retries = retries.increment(
  File "/usr/local/lib/python3.8/site-packages/urllib3/util/retry.py", line 574, in increment
    raise MaxRetryError(_pool, url, error or ResponseError(cause))
urllib3.exceptions.MaxRetryError: HTTPSConnectionPool(host='www.fail.org', port=443): Max retries exceeded with url: /record/resource/1234567 (Caused by SSLError(SSLCertVerificationError("hostname 'www.fail.org' doesn't match either of 'shortener.secureserver.net', 'www.shortener.secureserver.net'")))

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python3.8/site-packages/requests/api.py", line 76, in get
    return request('get', url, params=params, **kwargs)
  File "/usr/local/lib/python3.8/site-packages/requests/api.py", line 61, in request
    return session.request(method=method, url=url, **kwargs)
  File "/usr/local/lib/python3.8/site-packages/requests/sessions.py", line 542, in request
    resp = self.send(prep, **send_kwargs)
  File "/usr/local/lib/python3.8/site-packages/requests/sessions.py", line 655, in send
    r = adapter.send(request, **kwargs)
  File "/usr/local/lib/python3.8/site-packages/requests/adapters.py", line 514, in send
    raise SSLError(e, request=request)
requests.exceptions.SSLError: HTTPSConnectionPool(host='www.fail.org', port=443): Max retries exceeded with url: /record/resource/1234567 (Caused by SSLError(SSLCertVerificationError("hostname 'www.fail.org' doesn't match either of 'shortener.secureserver.net', 'www.shortener.secureserver.net'")))
kratsg commented 3 years ago

See this failure: https://github.com/scikit-hep/pyhf/runs/2571497115?check_suite_focus=true

matthewfeickert commented 3 years ago

See this failure: https://github.com/scikit-hep/pyhf/runs/2571497115?check_suite_focus=true

I'm confused though. This is a ConnectionResetError that doesn't seem to have anything to do with the code and doesn't show up again anywhere else in the runs on PR #1450

>       assert (
            "SSLCertificateError" and "hostname 'www.fail.org' doesn't match" in ret.stderr
        )
E       assert ('SSLCertificateError' and "hostname 'www.fail.org' doesn't match" in 'Traceback (most recent call last):\n  File "/opt/hostedtoolcache/Python/3.8.10/x64/lib/python3.8/site-packages/urllib...uests.exceptions.ConnectionError: (\'Connection aborted.\', ConnectionResetError(104, \'Connection reset by peer\'))\n')
E        +  where 'Traceback (most recent call last):\n  File "/opt/hostedtoolcache/Python/3.8.10/x64/lib/python3.8/site-packages/urllib...uests.exceptions.ConnectionError: (\'Connection aborted.\', ConnectionResetError(104, \'Connection reset by peer\'))\n' = <pytest_console_scripts.RunResult object at 0x7fae1ed21040>.stderr

Can you explain in like very basic terms why this needs to get changed? I'm missing how we're doing anything wrong.

kratsg commented 3 years ago

This happened because the site is not stable. You're relying on fail.org which is not owned and therefore you can't guarantee proper SSL connections or that it will work. You'll want to rely on something more stable, or otherwise the CI will fail randomly like it did in this case.

matthewfeickert commented 3 years ago

This happened because the site is not stable. You're relying on fail.org which is not owned and therefore you can't guarantee proper SSL connections or that it will work. You'll want to rely on something more stable, or otherwise the CI will fail randomly like it did in this case.

okay thanks. :+1: This seems pretty minor given that it isn't reproducible on a regular basis, so let's label this and kick it down the road until we're through things for v0.6.2 at least (assuming that this doesn't just start crashing the CI all the time :stuck_out_tongue:). Sound good?