SNEWS2 / snewpy

A Python package for working with supernova neutrinos
https://snewpy.readthedocs.io
BSD 3-Clause "New" or "Revised" License
26 stars 19 forks source link

Problem downloading models #109

Closed apizzuto closed 3 years ago

apizzuto commented 3 years ago

Hi all, this is part of https://github.com/openjournals/joss-reviews/issues/3772. I was trying to install the code, and ran into some problems obtaining the models. I'm not sure if this is a bug or permissions issue, or if I misunderstood the install directions in the README, but here is a MWE reproducing the error:

MBP-C02F714BQ05Q:snewpy apizzuto$ python3 -m venv snewpy-venv
MBP-C02F714BQ05Q:snewpy apizzuto$ source snewpy-venv/bin/activate
(snewpy-venv) MBP-C02F714BQ05Q:snewpy apizzuto$ pip install snewpy
 . . . 
(snewpy-venv) MBP-C02F714BQ05Q:snewpy apizzuto$ python -c 'import snewpy; snewpy.get_models()'
Available models in this version of SNEWPY: ['Bollig_2016', 'Fornax_2019', 'Fornax_2021', 'Kuroda_2020', 'Nakazato_2013', 'OConnor_2013', 'OConnor_2015', 'PISN', 'Sukhbold_2015', 'Tamborra_2014', 'Type_Ia', 'Walk_2018', 'Walk_2019', 'Warren_2020', 'Zha_2021']

Type a model name, 'all' to download all models or <Enter> to cancel: all
. . . 

and then I get a bunch of messages along the lines of Failed to download https://github.com/SNEWS2/snewpy/raw/v1.1b3/models/{model_name}/README.md to 'SNEWPY_models/{model_name}/{file_name}'. After those outputs I get ERROR: 719 exceptions occured. ([URLError(SSLCertVerificationError(1, '[SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: unable to get local issuer certificate (_ssl.c:1129)')) . . . (I trimmed the entire message because it lists out all 719 errors). I have a subdirectory called SNEWPY_models now with all of the model names as subdirectories, but those subdirectories seem to be empty

apizzuto commented 3 years ago

It looks like a way around this that doesn't introduce any dependencies would be adding import ssl with the other imports and then in the except block in https://github.com/SNEWS2/snewpy/blob/3e378d397fddad658775d8a9e39bd514d712915c/python/snewpy/__init__.py#L66 adding a

if (not os.environ.get('PYTHONHTTPSVERIFY', '') and getattr(ssl, '_create_unverified_context', None)):
    ssl._create_default_https_context = ssl._create_unverified_context

Happy to submit a pull request if the developers are happy with this patch

JostMigenda commented 3 years ago

Hi @apizzuto, thanks for opening this issue!

Unfortunately, I can’t reproduce this—downloading the model files works just fine for me (and also when running it as part of our release workflow)—so I suspect there may be a configuration issue. Normally, as far as I understand, Python will check the system’s certificate store to verify the certificate presented by github.com before downloading the file. If this doesn’t work, my initial guesses would be that either your OS’s certificate store is out of date or Python is for some reason not able to access it. Can you let us know what Python and OS version you’re using?

Re the proposed patch: PYTHONHTTPSVERIFY should only be present in Python 2.7.x, while SNEWPY requires 3.x; so that shouldn’t be necessary. Either changing the default https context (as you suggest), or explicitly passing the context as an argument should work.

However, disabling certificate verification is really not a good idea in production use, so I’d be extremely hesitant to do this. I’d much rather understand why your Python can’t verify GitHub’s TLS certificate and fix that underlying problem; not turn off this security feature entirely.

apizzuto commented 3 years ago

Hi Jost, thanks for looking into this! I'm happy to close this issue because after your comment I think that it's my machine (OS: MacOS 11.2.3, python3.9.5). I recently started working on an M1 mac and it seems like issues like this always seem to crop up. I tried the install on my previous machine (OS: macOS 10.15.3 and python3.8.5) and everything worked completely fine. I'll finish up the review on that machine or I'll add the context as an argument on my M1 and continue the review there. Thanks for being so prompt with this response!