Skip to content

Add per-component build_timeout for system test Docker builds#848

Open
PranjalManhgaye wants to merge 2 commits into
precice:developfrom
PranjalManhgaye:feature/840-build-timeout
Open

Add per-component build_timeout for system test Docker builds#848
PranjalManhgaye wants to merge 2 commits into
precice:developfrom
PranjalManhgaye:feature/840-build-timeout

Conversation

@PranjalManhgaye

Copy link
Copy Markdown
Collaborator

Summary

  • add optional build_timeout to components in components.yaml (build step only)
  • resolve per-test build timeout as the maximum across distinct participants
  • set dumux-adapter : build_timeout: 1800 for long Dumux Docker builds
  • Document separate build vs run/compare timeouts in readme
    fixes Timeout for building system tests containers #840

Fixes precice#840. Components can set build_timeout in components.yaml; each test
uses the max across its participants. DuMuX builds get 1800s by default.

@MakisH MakisH left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for implementing! Here are some comments before testing on GHA.

Comment thread changelog-entries/848.md
@@ -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)).

Copy link
Copy Markdown
Member

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:

Suggested change
- Add optional per-component `build_timeout` in `components.yaml` for the system test Docker build step ([#848](https://github.com/precice/tutorials/pull/848)).
- Added optional per-component `build_timeout` in `components.yaml` for the system test Docker build step ([#848](https://github.com/precice/tutorials/pull/848)).

name: str
template: str
parameters: BuildArguments
build_timeout: Optional[int] = None

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think that Optional is not the current way of implementing optional arguments in Python. One reference: https://stackoverflow.com/a/63442322/2254346



GLOBAL_TIMEOUT = int(os.environ.get("PRECICE_SYSTEMTESTS_TIMEOUT", 600))
DEFAULT_BUILD_TIMEOUT = int(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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.


dumux-adapter:
template: component-templates/dumux-adapter.yaml
build_timeout: 1800

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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 GLOBAL_TIMEOUT to 300s. The SU2 adapter probably also needs the 600s.

There will be more cases with longer timeouts later.

Comment thread tools/tests/README.md
### 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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
Build and run/compare use separate timeouts.
The build and and the run/compare steps use separate timeouts.

Comment thread tools/tests/README.md
Build and run/compare use separate timeouts.

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`.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Timeout for building system tests containers

2 participants