-
-
Notifications
You must be signed in to change notification settings - Fork 164
Add per-component build_timeout for system test Docker builds #848
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
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 optional per-component `build_timeout` in `components.yaml` for the system test Docker build step ([#848](https://github.com/precice/tutorials/pull/848)). | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -329,8 +329,10 @@ This template defines: | |||||
|
|
||||||
| ### Timeouts | ||||||
|
|
||||||
| A `GLOBAL_TIMEOUT` is used for all operations. Its default value is 600s (5min), it is set in the beginning of [`Systemtests.py`](https://github.com/precice/tutorials/blob/develop/tools/tests/systemtests/Systemtest.py), and it can be overridden via the `PRECICE_SYSTEMTESTS_TIMEOUT` environment variable. | ||||||
| Build and run/compare use separate timeouts. | ||||||
|
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.
Suggested change
|
||||||
|
|
||||||
| Tests can define a different `timeout` in their `tests.yaml` entry, which applies to the running and results comparison steps. | ||||||
| **Build:** Each component in `components.yaml` may define `build_timeout` (seconds). For a test, the build step uses the maximum `build_timeout` among its components; components without `build_timeout` use `DEFAULT_BUILD_TIMEOUT` (600s by default, same as `GLOBAL_TIMEOUT`). Override the default via `PRECICE_SYSTEMTESTS_BUILD_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. Does that mean that if I have a test that uses OpenFOAM and DuMux, building the OpenFOAM Docker image will timeout at the DuMux build timeout? Why is that? |
||||||
|
|
||||||
| **Run and compare:** `GLOBAL_TIMEOUT` defaults to 600s and can be overridden via `PRECICE_SYSTEMTESTS_TIMEOUT`. Tests can define a different `timeout` in their `tests.yaml` entry, which applies to the running and results comparison steps only. | ||||||
|
|
||||||
| </details> | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -50,6 +50,7 @@ dealii-adapter: | |
|
|
||
| dumux-adapter: | ||
| template: component-templates/dumux-adapter.yaml | ||
| build_timeout: 1800 | ||
|
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. DuMux already builds with 600s. Let's set this to 600s, and reduce the There will be more cases with longer timeouts later. |
||
| build_arguments: | ||
| PLATFORM: | ||
| default: "ubuntu_2404" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -97,6 +97,7 @@ class Component: | |
| name: str | ||
| template: str | ||
| parameters: BuildArguments | ||
| build_timeout: Optional[int] = 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. I think that |
||
|
|
||
| def __eq__(self, other): | ||
| if isinstance(other, Component): | ||
|
|
@@ -130,11 +131,17 @@ def from_yaml(cls, path): | |
| with open(path, 'r') as f: | ||
| data = yaml.safe_load(f) | ||
| for component_name in data: | ||
| parameters = BuildArguments.from_components_yaml( | ||
| data[component_name]) | ||
| template = data[component_name]["template"] | ||
| component_data = data[component_name] | ||
| parameters = BuildArguments.from_components_yaml(component_data) | ||
| template = component_data["template"] | ||
| build_timeout = component_data.get("build_timeout", None) | ||
| if build_timeout is not None: | ||
| if not isinstance(build_timeout, int) or build_timeout <= 0: | ||
| raise ValueError( | ||
| f"build_timeout must be a positive integer for component " | ||
| f"'{component_name}', got {build_timeout!r}") | ||
| components.append( | ||
| Component(component_name, template, parameters)) | ||
| Component(component_name, template, parameters, build_timeout)) | ||
|
|
||
| return cls(components) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,8 @@ | |
|
|
||
|
|
||
| GLOBAL_TIMEOUT = int(os.environ.get("PRECICE_SYSTEMTESTS_TIMEOUT", 600)) | ||
| DEFAULT_BUILD_TIMEOUT = int( | ||
|
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. Similar comment as in #847: I get the impression we need a dictionary of defaults here. But maybe in a separate PR. |
||
| os.environ.get("PRECICE_SYSTEMTESTS_BUILD_TIMEOUT", GLOBAL_TIMEOUT)) | ||
| SHORT_TIMEOUT = 10 | ||
|
|
||
| DIFF_RESULTS_DIR = "diff-results" | ||
|
|
@@ -236,6 +238,24 @@ def __hash__(self) -> int: | |
| def __post_init__(self): | ||
| self.__init_args_to_use() | ||
| self.env = {} | ||
| self.build_timeout = self._resolve_build_timeout() | ||
|
|
||
| def _resolve_build_timeout(self) -> int: | ||
| """ | ||
| Use the maximum build_timeout of the distinct components in this test. | ||
| Components without build_timeout use DEFAULT_BUILD_TIMEOUT. | ||
| """ | ||
| timeouts = [] | ||
| seen_components = set() | ||
| for case in self.case_combination.cases: | ||
| if case.component.name in seen_components: | ||
| continue | ||
| seen_components.add(case.component.name) | ||
| if case.component.build_timeout is not None: | ||
| timeouts.append(case.component.build_timeout) | ||
| else: | ||
| timeouts.append(DEFAULT_BUILD_TIMEOUT) | ||
| return max(timeouts) if timeouts else DEFAULT_BUILD_TIMEOUT | ||
|
|
||
| def __init_args_to_use(self): | ||
| """ | ||
|
|
@@ -714,6 +734,8 @@ def _build_docker(self): | |
| Builds the docker image | ||
| """ | ||
| logging.debug(f"Building docker image for {self}") | ||
| logging.info( | ||
| f"Using build timeout {self.build_timeout}s for {self}") | ||
| time_start = time.perf_counter() | ||
| docker_compose_content = self.__get_docker_compose_file() | ||
| with open(self.system_test_dir / "docker-compose.tutorial.yaml", 'w') as file: | ||
|
|
@@ -729,7 +751,7 @@ def _build_docker(self): | |
| 'build', | ||
| ], | ||
| "build", | ||
| GLOBAL_TIMEOUT, | ||
| self.build_timeout, | ||
| ) | ||
| elapsed_time = time.perf_counter() - time_start | ||
| return DockerComposeResult(exit_code, stdout_data, stderr_data, self, elapsed_time) | ||
|
|
||
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.
Remember, changelogs are written in past tense: