systemtests: add configurable timeout per test case#735
systemtests: add configurable timeout per test case#735AdityaGupta716 wants to merge 1 commit intoprecice:developfrom
Conversation
51e92a0 to
1ffd4f0
Compare
|
Hi @MakisH, the timeout is currently attached to ReferenceResult since that's where per-test configuration lives. Happy to move it to a dedicated field or separate dataclass if you prefer a different structure. Also kept BUILD_TIMEOUT fixed since build time feels infrastructure-dependent rather than simulation-dependent — let me know if you'd like that configurable too. |
|
This pull request has been mentioned on preCICE Forum on Discourse. There might be relevant details there: https://precice.discourse.group/t/gsoc-2026-aditya-gupta/2773/4 |
MakisH
left a comment
There was a problem hiding this comment.
Thanks for the PR! This is going in a good direction, but needs some further refactoring and cleanup. See some comments.
| class ReferenceResult: | ||
| path: Path | ||
| case_combination: CaseCombination | ||
|
|
|
|
||
|
|
||
| GLOBAL_TIMEOUT = 900 | ||
| BUILD_TIMEOUT = 900 |
There was a problem hiding this comment.
Why rename it?
How does this fit with the changes suggested/discussed in #731?
Let's discuss directly in the Changelog entry thread.
There was a problem hiding this comment.
No longer renaming, GLOBAL_TIMEOUT stays as is, now overridable via PRECICE_SYSTEMTEST_TIMEOUT environment variable as suggested .
tools/tests/README.md
Outdated
| timeout: 3600 | ||
| ``` | ||
|
|
||
| The optional `timeout` field (in seconds, default: 600) sets the maximum time allowed for the solver run and fieldcompare phases. Use this for long-running tutorials such as `heat-exchanger` or `turek-hron-fsi3`. |
There was a problem hiding this comment.
To be able to test this PR, you could set a timeout to one tutorial. We can remove it again before merging. For example, the Nutils cases are much slower to run than the rest, while the new DuMux cases take the longest to build.
Note that the default recently changed to 900 in #696
There was a problem hiding this comment.
Added timeout: 1200 to nutils_test. Default now references to GLOBAL_TIMEOUT
changelog-entries/735.md
Outdated
| @@ -0,0 +1 @@ | |||
| - Add optional `timeout` field to `tests.yaml` entries, allowing per-test timeout configuration. Defaults to 600s for backward compatibility. Applies to the solver run and fieldcompare phases. | |||
There was a problem hiding this comment.
- I would not mention the default here (it recently changed to 900s in Add tutorials with DuMux and Micro-Manager into system tests #696)
- Is there a design reason for only applying to solver run and fieldcompare, or is it just complicated to apply it to the build phase as well?
The total timeout now per case would be the build timeout + the case-specific timeout = 900s + 600s = 1500s = 25min by default. Is that intended? That would be much larger than what we currently have.
There was a problem hiding this comment.
Removed the default mention from changelog. The per-test timeout only applies to solver run and fieldcompare because build time depends on infrastructure , not on the simulation itself. Default behavior is unchanged all three phases default to GLOBAL_TIMEOUT . The per-test timeout only takes effect when set in tests.yaml.
tools/tests/systemtests/TestSuite.py
Outdated
| name: str | ||
| cases_of_tutorial: Dict[Tutorial, List[CaseCombination]] | ||
| reference_results: Dict[Tutorial, List[ReferenceResult]] | ||
| timeouts: Dict[Tutorial, List[int]] = field(default_factory=dict) |
There was a problem hiding this comment.
Why is this a (dictionary with a) list per Tutorial and not a single variable per CaseCombination?
There was a problem hiding this comment.
i followed it from cases_of_tutorial and reference_result thought should do the same here should i change it ?
| arguments: SystemtestArguments | ||
| case_combination: CaseCombination | ||
| reference_result: ReferenceResult | ||
| timeout: int = 600 |
There was a problem hiding this comment.
This number seems to now be hard-coded in different places.
There was a problem hiding this comment.
fixed it , now timeout: int = GLOBAL_TIMEOUT
tools/tests/systemtests.py
Outdated
| for tutorial in tutorials: | ||
| for case, reference_result in zip( | ||
| test_suite.cases_of_tutorial[tutorial], test_suite.reference_results[tutorial]): | ||
| timeouts = test_suite.timeouts.get(tutorial, [600] * len(test_suite.cases_of_tutorial[tutorial])) |
There was a problem hiding this comment.
Why hard-coding the 600 here? Same for the generate_reference_results.py.
e4b349c to
d7345a4
Compare
Adds an optional
timeoutfield to entries intests.yaml, allowing per-test timeout configuration instead of a single hardcoded global value.Closes #371
Changes
metadata_parser/metdata.py— addedtimeout: int = 600toReferenceResultsystemtests/TestSuite.py— parses optionaltimeoutfromtests.yaml; accepts both integer (600) and string (600s) formats; validates that the value is positivesystemtests/Systemtest.py— renamedGLOBAL_TIMEOUTtoBUILD_TIMEOUT(docker build stays fixed); solver run and fieldcompare now use the per-test configured timeouttools/tests/README.md— documented the new optional field with an exampleExample usage in
tests.yamlBackward compatible — existing entries without
timeoutuse the previous default of 600s.Checklist
changelog-entries/371.md