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.

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

Fetchers docstring examples trigger dataset fetch in CI #28707

Closed
jeremiedbb opened this issue Mar 27, 2024 · 4 comments · Fixed by #28709
Closed

Fetchers docstring examples trigger dataset fetch in CI #28707

jeremiedbb opened this issue Mar 27, 2024 · 4 comments · Fixed by #28709

Comments

@jeremiedbb
Copy link
Member

Docstring examples were recently added to the fetchers. This makes the doc tests executed by pytest actually fetch the datasets.
In the fetcher tests we took some precaution to not fetch the real datasets, see

def mock_data_home(tmp_path_factory):

It has a significant impact on the duration of the test suite (and probably on memory usage as well)

27.75s call     ::__init__.py::_lfw.py::sklearn.datasets._lfw.fetch_lfw_pairs
20.98s call     ::__init__.py::_lfw.py::sklearn.datasets._lfw.fetch_lfw_people
@lesteve
Copy link
Member

lesteve commented Mar 27, 2024

This is probably doable to skip doctests in sklearn/conftest.py pytest_collection_modifyitems based on SKLEARN_SKIP_NETWORK_TESTS.

@glemaitre
Copy link
Member

Uhm normally, I almost sure to have written some code to avoid this using some fixture.

@glemaitre
Copy link
Member

@glemaitre
Copy link
Member

And indeed, the fetch_lfw_pairs is not added as a fixture here:

dataset_fetchers = {
"fetch_20newsgroups_fxt": fetch_20newsgroups,
"fetch_20newsgroups_vectorized_fxt": fetch_20newsgroups_vectorized,
"fetch_california_housing_fxt": fetch_california_housing,
"fetch_covtype_fxt": fetch_covtype,
"fetch_kddcup99_fxt": fetch_kddcup99,
"fetch_olivetti_faces_fxt": fetch_olivetti_faces,
"fetch_rcv1_fxt": fetch_rcv1,
"fetch_species_distributions_fxt": fetch_species_distributions,
}

So this is a simple fix.

/take

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants