Fix SSL Error #4825

nv-rliu · 2024-12-10T21:04:21Z

Addresses errors in wheel-tests seen here

Installs SSL certificates with certifi as outlined in this issue here

copy-pr-bot · 2024-12-10T21:04:25Z

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

nv-rliu · 2024-12-10T21:31:22Z

/ok to test

jameslamb

I'm supportive of using certifi certs to support the dataset-downloading you need to do on Rocky Linux 8, but don't think we should be adding a new runtime dependency to the cugraph Python package.

jameslamb · 2024-12-11T00:26:53Z

python/cugraph/cugraph/utilities/utils.py

@@ -549,3 +553,13 @@ def create_directory_with_overwrite(directory):
    if os.path.exists(directory):
        shutil.rmtree(directory)
    os.makedirs(directory)
+
+
+def install_ssl_cert():


Putting this here in cugraph.utilities means that now certifi needs to be introduced as a runtime dependency of cugraph.

ModuleNotFoundError: No module named 'certifi'

(build link)

I'm really nervous about the idea of introducing a new runtime dependency this long after code freeze and this close to the release.

And even just in general... does this really need to be in the cugraph package? Are those code paths in datasets.py intended to be used by downstream users, or are they just in the cugraph package for convenience in its own testing (and testing for cugraph-gnn libraries)?

Here in 24.12, to minimize risk, I think we should only run this at test time in CI here.

That'd probably mean:

make certifi a test-only dependency

put this bit of Python code in a script like ci/install-certifi-certs.py

run that in each ci/test_* script, before any tests

I agree that it is not a good time to add an additional runtime dependency..

The codepaths in datasets.py is used both by end users and for our own testing purposes. You can think of them as analogous to NetworkX's built-in graphs

I don't think that the SSL issue would affect our users. I think this is just affecting CI because of how our images are setup. It would probably be fine to go with the solution you suggested and install it for our tests. I'll go ahead and add that

Ok got it, thanks for the explanation! Then yeah, I think doing this only as a testing dependency is a good solution.

I tested whether this could just be run outside of of pytest, like I suggested... and you're absolutely right, it can't. I guess something in this solution must also modify os.environ or similar.

code I used to confirm that (click me)

docker run \ --rm \ -it rapidsai/citestwheel:cuda11.8.0-rockylinux8-py3.10 \ bash python -c 'import urllib.request; urllib.request.urlretrieve("https://data.rapids.ai/cugraph/results/resultsets.tar.gz", "foo.tar.gz")' # urllib.error.URLError: <urlopen error [SSL: CERTIFICATE_VERIFY_FAILED cat > ./install-ca-certs.py <<EOF import certifi from ssl import create_default_context from urllib.request import build_opener, HTTPSHandler, install_opener ssl_context = create_default_context(cafile=certifi.where()) https_handler = HTTPSHandler(context=ssl_context) install_opener(build_opener(https_handler)) EOF python -m pip install certifi python ./install-ca-certs.py python -c 'import urllib.request; urllib.request.urlretrieve("https://data.rapids.ai/cugraph/results/resultsets.tar.gz", "foo.tar.gz")' # urllib.error.URLError: <urlopen error [SSL: CERTIFICATE_VERIFY_FAILED

Given that, I think the way you've set this up as a testing-only dependency is a good solution. Thanks for working through it with me.

Thank you for helping track this down and brainstorming a solution with me.

jameslamb

I support this change, especially since you saw it fix the error locally. Thanks for finding a minimal reproducible example and then putting up a fix so quickly!

jameslamb · 2024-12-11T04:14:23Z

python/cugraph/cugraph/datasets/dataset.py

@@ -458,7 +459,7 @@ def download_all(force=False):
                filename = meta["name"] + meta["file_type"]
                save_to = default_download_dir.path / filename
                if not save_to.is_file() or force:
-                    urllib.request.urlretrieve(meta["url"], str(save_to))
+                    urlretrieve(meta["url"], str(save_to))


If you end up needing to push another change, I recommend reverting these import changes, which just look like stylistic changes to me (please correct me).

That'd make this whole file drop out of the diff, and make us even more confident that this is safe to merge this late in the release cycle.

But the changes look fine to me so don't push another commit and go through another round of CI just to revert this.

Hmm, I don't believe I have any more changes to push as of now. The PR is just about to pass CI so let me know if you think it'll be better to still revert the changes to have as minimal of a diff has possible. Thanks!

For those finding this from search... we talked offline and agreed that given the time-sensitivity of this, it wasn't worth another round of CI to revert this change.

bdice · 2024-12-11T14:47:46Z

dependencies.yaml

@@ -470,6 +470,7 @@ dependencies:
    common:
      - output_types: [conda, requirements]
        packages:
+          - certifi


