-
-
Notifications
You must be signed in to change notification settings - Fork 158
systemtests: add per-test max_time override for precice-config #744
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
base: develop
Are you sure you want to change the base?
Changes from all commits
6480cad
d8711c8
a283ee8
90a4660
9858698
cb5833b
27451c3
aa74b51
0086da0
8201a5c
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 |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| - Add per-test `max_time` override in `tests.yaml` to cap preCICE simulation time without editing `precice-config.xml` manually. Applies consistently to both test runs and reference result generation. Handles multiple `<max-time>` tags with a warning. | ||
|
Member
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. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,7 +19,7 @@ | |
| import os | ||
|
|
||
|
|
||
| GLOBAL_TIMEOUT = 900 | ||
| BUILD_TIMEOUT = 900 | ||
|
Member
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. |
||
| SHORT_TIMEOUT = 10 | ||
|
|
||
|
|
||
|
|
@@ -134,6 +134,7 @@ class Systemtest: | |
| arguments: SystemtestArguments | ||
| case_combination: CaseCombination | ||
| reference_result: ReferenceResult | ||
| max_time: Optional[float] = None | ||
|
Member
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.
But I am not very up-to-date in Python. |
||
| params_to_use: Dict[str, str] = field(init=False) | ||
| env: Dict[str, str] = field(init=False) | ||
|
|
||
|
|
@@ -394,7 +395,7 @@ def _run_field_compare(self): | |
| cwd=self.system_test_dir) | ||
|
|
||
| try: | ||
| stdout, stderr = process.communicate(timeout=GLOBAL_TIMEOUT) | ||
| stdout, stderr = process.communicate(timeout=self.timeout) | ||
|
Member
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. Originates from #735, please separate the two PRs. |
||
| except KeyboardInterrupt as k: | ||
| process.kill() | ||
| raise KeyboardInterrupt from k | ||
|
|
@@ -439,7 +440,7 @@ def _build_docker(self): | |
| cwd=self.system_test_dir) | ||
|
|
||
| try: | ||
| stdout, stderr = process.communicate(timeout=GLOBAL_TIMEOUT) | ||
| stdout, stderr = process.communicate(timeout=BUILD_TIMEOUT) | ||
| except KeyboardInterrupt as k: | ||
| process.kill() | ||
| # process.send_signal(9) | ||
|
|
@@ -483,7 +484,7 @@ def _run_tutorial(self): | |
| cwd=self.system_test_dir) | ||
|
|
||
| try: | ||
| stdout, stderr = process.communicate(timeout=GLOBAL_TIMEOUT) | ||
| stdout, stderr = process.communicate(timeout=self.timeout) | ||
| except KeyboardInterrupt as k: | ||
| process.kill() | ||
| # process.send_signal(9) | ||
|
|
@@ -513,11 +514,52 @@ def __write_logs(self, stdout_data: List[str], stderr_data: List[str]): | |
| with open(self.system_test_dir / "stderr.log", 'w') as stderr_file: | ||
| stderr_file.write("\n".join(stderr_data)) | ||
|
|
||
| def __apply_precice_max_time_override(self): | ||
| """ | ||
| If max_time is set, override the <max-time value="..."/> in precice-config.xml | ||
| of the copied tutorial directory. Applies to both test runs and reference generation. | ||
| Targets only <max-time> tags to avoid modifying time-window-size or other attributes. | ||
| """ | ||
| if self.max_time is None: | ||
| return | ||
| if not (isinstance(self.max_time, (int, float)) and self.max_time > 0): | ||
| logging.warning( | ||
| f"Invalid max_time {self.max_time!r} for {self}; must be a positive number. Skipping override.") | ||
| return | ||
| config_path = self.system_test_dir / "precice-config.xml" | ||
| if not config_path.exists(): | ||
| logging.warning( | ||
| f"Requested max_time override for {self}, but no precice-config.xml " | ||
| f"found in {self.system_test_dir}") | ||
| return | ||
|
Comment on lines
+529
to
+534
Member
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 should never happen in the system tests: all tests have a |
||
| try: | ||
| text = config_path.read_text() | ||
| except Exception as e: | ||
| logging.warning(f"Could not read {config_path} to apply max_time override: {e}") | ||
| return | ||
| pattern = r'(<max-time\s+value=")([^"]*)(")' | ||
| matches = re.findall(pattern, text) | ||
| if not matches: | ||
| logging.warning( | ||
| f"Requested max_time override for {self}, but no <max-time .../> tag " | ||
| f"found in {config_path}") | ||
| return | ||
| if len(matches) > 1: | ||
| logging.warning( | ||
| f"Multiple <max-time> tags found in {config_path}; overriding all to {self.max_time}") | ||
| new_text = re.sub(pattern, rf"\g<1>{self.max_time}\g<3>", text) | ||
| try: | ||
| config_path.write_text(new_text) | ||
| logging.info(f"Overwrote max-time in {config_path} to {self.max_time} for {self}") | ||
| except Exception as e: | ||
| logging.warning(f"Failed to write updated {config_path}: {e}") | ||
|
|
||
|
Comment on lines
+535
to
+556
Member
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. Note that https://precice.org/configuration-xml-reference.html#max-time https://precice.org/configuration-xml-reference.html#max-time-windows I would leave it up to the user to set the right thing, and not add so much error handling. I think this function is a bit more complicated than it needs to be. Note that an alternative approach would be to extend the docker compose service before running. We already set some of these in the template, e.g., to always export VTK files: https://github.com/precice/tutorials/blob/develop/tools/tests/docker-compose.template.yaml
Member
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. Actually, for shorter tests, it would be convenient to directly define As a maintainer, I would be interested to know that, e.g., three coupling time windows complete, since most problems appear there. The time is more relevant for the application, not for the coupling. |
||
| def __prepare_for_run(self, run_directory: Path): | ||
| """ | ||
| Prepares the run_directory with folders and datastructures needed for every systemtest execution | ||
| """ | ||
| self.__copy_tutorial_into_directory(run_directory) | ||
| self.__apply_precice_max_time_override() | ||
| self.__copy_tools(run_directory) | ||
| self.__put_gitignore(run_directory) | ||
| host_uid, host_gid = self.__get_uid_gid() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ class TestSuite: | |
| name: str | ||
| cases_of_tutorial: Dict[Tutorial, List[CaseCombination]] | ||
| reference_results: Dict[Tutorial, List[ReferenceResult]] | ||
| max_times: Dict[Tutorial, List[Optional[float]]] = field(default_factory=dict) | ||
|
Member
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. Similarly to #735: Why is this a (dictionary of a) list per tutorial, and not a value per |
||
|
|
||
| def __repr__(self) -> str: | ||
| return_string = f"Test suite: {self.name} contains:" | ||
|
|
@@ -48,6 +49,7 @@ def from_yaml(cls, path, parsed_tutorials: Tutorials): | |
| for test_suite_name in test_suites_raw: | ||
| case_combinations_of_tutorial = {} | ||
| reference_results_of_tutorial = {} | ||
| max_times_of_tutorial = {} | ||
| # iterate over tutorials: | ||
| for tutorial_case in test_suites_raw[test_suite_name]['tutorials']: | ||
| tutorial = parsed_tutorials.get_by_path(tutorial_case['path']) | ||
|
|
@@ -57,6 +59,7 @@ def from_yaml(cls, path, parsed_tutorials: Tutorials): | |
| if tutorial not in case_combinations_of_tutorial: | ||
| case_combinations_of_tutorial[tutorial] = [] | ||
| reference_results_of_tutorial[tutorial] = [] | ||
| max_times_of_tutorial[tutorial] = [] | ||
|
|
||
| all_case_combinations = tutorial.case_combinations | ||
| case_combination_requested = CaseCombination.from_string_list( | ||
|
|
@@ -65,12 +68,19 @@ def from_yaml(cls, path, parsed_tutorials: Tutorials): | |
| case_combinations_of_tutorial[tutorial].append(case_combination_requested) | ||
| reference_results_of_tutorial[tutorial].append(ReferenceResult( | ||
| tutorial_case['reference_result'], case_combination_requested)) | ||
| raw_max_time = tutorial_case.get('max_time') | ||
| if raw_max_time is not None: | ||
| if not (isinstance(raw_max_time, (int, float)) and raw_max_time > 0): | ||
| raise ValueError( | ||
| f"Invalid max_time {raw_max_time!r} for tutorial " | ||
| f"'{tutorial_case['path']}'; must be a positive number.") | ||
| max_times_of_tutorial[tutorial].append(raw_max_time) | ||
| else: | ||
| raise Exception( | ||
| f"Could not find the following cases {tutorial_case['case-combination']} in the current metadata of tutorial {tutorial.name}") | ||
|
|
||
| testsuites.append(TestSuite(test_suite_name, case_combinations_of_tutorial, | ||
| reference_results_of_tutorial)) | ||
| reference_results_of_tutorial, max_times_of_tutorial)) | ||
|
|
||
| return cls(testsuites) | ||
|
|
||
|
|
||
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.
This should be a warning in preCICE library itself, not in the system tests.