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.

Sign up for GitHub

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

Fix SSL Error #4825

Merged
merged 3 commits into from
Dec 11, 2024
Merged

Fix SSL Error #4825

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bfcf422
Add SSL certs from certifi
nv-rliu Dec 10, 2024
f3d896a
Add SSL cert installation to resultset
nv-rliu Dec 10, 2024
ba252cc
Move installing SSL code to conftest.py
nv-rliu Dec 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions conda/environments/all_cuda-118_arch-x86_64.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ dependencies:
- aiohttp
- breathe
- c-compiler
- certifi
- cmake>=3.26.4,!=3.30.0
- cuda-nvtx
- cuda-version=11.8
Expand Down
1 change: 1 addition & 0 deletions conda/environments/all_cuda-125_arch-x86_64.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ dependencies:
- aiohttp
- breathe
- c-compiler
- certifi
- cmake>=3.26.4,!=3.30.0
- cuda-cudart-dev
- cuda-nvcc
Expand Down
2 changes: 2 additions & 0 deletions dependencies.yaml
100644 → 100755
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,6 +470,7 @@ dependencies:
common:
- output_types: [conda, requirements]
packages:
- certifi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

25.02 please!

- ipython
- notebook>=0.5.0
- output_types: [conda]
Expand All @@ -489,6 +490,7 @@ dependencies:
common:
- output_types: [conda, pyproject]
packages:
- certifi
- networkx>=2.5.1
- *numpy
- python-louvain
Expand Down
1 change: 1 addition & 0 deletions python/cugraph-service/server/pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ cugraph-service-server = "cugraph_service_server.__main__:main"

[project.optional-dependencies]
test = [
"certifi",
"networkx>=2.5.1",
"numpy>=1.23,<3.0a0",
"pandas",
Expand Down
15 changes: 8 additions & 7 deletions python/cugraph/cugraph/datasets/dataset.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,15 @@
# See the License for the specific language governing permissions and
# limitations under the License.

import cudf
import dask_cudf
import yaml
import os
import pandas as pd
import cugraph.dask as dcg
import yaml
from pathlib import Path
import urllib.request
from urllib.request import urlretrieve

import cudf
import cugraph.dask as dcg
import dask_cudf
from cugraph.structure.graph_classes import Graph


Expand Down Expand Up @@ -142,7 +143,7 @@ def __download_csv(self, url):
filename = self.metadata["name"] + self.metadata["file_type"]
if self._dl_path.path.is_dir():
self._path = self._dl_path.path / filename
urllib.request.urlretrieve(url, str(self._path))
urlretrieve(url, str(self._path))

else:
raise RuntimeError(
Expand Down Expand Up @@ -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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.



def set_download_dir(path):
Expand Down
3 changes: 0 additions & 3 deletions python/cugraph/cugraph/testing/resultset.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@

import warnings
import tarfile

import urllib.request

import cudf
Expand All @@ -22,8 +21,6 @@
default_download_dir,
)

# results_dir_path = utils.RAPIDS_DATASET_ROOT_DIR_PATH / "tests" / "resultsets"


class Resultset:
"""
Expand Down
13 changes: 12 additions & 1 deletion python/cugraph/cugraph/tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,20 @@
# Avoid timeout during shutdown
from dask_cuda.utils_test import IncreasedCloseTimeoutNanny

# module-wide fixtures
import certifi
from ssl import create_default_context
from urllib.request import build_opener, HTTPSHandler, install_opener


# Install SSL certificates
def pytest_sessionstart(session):
ssl_context = create_default_context(cafile=certifi.where())
https_handler = HTTPSHandler(context=ssl_context)
install_opener(build_opener(https_handler))


# module-wide fixtures

# Spoof the gpubenchmark fixture if it's not available so that asvdb and
# rapids-pytest-benchmark do not need to be installed to run tests.
if "gpubenchmark" not in globals():
Expand Down
1 change: 1 addition & 0 deletions python/cugraph/pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ classifiers = [

[project.optional-dependencies]
test = [
"certifi",
"networkx>=2.5.1",
"numpy>=1.23,<3.0a0",
"pandas",
Expand Down
Loading