@nv-rliu Can I ask why this was added to notebook dependencies? It looks like the package is only imported in tests, not notebooks.

Ah, my apologies. I thought that we needed this package in the notebook tests because we also use the datasets API in the notebooks a lot. It may not be necessary after all..

I don't know if "having certifi installed" is sufficient to avoid this issue or not. Were you seeing certificate failures on notebook jobs before? Are they resolved now?

You're right. Just having it installed doesn't actually do anything for the notebooks.

I am not seeing any SSL failures for the notebooks so we can remove this dependency. Should I go ahead and do that in 24.12 or should it be in 25.02?

25.02 please!

Last night, [wheel-tests-cuml / 11.8.0, 3.10, arm64, rockylinux8, a100, latest-driver, oldest-deps](https://github.com/rapidsai/cuml/actions/runs/12273029767/job/34243006376#logs) failed. The failure occurred in `testing/dask/utils.py`'s `load_text_corpus`: https://github.com/rapidsai/cuml/blob/052cddef9648c3266974a0b43970afb347ba9d01/python/cuml/cuml/testing/dask/utils.py#L12 The errors looked like: ``` FAILED test_dask_naive_bayes.py::test_basic_fit_predict - urllib.error.URLError: <urlopen error [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: unable to get local issuer certificate (_ssl.c:1007)> FAILED test_dask_naive_bayes.py::test_single_distributed_exact_results - urllib.error.URLError: <urlopen error [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: unable to get local issuer certificate (_ssl.c:1007)> FAILED test_dask_naive_bayes.py::test_score - urllib.error.URLError: <urlopen error [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: unable to get local issuer certificate (_ssl.c:1007)> ``` This adopts some fixes from rapidsai/cugraph#4825 that will hopefully help with SSL certificate failures.

Addresses errors in `wheel-tests` seen [here](rapidsai#4818) Installs SSL certificates with `certifi` as outlined in this issue [here](rapidsai/build-infra#56)

Add SSL certs from certifi

bfcf422

github-actions bot added the python label Dec 10, 2024

nv-rliu added this to the 24.12 milestone Dec 10, 2024

nv-rliu added bug Something isn't working breaking Breaking change labels Dec 10, 2024

Add SSL cert installation to resultset

f3d896a

nv-rliu changed the base branch from branch-24.12 to branch-25.02 December 10, 2024 21:30

nv-rliu requested a review from jameslamb December 10, 2024 21:36

nv-rliu changed the base branch from branch-25.02 to branch-24.12 December 10, 2024 21:47

nv-rliu marked this pull request as ready for review December 10, 2024 21:48

nv-rliu requested a review from a team as a code owner December 10, 2024 21:48

jameslamb requested changes Dec 11, 2024

View reviewed changes

Move installing SSL code to conftest.py

ba252cc

nv-rliu requested a review from a team as a code owner December 11, 2024 03:07

nv-rliu requested review from jameslamb and removed request for a team December 11, 2024 03:07

github-actions bot added the conda label Dec 11, 2024

jameslamb approved these changes Dec 11, 2024

View reviewed changes

raydouglass merged commit 2d32c86 into rapidsai:branch-24.12 Dec 11, 2024
71 of 73 checks passed

bdice reviewed Dec 11, 2024

View reviewed changes

nv-rliu deleted the b24.12-fix-ssl-error branch December 11, 2024 14:55

bdice mentioned this pull request Dec 11, 2024

Fix SSL error. rapidsai/cuml#6177

Merged

bdice mentioned this pull request Dec 15, 2024

Retry on CondaSSLError. rapidsai/gha-tools#127

Merged

This was referenced Dec 16, 2024

Add certifi to CI images rapidsai/ci-imgs#218

Merged

[BUG]: 24.12 nightly wheel tests failing with SSL certificate issues #4818

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix SSL Error #4825

Fix SSL Error #4825

nv-rliu commented Dec 10, 2024 •

edited

Loading

copy-pr-bot bot commented Dec 10, 2024

nv-rliu commented Dec 10, 2024

jameslamb left a comment

jameslamb Dec 11, 2024

nv-rliu Dec 11, 2024

jameslamb Dec 11, 2024

nv-rliu Dec 11, 2024

jameslamb left a comment

jameslamb Dec 11, 2024

nv-rliu Dec 11, 2024

jameslamb Dec 12, 2024

bdice Dec 11, 2024

nv-rliu Dec 11, 2024

bdice Dec 12, 2024

nv-rliu Dec 12, 2024

bdice Dec 12, 2024

Fix SSL Error #4825

Fix SSL Error #4825

Conversation

nv-rliu commented Dec 10, 2024 • edited Loading

copy-pr-bot bot commented Dec 10, 2024

nv-rliu commented Dec 10, 2024

jameslamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jameslamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nv-rliu commented Dec 10, 2024 •

edited

Loading