-
Notifications
You must be signed in to change notification settings - Fork 915
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
Reduce pytest runtime #10203
Reduce pytest runtime #10203
Changes from 8 commits
4d7ae60
1f9e36d
70932d5
b94cce4
67e3994
e05d8bc
f367e01
9e37504
d298d4a
6e9a0ef
e4a98d0
289a13b
2e25ed2
9393bd0
a20102e
9c800cd
025c69d
60a0a92
6128b2a
227be3c
ecdb986
e3b6500
62d56be
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
# Copyright (c) 2020-2021, NVIDIA CORPORATION. | ||
|
||
import itertools | ||
import re | ||
import warnings | ||
from collections.abc import Mapping, Sequence | ||
|
@@ -330,3 +331,9 @@ def does_not_raise(): | |
|
||
def xfail_param(param, **kwargs): | ||
return pytest.param(param, marks=pytest.mark.xfail(**kwargs)) | ||
|
||
|
||
deduped_numeric_dtype_tests = pytest.mark.parametrize( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Naming nit: maybe |
||
"left_dtype,right_dtype", | ||
list(itertools.combinations_with_replacement(NUMERIC_TYPES, 2)), | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,7 @@ | |
import decimal | ||
import operator | ||
import random | ||
from itertools import product | ||
from itertools import combinations_with_replacement, product | ||
|
||
import cupy as cp | ||
import numpy as np | ||
|
@@ -217,10 +217,11 @@ def test_series_compare(cmpop, obj_class, dtype): | |
|
||
def _series_compare_nulls_typegen(): | ||
tests = [] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another way to write this would be: return [
*combinations_with_replacement(DATETIME_TYPES, 2),
*combinations_with_replacement(TIMEDELTA_TYPES, 2),
*combinations_with_replacement(NUMERIC_TYPES, 2),
*combinations_with_replacement(STRING_TYPES, 2),
] However you prefer is fine. |
||
tests += list(product(DATETIME_TYPES, DATETIME_TYPES)) | ||
tests += list(product(TIMEDELTA_TYPES, TIMEDELTA_TYPES)) | ||
tests += list(product(NUMERIC_TYPES, NUMERIC_TYPES)) | ||
tests += list(product(STRING_TYPES, STRING_TYPES)) | ||
|
||
tests += list(combinations_with_replacement(DATETIME_TYPES, 2)) | ||
tests += list(combinations_with_replacement(DATETIME_TYPES, 2)) | ||
brandon-b-miller marked this conversation as resolved.
Show resolved
Hide resolved
|
||
tests += list(combinations_with_replacement(NUMERIC_TYPES, 2)) | ||
tests += list(combinations_with_replacement(STRING_TYPES, 2)) | ||
|
||
return tests | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6670,7 +6670,6 @@ def test_dataframe_info_null_counts(): | |
"data1", | ||
[ | ||
[1, 2, 3, 4, 5, 6, 7], | ||
[1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0], | ||
[ | ||
1.9876543, | ||
2.9876654, | ||
|
@@ -6689,31 +6688,12 @@ def test_dataframe_info_null_counts(): | |
-6.88918237, | ||
-7.00001, | ||
], | ||
[ | ||
1.987654321, | ||
2.987654321, | ||
3.987654321, | ||
0.1221, | ||
2.1221, | ||
0.112121, | ||
-21.1212, | ||
], | ||
[ | ||
-1.987654321, | ||
-2.987654321, | ||
-3.987654321, | ||
-0.1221, | ||
-2.1221, | ||
-0.112121, | ||
21.1212, | ||
], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here I am just deleting tests that might be redundant. I am basing this off the assumption that all these different sets of numbers are not really increasing test coverage. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For some values of rtol and atol, the deleted values seem to be testing the behavior of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAICT the these tests were only covering There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I wasn't very clear. The issue isn't really about data types. It's about the rtol / atol precision requiring If you look at the I agree that this test is grossly over-parametrized, but the deletions here are removing an important case to check. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's just undo these changes and add a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that would be good. I can file an issue describing what I've discussed with @brandon-b-miller via Slack. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good to me. Thank you, Bradley! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Issue #10284 filed. I can put this in my backlog of things to do, or I can help someone else construct the specific cases I have in mind for test coverage. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change has been reverted |
||
], | ||
) | ||
@pytest.mark.parametrize( | ||
"data2", | ||
[ | ||
[1, 2, 3, 4, 5, 6, 7], | ||
[1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0], | ||
[ | ||
1.9876543, | ||
2.9876654, | ||
|
@@ -6732,24 +6712,6 @@ def test_dataframe_info_null_counts(): | |
-6.88918237, | ||
-7.00001, | ||
], | ||
[ | ||
1.987654321, | ||
2.987654321, | ||
3.987654321, | ||
0.1221, | ||
2.1221, | ||
0.112121, | ||
-21.1212, | ||
], | ||
[ | ||
-1.987654321, | ||
-2.987654321, | ||
-3.987654321, | ||
-0.1221, | ||
-2.1221, | ||
-0.112121, | ||
21.1212, | ||
], | ||
], | ||
) | ||
@pytest.mark.parametrize("rtol", [0, 0.01, 1e-05, 1e-08, 5e-1, 50.12]) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1294,8 +1294,8 @@ def test_loc_datetime_index(sli, is_dataframe): | |
@pytest.mark.parametrize( | ||
bdice marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"gdf", | ||
[ | ||
cudf.DataFrame({"a": range(1000000)}), | ||
cudf.DataFrame({"a": range(1000000), "b": range(1000000)}), | ||
cudf.DataFrame({"a": range(100000)}), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we remove the construction of GPU objects from the @pytest.mark.parametrize(
"gdf_kwargs",
[
dict(data={"a": range(100000)}),
dict(data={"a": range(100000), "b": range(100000)}),
# ...
dict(index=[1, 2, 3]),
# ...
],
) then: def test_dataframe_sliced(gdf_kwargs, slice):
gdf = cudf.DataFrame(**gdf_kwargs)
pdf = gdf.to_pandas()
# ... |
||
cudf.DataFrame({"a": range(100000), "b": range(100000)}), | ||
cudf.DataFrame({"a": range(20), "b": range(20)}), | ||
brandon-b-miller marked this conversation as resolved.
Show resolved
Hide resolved
|
||
cudf.DataFrame( | ||
{ | ||
|
@@ -1305,25 +1305,23 @@ def test_loc_datetime_index(sli, is_dataframe): | |
} | ||
), | ||
cudf.DataFrame(index=[1, 2, 3]), | ||
cudf.DataFrame(index=range(1000000)), | ||
cudf.DataFrame(index=range(100000)), | ||
cudf.DataFrame(columns=["a", "b", "c", "d"]), | ||
cudf.DataFrame(columns=["a"], index=range(1000000)), | ||
cudf.DataFrame( | ||
columns=["a", "col2", "...col n"], index=range(1000000) | ||
), | ||
cudf.DataFrame(index=cudf.Series(range(1000000)).astype("str")), | ||
cudf.DataFrame(columns=["a"], index=range(100000)), | ||
cudf.DataFrame(columns=["a", "col2", "...col n"], index=range(100000)), | ||
cudf.DataFrame(index=cudf.Series(range(100000)).astype("str")), | ||
cudf.DataFrame( | ||
columns=["a", "b", "c", "d"], | ||
index=cudf.Series(range(1000000)).astype("str"), | ||
index=cudf.Series(range(100000)).astype("str"), | ||
), | ||
], | ||
) | ||
@pytest.mark.parametrize( | ||
"slice", | ||
[ | ||
slice(250000, 500000), | ||
slice(250000, 250001), | ||
slice(500000), | ||
slice(25000, 50000), | ||
slice(25000, 25001), | ||
slice(50000), | ||
slice(1, 10), | ||
slice(10, 20), | ||
slice(15, 24000), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1105,9 +1105,9 @@ def test_parquet_reader_list_large_multi_rowgroup_nulls(tmpdir): | |
assert_eq(expect, got) | ||
|
||
|
||
@pytest.mark.parametrize("skip", range(0, 128)) | ||
@pytest.mark.parametrize("skip", range(0, 10)) | ||
def test_parquet_reader_list_skiprows(skip, tmpdir): | ||
num_rows = 128 | ||
num_rows = 10 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here I reason that that if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe even replace |
||
src = pd.DataFrame( | ||
{ | ||
"a": list_gen(int_gen, 0, num_rows, 80, 50), | ||
|
@@ -1124,9 +1124,9 @@ def test_parquet_reader_list_skiprows(skip, tmpdir): | |
assert_eq(expect, got, check_dtype=False) | ||
|
||
|
||
@pytest.mark.parametrize("skip", range(0, 120)) | ||
@pytest.mark.parametrize("skip", range(0, 10)) | ||
def test_parquet_reader_list_num_rows(skip, tmpdir): | ||
num_rows = 128 | ||
num_rows = 20 | ||
src = pd.DataFrame( | ||
{ | ||
"a": list_gen(int_gen, 0, num_rows, 80, 50), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,7 +13,7 @@ | |
from cudf.testing import _utils as utils | ||
bdice marked this conversation as resolved.
Show resolved
Hide resolved
|
||
from cudf.utils.dtypes import np_dtypes_to_pandas_dtypes | ||
|
||
repr_categories = utils.NUMERIC_TYPES + ["str", "category", "datetime64[ns]"] | ||
repr_categories = ["int64", "float64", "str", "category", "datetime64[ns]"] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here I reason that we only need one of each There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Agree, can we also add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, if the tests can handle There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have added There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, not needed for a new issue if we are covering bool atleast anywhere in our repr testing. Else, we probably want to cover it as part of another PR. |
||
|
||
|
||
@pytest.mark.parametrize("dtype", repr_categories) | ||
|
@@ -85,15 +85,12 @@ def test_full_series(nrows, dtype): | |
|
||
|
||
@pytest.mark.parametrize("dtype", repr_categories) | ||
@pytest.mark.parametrize("nrows", [0, 1, 2, 9, 20 / 2, 11, 20 - 1, 20, 20 + 1]) | ||
@pytest.mark.parametrize("ncols", [0, 1, 2, 9, 20 / 2, 11, 20 - 1, 20, 20 + 1]) | ||
def test_full_dataframe_20(dtype, nrows, ncols): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we just merge |
||
def test_full_dataframe_20(dtype): | ||
size = 20 | ||
pdf = pd.DataFrame( | ||
{idx: np.random.randint(0, 100, size) for idx in range(size)} | ||
).astype(dtype) | ||
gdf = cudf.from_pandas(pdf) | ||
|
||
assert pdf.__repr__() == gdf.__repr__() | ||
bdice marked this conversation as resolved.
Show resolved
Hide resolved
|
||
assert pdf._repr_html_() == gdf._repr_html_() | ||
assert pdf._repr_latex_() == gdf._repr_latex_() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -532,12 +532,7 @@ def _cat_convert_seq_to_cudf(others): | |
@pytest.mark.parametrize("sep", [None, "", " ", "|", ",", "|||"]) | ||
@pytest.mark.parametrize("na_rep", [None, "", "null", "a"]) | ||
@pytest.mark.parametrize( | ||
"index", | ||
[ | ||
["1", "2", "3", "4", "5"], | ||
pd.Series(["1", "2", "3", "4", "5"]), | ||
pd.Index(["1", "2", "3", "4", "5"]), | ||
], | ||
"index", [["1", "2", "3", "4", "5"]], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAICT the only place the raw |
||
) | ||
def test_string_cat(ps_gs, others, sep, na_rep, index): | ||
ps, gs = ps_gs | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copyright