TEST-#6016: make sure `eval_general` doesn't expect exceptions by default #6954

anmyachev · 2024-02-21T17:17:03Z

What do these changes do?

first commit message and PR title follow format outlined here

NOTE: If you edit the PR title to match this format, you need to add another commit (even if it's empty) or amend your last commit for the CI job that checks the PR title to pick up the new PR title.
passes flake8 modin/ asv_bench/benchmarks scripts/doc_checker.py
passes black --check modin/ asv_bench/benchmarks scripts/doc_checker.py
signed commit with git commit -s
Resolves TEST: eval_general should assume by default that pandas does not throw an error #6016
tests added and passing
module layout described at docs/development/architecture.rst is up-to-date

modin/pandas/test/test_series.py

…ptions by default Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>

Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>

anmyachev · 2024-03-07T19:24:48Z

@dchigarev @YarShev ready for review

…issue6016

Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>

anmyachev · 2024-03-13T17:00:02Z

modin/pandas/test/utils.py

@@ -1420,7 +1410,7 @@ def _csv_file_maker(
                    value=[char if (x + 2) == 0 else x for x in range(row_size)],
                )

-            if thousands_separator:
+            if thousands_separator is not None:


can be empty string.

anmyachev · 2024-03-13T17:00:30Z

modin/pandas/test/utils.py

-    finally:
-        if os.path.exists(unique_filename):
-            try:
-                os.remove(unique_filename)
-            except PermissionError:
-                pass


No need to cleanup, we use tmp_path fixture.

anmyachev · 2024-03-13T17:01:40Z

modin/pandas/test/utils.py

@@ -331,7 +331,7 @@
    "sum of certain elements": lambda axis: (
        axis.iloc[0] + axis.iloc[-1] if isinstance(axis, pandas.Series) else axis + axis
    ),
-    "should raise TypeError": 1,
+    "should raise AssertionError": 1,


This is more similar to the errors that I have seen. Can be called more neutrally (should raise Exception).

anmyachev · 2024-03-13T17:02:44Z

modin/pandas/test/dataframe/test_window.py

    eval_general(
        *create_test_dfs(test_data["float_nan_data"]),
        lambda df: getattr(df, method)(axis=axis, skipna=skipna),
    )


-def test_rank_except():


It was convenient to combine them.

anmyachev · 2024-03-13T17:04:08Z

.github/workflows/ci.yml

@@ -395,7 +395,7 @@ jobs:
          - ubuntu
          - windows
        python-version: ["3.9"]
-        engine: ${{ fromJSON( github.event_name == 'push' && '["python", "ray", "dask"]' || needs.execution-filter.outputs.engines ) }}


Just for testing purpose. Need to revert.

anmyachev · 2024-03-13T17:06:08Z

modin/pandas/test/utils.py

@@ -279,7 +279,7 @@
 test_string_list_data_values = list(test_string_list_data.values())
 test_string_list_data_keys = list(test_string_list_data.keys())

-string_seperators = {"empty sep": "", "comma sep": ",", "None sep": None}


Invalid separators.

anmyachev · 2024-03-13T17:07:10Z

@YarShev ready for review

YarShev · 2024-03-14T11:51:24Z

modin/experimental/core/execution/native/implementations/hdk_on_native/io/io.py

@@ -457,7 +457,10 @@ def _read_csv_check_support(
                    + "'infer' header values",
                )
            if isinstance(parse_dates, list) and not set(parse_dates).issubset(names):
-                raise ValueError("Missing column provided to 'parse_dates'")
+                missed_columns = set(parse_dates) - set(names)


Suggested change

missed_columns = set(parse_dates) - set(names)

missing_columns = set(parse_dates) - set(names)

YarShev · 2024-03-14T11:52:55Z

modin/pandas/test/dataframe/test_binary.py

@@ -319,7 +335,9 @@ def test_equals_with_nans():
 @pytest.mark.parametrize(
    "is_idx_aligned", [True, False], ids=["idx_aligned", "idx_not_aligned"]
 )
-def test_mismatched_row_partitions(is_idx_aligned, op_type, is_more_other_partitions):
+def test_mismatched_row_partitions(
+    is_idx_aligned, op_type, is_more_other_partitions, request


Why is request added?

YarShev · 2024-03-14T11:55:56Z

modin/pandas/test/dataframe/test_binary.py

+    raising_exceptions = None
+    if (
+        "bool-bool" in request.node.callspec.id
+        or "bool scalar-bool" in request.node.callspec.id


I wonder if we are going to proceed using request.* to identify the exact errors and how difficult it is for developers to find the exact cases for those errors using request.*?

I wonder if we are going to proceed using request.* to identify the exact errors and how difficult it is for developers to find the exact cases for those errors using request.*?

Well, this is quite inconvenient, but this is the price we need to pay in order to be more confident that what is actually being tested is what is intended.

I would also like to note that to avoid this inconvenience, when writing a test, we need to separate test cases that should work correctly from those that should not.

I would also like to note that to avoid this inconvenience, when writing a test, we need to separate test cases that should work correctly from those that should not.

Yes, separating tests would probably make sense here. Matching error-prone parameters with exceptions would help I think.

modin/pandas/test/dataframe/test_map_metadata.py

modin/pandas/test/dataframe/test_udf.py

YarShev · 2024-03-14T11:58:36Z

modin/pandas/test/dataframe/test_window.py

@@ -518,15 +513,6 @@ def test_median_skew_transposed(axis, method):
    )


-@pytest.mark.parametrize("numeric_only", [True, False, None])
-@pytest.mark.parametrize("method", ["median", "skew", "std", "var", "rank", "sem"])
-def test_median_skew_std_var_rank_sem_specific(numeric_only, method):


Why removed?

These methods are already being tested in test_reduce.py IIRC.

YarShev · 2024-03-14T12:01:45Z

modin/pandas/test/utils.py

@@ -924,6 +920,8 @@ def execute_callable(fn, inplace=False, md_kwargs={}, pd_kwargs={}):
                    assert (
                        pd_e.args == raising_exceptions.args
                    ), f"not acceptable Pandas' exception: [{repr(pd_e)}]"
+                elif raising_exceptions is not False:


When can we get into this branch if we already have if raising_exceptions, and why do we need this branch?

This is the only way to disable exception checking, it only works when raising_exceptions=False.

The not-so-obvious value for the parameter should probably be reconsidered separately.

Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>

anmyachev force-pushed the issue6016 branch 4 times, most recently from 50fa735 to 79b5ba0 Compare February 27, 2024 12:21

github-advanced-security bot found potential problems Feb 29, 2024

View reviewed changes

modin/pandas/test/test_series.py Fixed Show fixed Hide fixed

modin/pandas/test/test_series.py Fixed Show fixed Hide fixed

anmyachev force-pushed the issue6016 branch from c3a06cf to 4928719 Compare February 29, 2024 14:45

anmyachev added 14 commits March 1, 2024 12:03

TEST-modin-project#6016: make sure 'eval_general' doesn't expect exce…

1bcb09e

…ptions by default Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>

update 'test_general'

22207df

Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>

update 'test_binary.py'

b281235

Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>

fixes

c014163

Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>

fixes

a5db6a3

Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>

update 'test_indexing.py'

2971259

Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>

fixes

cb1f705

Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>

update 'test_reduce.py'

d52e6b9

Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>

update 'test_udf.py'

4d2c028

Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>

fixes

c9122d4

Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>

fixes

46cfb38

Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>

update 'test_series.py'

8eb878b

Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>

update 'test_map_metadata.py'

27e1444

Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>

fixes

0e7e206

Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>

anmyachev force-pushed the issue6016 branch from c9fdd4a to 0e7e206 Compare March 1, 2024 11:05

anmyachev added 4 commits March 1, 2024 15:20

update 'test_io.py'

5ec5984

Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>

fixes

74f3ebf

Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>

fixes

a40b71f

Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>

fixes

93af135

Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>

anmyachev force-pushed the issue6016 branch from f080c28 to 61445ad Compare March 3, 2024 16:52

fixes

e7e30f0

Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>

anmyachev force-pushed the issue6016 branch from 61445ad to e7e30f0 Compare March 3, 2024 17:02

anmyachev added 2 commits March 3, 2024 18:13

fixes

54910b4

Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>

fixes

d0a0867

Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>

fixes

cd04805

Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>

anmyachev force-pushed the issue6016 branch from b7735e3 to cd04805 Compare March 7, 2024 16:28

fixes

d3ce0c2

Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>

This was referenced Mar 11, 2024

FIX-#7051: Update exception message for astype function #7052

Merged

TEST-#7066: Explicitly check for exceptions in test_io.py #7067

Merged

Merge branch 'master' of https://github.com/modin-project/modin into …

6abeba7

…issue6016

anmyachev force-pushed the issue6016 branch from 3a962f8 to b7c7067 Compare March 13, 2024 16:17

fixes

6bcd51d

Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>

anmyachev force-pushed the issue6016 branch from b7c7067 to 6bcd51d Compare March 13, 2024 16:18

fixes

9a340ec

Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>

anmyachev commented Mar 13, 2024

View reviewed changes

YarShev reviewed Mar 14, 2024

View reviewed changes

address review comments and remove debug stuff

46c4abe

Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>

YarShev approved these changes Mar 14, 2024

View reviewed changes

YarShev merged commit c753436 into modin-project:master Mar 14, 2024
37 checks passed

anmyachev deleted the issue6016 branch March 14, 2024 17:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TEST-#6016: make sure `eval_general` doesn't expect exceptions by default #6954

TEST-#6016: make sure `eval_general` doesn't expect exceptions by default #6954

anmyachev commented Feb 21, 2024 •

edited

Loading

anmyachev commented Mar 7, 2024

anmyachev Mar 13, 2024

anmyachev Mar 13, 2024

anmyachev Mar 13, 2024

anmyachev Mar 13, 2024

anmyachev Mar 13, 2024

anmyachev Mar 14, 2024

anmyachev Mar 13, 2024

anmyachev commented Mar 13, 2024

YarShev Mar 14, 2024

YarShev Mar 14, 2024

anmyachev Mar 14, 2024

YarShev Mar 14, 2024

anmyachev Mar 14, 2024

YarShev Mar 14, 2024

YarShev Mar 14, 2024

anmyachev Mar 14, 2024

YarShev Mar 14, 2024

anmyachev Mar 14, 2024

anmyachev Mar 14, 2024

	missed_columns = set(parse_dates) - set(names)
	missing_columns = set(parse_dates) - set(names)

TEST-#6016: make sure eval_general doesn't expect exceptions by default #6954

TEST-#6016: make sure eval_general doesn't expect exceptions by default #6954

Conversation

anmyachev commented Feb 21, 2024 • edited Loading

What do these changes do?

anmyachev commented Mar 7, 2024

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

anmyachev commented Mar 13, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TEST-#6016: make sure `eval_general` doesn't expect exceptions by default #6954

TEST-#6016: make sure `eval_general` doesn't expect exceptions by default #6954

anmyachev commented Feb 21, 2024 •

edited

Loading