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

Jump to bottom

253 Raise a visible error in the border case where no error message is available #255

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

253 Raise a visible error in the border case where no error message is available #255

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 7 additions & 0 deletions khiops/core/internals/runner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -746,6 +746,13 @@ def _report_exit_status(
f"Khiops ended correctly but there were minor issues:\n{error_msg}"
)
warnings.warn(error_msg.rstrip())
else:
# no message available, an error must be raised though
Copy link
Contributor

@folmos-at-orange folmos-at-orange Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'd put Raise an exception anyway for a non-zero return code with no messages

if return_code != 0:
raise KhiopsRuntimeError(
f"{tool_name} execution had errors (return code {return_code}) "
"but no message is available\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the \n

)

def _collect_errors(self, log_file_path):
# Collect errors any errors found in the log
Expand Down
19 changes: 19 additions & 0 deletions tests/test_core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import khiops
import khiops.core as kh
from khiops.core import KhiopsRuntimeError
from khiops.core.internals.common import create_unambiguous_khiops_path
from khiops.core.internals.io import KhiopsOutputWriter
from khiops.core.internals.runner import KhiopsLocalRunner, KhiopsRunner
Expand Down Expand Up @@ -2691,6 +2692,24 @@ def test_khiops_environment_variables_basic(self):
else:
os.environ[fixture["variable"]] = old_value

def test_raise_exception_on_error_case_without_a_message(self):
with self.assertRaises(KhiopsRuntimeError):
with MockedRunnerContext(
create_mocked_raw_run(
False, # ask for an empty stdout
False, # ask for an empty stderr
9, # non-zero error code
Comment on lines +2699 to +2701
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use named parameters to self document:

create_mocked_raw_run(
    stdout=False,
    stderr=False,
    return_code=9
)

)
):
kh.train_predictor(
"/tmp/Iris.kdic",
dictionary_name="Iris",
data_table_path="/tmp/Iris.txt",
target_variable="Class",
results_dir="/tmp",
trace=True,
)


if __name__ == "__main__":
unittest.main()
Loading