From 98d8b7191425f1b1be8b4192c73abaf3c84b0fb6 Mon Sep 17 00:00:00 2001 From: Kevin Messiaen Date: Fri, 19 Jan 2024 11:28:43 +0700 Subject: [PATCH 1/7] GSK-2617 Missing kwargs in a test make the whole suite fail (Hub) --- giskard/core/suite.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/giskard/core/suite.py b/giskard/core/suite.py index 7b26200f13..4749ad02a1 100644 --- a/giskard/core/suite.py +++ b/giskard/core/suite.py @@ -354,10 +354,6 @@ def run(self, verbose: bool = True, **suite_run_args): run_args.update(suite_run_args) results: List[(str, TestResult, Dict[str, Any])] = list() - required_params = self.find_required_params() - undefined_params = {k: v for k, v in required_params.items() if k not in run_args} - if len(undefined_params): - raise ValueError(f"Missing {len(undefined_params)} required parameters: {undefined_params}") for test_partial in self.tests: test_params = self.create_test_params(test_partial, run_args) From eb70813fc53454b8157368a268084eb8db4e0d40 Mon Sep 17 00:00:00 2001 From: Kevin Messiaen Date: Fri, 19 Jan 2024 13:47:26 +0700 Subject: [PATCH 2/7] GSK-2617 Missing kwargs in a test make the whole suite fail (Hub) --- tests/test_programmatic.py | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/tests/test_programmatic.py b/tests/test_programmatic.py index 680e1eb474..b8a737ca40 100644 --- a/tests/test_programmatic.py +++ b/tests/test_programmatic.py @@ -46,18 +46,35 @@ def test_a_greater_b_pass(): def test_missing_arg(): - with pytest.raises(Exception, match="Missing 1 required parameters: {'b': }"): - Suite().add_test(_test_a_greater_b(a=2)).run() + result = Suite().add_test(_test_a_greater_b(a=2)).run() + + assert not result.passed + + assert len(result.results) == 1 + _, test_result, _ = result.results[0] + assert "1 validation error for _test_a_greater_b" in test_result.messages[0].text def test_missing_args(): - with pytest.raises(Exception, match="Missing 2 required parameters: {'a': , 'b': }"): - Suite().add_test(_test_a_greater_b()).run() + result = Suite().add_test(_test_a_greater_b()).run() + + assert not result.passed + + assert len(result.results) == 1 + _, test_result, _ = result.results[0] + assert test_result.is_error + assert "2 validation errors for _test_a_greater_b" in test_result.messages[0].text def test_missing_arg_one_global(): - with pytest.raises(Exception, match="Missing 1 required parameters: {'b': }"): - Suite().add_test(_test_a_greater_b()).run(a=2) + result = Suite().add_test(_test_a_greater_b()).run(a=2) + + assert not result.passed + + assert len(result.results) == 1 + _, test_result, _ = result.results[0] + assert test_result.is_error + assert "1 validation error for _test_a_greater_b" in test_result.messages[0].text def test_all_global(): From 0ac052932f1b60a86c2053f21f6f5c768fb89645 Mon Sep 17 00:00:00 2001 From: Kevin Messiaen Date: Fri, 19 Jan 2024 16:55:10 +0700 Subject: [PATCH 3/7] GSK-2617 Missing kwargs in a test make the whole suite fail (Hub) --- tests/test_suite.py | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/tests/test_suite.py b/tests/test_suite.py index 55a21e5eba..2c0d9088dc 100644 --- a/tests/test_suite.py +++ b/tests/test_suite.py @@ -27,15 +27,25 @@ def test_default_parameters_are_used_at_runtime(german_credit_data, german_credi # This will miss dataset suite = Suite(default_params=dict(model=german_credit_model)) suite.add_test(my_test) - with pytest.raises(ValueError): - suite.run() + result = suite.run() + assert not result.passed + _, test_result, _ = result.results[0] + assert test_result.is_error # But we can pass dataset at runtime - suite.run(dataset=german_credit_data) + result = suite.run(dataset=german_credit_data) + assert result.passed + _, test_result, _ = result.results[0] + assert not test_result.is_error + assert test_result.passed # Or we can provide it in the suite defaults suite.default_params["dataset"] = german_credit_data - suite.run() + result = suite.run() + assert result.passed + _, test_result, _ = result.results[0] + assert not test_result.is_error + assert test_result.passed def test_runtime_parameters_override_default_parameters(german_credit_data, german_credit_model): From d1116f2e82087e949331286566b346222547fa28 Mon Sep 17 00:00:00 2001 From: Kevin Messiaen Date: Tue, 30 Jan 2024 09:53:10 +0700 Subject: [PATCH 4/7] GSK-2617 Add warning when missing params for `Suite.run` and `Suite.to_unittest` --- giskard/core/suite.py | 13 +++++++++---- tests/test_programmatic.py | 37 ++++++++++++++++++++----------------- 2 files changed, 29 insertions(+), 21 deletions(-) diff --git a/giskard/core/suite.py b/giskard/core/suite.py index 57ffb9ddc7..a92fd462f5 100644 --- a/giskard/core/suite.py +++ b/giskard/core/suite.py @@ -399,6 +399,7 @@ def run(self, verbose: bool = True, **suite_run_args): """ run_args = self.default_params.copy() run_args.update(suite_run_args) + self.verify_required_params(run_args) results: List[(str, TestResult, Dict[str, Any])] = list() @@ -459,10 +460,7 @@ def to_unittest(self, **suite_gen_args) -> List[TestPartial]: run_args.update(suite_gen_args) unittests: List[TestPartial] = list() - required_params = self.find_required_params() - undefined_params = {k: v for k, v in required_params.items() if k not in run_args} - if len(undefined_params): - raise ValueError(f"Missing {len(undefined_params)} required parameters: {undefined_params}") + self.verify_required_params(run_args) for test_partial in self.tests: test_params = self.create_test_params(test_partial, run_args) @@ -478,6 +476,13 @@ def to_unittest(self, **suite_gen_args) -> List[TestPartial]: return unittests + def verify_required_params(self, run_args): + required_params = self.find_required_params() + undefined_params = {k: v for k, v in required_params.items() if k not in run_args} + + if len(undefined_params): + warning(f"Missing {len(undefined_params)} required parameters: {undefined_params}") + @staticmethod def create_test_params(test_partial, kwargs): if isinstance(test_partial.giskard_test, GiskardTestMethod): diff --git a/tests/test_programmatic.py b/tests/test_programmatic.py index b8a737ca40..79b8e1a6bd 100644 --- a/tests/test_programmatic.py +++ b/tests/test_programmatic.py @@ -46,35 +46,38 @@ def test_a_greater_b_pass(): def test_missing_arg(): - result = Suite().add_test(_test_a_greater_b(a=2)).run() + with pytest.warns(match="Missing 1 required parameters: {'b': }"): + result = Suite().add_test(_test_a_greater_b(a=2)).run() - assert not result.passed + assert not result.passed - assert len(result.results) == 1 - _, test_result, _ = result.results[0] - assert "1 validation error for _test_a_greater_b" in test_result.messages[0].text + assert len(result.results) == 1 + _, test_result, _ = result.results[0] + assert "1 validation error for _test_a_greater_b" in test_result.messages[0].text def test_missing_args(): - result = Suite().add_test(_test_a_greater_b()).run() + with pytest.warns(match="Missing 2 required parameters: {'a': , 'b': }"): + result = Suite().add_test(_test_a_greater_b()).run() - assert not result.passed + assert not result.passed - assert len(result.results) == 1 - _, test_result, _ = result.results[0] - assert test_result.is_error - assert "2 validation errors for _test_a_greater_b" in test_result.messages[0].text + assert len(result.results) == 1 + _, test_result, _ = result.results[0] + assert test_result.is_error + assert "2 validation errors for _test_a_greater_b" in test_result.messages[0].text def test_missing_arg_one_global(): - result = Suite().add_test(_test_a_greater_b()).run(a=2) + with pytest.warns(match="Missing 1 required parameters: {'b': }"): + result = Suite().add_test(_test_a_greater_b()).run(a=2) - assert not result.passed + assert not result.passed - assert len(result.results) == 1 - _, test_result, _ = result.results[0] - assert test_result.is_error - assert "1 validation error for _test_a_greater_b" in test_result.messages[0].text + assert len(result.results) == 1 + _, test_result, _ = result.results[0] + assert test_result.is_error + assert "1 validation error for _test_a_greater_b" in test_result.messages[0].text def test_all_global(): From 0a0faec23bf0afa349c01a6716afd9845b03d1ee Mon Sep 17 00:00:00 2001 From: Kevin Messiaen Date: Tue, 30 Jan 2024 12:16:31 +0700 Subject: [PATCH 5/7] GSK-2617 Fixed tests for Pydantic V1 --- tests/test_programmatic.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/test_programmatic.py b/tests/test_programmatic.py index 79b8e1a6bd..4ea47dd6ea 100644 --- a/tests/test_programmatic.py +++ b/tests/test_programmatic.py @@ -53,7 +53,7 @@ def test_missing_arg(): assert len(result.results) == 1 _, test_result, _ = result.results[0] - assert "1 validation error for _test_a_greater_b" in test_result.messages[0].text + assert "1 validation error" in test_result.messages[0].text def test_missing_args(): @@ -65,7 +65,7 @@ def test_missing_args(): assert len(result.results) == 1 _, test_result, _ = result.results[0] assert test_result.is_error - assert "2 validation errors for _test_a_greater_b" in test_result.messages[0].text + assert "2 validation errors" in test_result.messages[0].text def test_missing_arg_one_global(): @@ -77,7 +77,7 @@ def test_missing_arg_one_global(): assert len(result.results) == 1 _, test_result, _ = result.results[0] assert test_result.is_error - assert "1 validation error for _test_a_greater_b" in test_result.messages[0].text + assert "1 validation error" in test_result.messages[0].text, test_result.messages[0].text def test_all_global(): From 32e1a36cebb878ecee55023b15228ed3b2d8f514 Mon Sep 17 00:00:00 2001 From: Kevin Messiaen <114553769+kevinmessiaen@users.noreply.github.com> Date: Thu, 8 Feb 2024 11:07:40 +0700 Subject: [PATCH 6/7] Update giskard/core/suite.py Co-authored-by: Jocelyn Vernay <59055698+vjern@users.noreply.github.com> --- giskard/core/suite.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/giskard/core/suite.py b/giskard/core/suite.py index 9517aad29d..de3036be75 100644 --- a/giskard/core/suite.py +++ b/giskard/core/suite.py @@ -510,7 +510,7 @@ def to_unittest(self, **suite_gen_args) -> List[TestPartial]: return unittests - def verify_required_params(self, run_args): + def verify_required_params(self, run_args: Dict[str, Any]): required_params = self.find_required_params() undefined_params = {k: v for k, v in required_params.items() if k not in run_args} From cdeb1f01eec2840bcac980ccbeac872402c76f35 Mon Sep 17 00:00:00 2001 From: Kevin Messiaen <114553769+kevinmessiaen@users.noreply.github.com> Date: Thu, 8 Feb 2024 11:08:09 +0700 Subject: [PATCH 7/7] Update giskard/core/suite.py Co-authored-by: Jocelyn Vernay <59055698+vjern@users.noreply.github.com> --- giskard/core/suite.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/giskard/core/suite.py b/giskard/core/suite.py index de3036be75..67cc583ddd 100644 --- a/giskard/core/suite.py +++ b/giskard/core/suite.py @@ -514,7 +514,7 @@ def verify_required_params(self, run_args: Dict[str, Any]): required_params = self.find_required_params() undefined_params = {k: v for k, v in required_params.items() if k not in run_args} - if len(undefined_params): + if undefined_params: warning(f"Missing {len(undefined_params)} required parameters: {undefined_params}") @staticmethod