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

Try to reduce network usage in cuML tests. #6174

Merged
merged 4 commits into from
Dec 12, 2024
Merged

Try to reduce network usage in cuML tests. #6174

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0833887
Try to reduce network usage in cuML tests.
bdice Dec 10, 2024
bf4ac51
Merge branch 'branch-25.02' into reduce-network-usage
bdice Dec 11, 2024
6ed322a
Add retry logic.
bdice Dec 11, 2024
c38a40b
Merge branch 'branch-25.02' into reduce-network-usage
bdice 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
36 changes: 26 additions & 10 deletions python/cuml/cuml/tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,13 @@
from ssl import create_default_context
from urllib.request import build_opener, HTTPSHandler, install_opener
import certifi
import functools
import hypothesis
from cuml.internals.safe_imports import gpu_only_import
import pytest
import os
import subprocess
import time
import pandas as pd
import cudf.pandas

Expand Down Expand Up @@ -212,7 +214,7 @@ def pytest_pyfunc_call(pyfuncitem):
pytest.skip("Test requires cudf.pandas accelerator")


@pytest.fixture(scope="module")
@pytest.fixture(scope="session")
def nlp_20news():
try:
twenty_train = fetch_20newsgroups(
Expand All @@ -228,7 +230,7 @@ def nlp_20news():
return X, Y


@pytest.fixture(scope="module")
@pytest.fixture(scope="session")
def housing_dataset():
try:
data = fetch_california_housing()
Expand All @@ -245,16 +247,30 @@ def housing_dataset():
return X, y, feature_names


@pytest.fixture(scope="module")
@functools.cache
def get_boston_data():
n_retries = 3
url = "https://raw.githubusercontent.com/scikit-learn/scikit-learn/baf828ca126bcb2c0ad813226963621cafe38adb/sklearn/datasets/data/boston_house_prices.csv" # noqa: E501
for _ in range(n_retries):
try:
return pd.read_csv(url, header=None)
except Exception:
time.sleep(1)
raise RuntimeError(
f"Failed to download file from {url} after {n_retries} retries."
)


@pytest.fixture(scope="session")
def deprecated_boston_dataset():
# dataset was removed in Scikit-learn 1.2, we should change it for a
# better dataset for tests, see
# https://github.com/rapidsai/cuml/issues/5158

df = pd.read_csv(
"https://raw.githubusercontent.com/scikit-learn/scikit-learn/baf828ca126bcb2c0ad813226963621cafe38adb/sklearn/datasets/data/boston_house_prices.csv",
header=None,
) # noqa: E501
try:
df = get_boston_data()
except: # noqa E722
pytest.xfail(reason="Error fetching Boston housing dataset")
Copy link
Member

Choose a reason for hiding this comment

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

Is there something else we can do here? My feeling is that by xfailing the test we are kinda saying "this will be a test that runs sometimes, but often it won't run. Eventually the test will break but no one will notice because it hardly ever runs. Until one day long after the change that broke the test when it will run and fail. Now someone from the future will have to scratch their head about what the heck just happened to them".

I am not sure what a good solution would be. Here some ideas:

  • we now cache the dataset so should run into rate limits less often, so just deal with the occasional failure
  • delete all the tests that use boston
  • switch all the tests that use boston to some newer dataset (like california?)
  • host the boston CSV somewhere we can more reliably fetch it from

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I chose the first (deal with the occasional failure) because that strategy is used elsewhere in the same tests for robustness. However, the other tests will retry network failures three times. I could do that here too if desired but I didn’t want to make the code more complex.

If the team desires something more substantive like a change in test data or hosting, I would recommend adopting that in a larger-scale rewrite of these tests. We must increase the robustness of our CI, even if it comes at a cost of coverage (if a download fails, just move on). It has been over 130 days since nightly CI passed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is an open issue about switching away from the Boston dataset, #5158, so it’d be great if you want to give that a shot!

Copy link
Member

Choose a reason for hiding this comment

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

Can you remove the pytest.xfail then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think "just deal with the occasional failure" should come at a cost to coverage, and not at a cost to CI pass/fail. CI robustness (signal/noise) is too low to be useful at present, and we need to have occasional passes for nightly CI to be of any value.

There is already an open issue to move away from this long-deprecated dataset, and I would recommend that if the test is xfailing too consistently. In the meantime I can match the same behavior used for other datasets: retry the download 3 times, cache the file locally, and xfail the test if the download does not succeed.

Copy link
Contributor Author

@bdice bdice Dec 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I added retry logic in 6ed322a. I now expect this test to be equally robust as the native sklearn fetch_* methods (aside from what is probably a different data host, GitHub vs. wherever fetch_* data comes from).

n_samples = int(df[0][0])
data = df[list(np.arange(13))].values[2:n_samples].astype(np.float64)
targets = df[13].values[2:n_samples].astype(np.float64)
Expand All @@ -266,7 +282,7 @@ def deprecated_boston_dataset():


@pytest.fixture(
scope="module",
scope="session",
params=["digits", "deprecated_boston_dataset", "diabetes", "cancer"],
)
def test_datasets(request, deprecated_boston_dataset):
Expand Down Expand Up @@ -313,7 +329,7 @@ def failure_logger(request):
print(error_msg)


@pytest.fixture(scope="module")
@pytest.fixture(scope="session")
def exact_shap_regression_dataset():
return create_synthetic_dataset(
generator=skl_make_reg,
Expand All @@ -326,7 +342,7 @@ def exact_shap_regression_dataset():
)


@pytest.fixture(scope="module")
@pytest.fixture(scope="session")
def exact_shap_classification_dataset():
return create_synthetic_dataset(
generator=skl_make_clas,
Expand Down
Loading