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.

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

Test Windows runtime on presubmit, merge build/test runtime jobs. #11032

Merged
merged 2 commits into from
Nov 7, 2022

Conversation

ScottTodd
Copy link
Member

@ScottTodd ScottTodd commented Nov 3, 2022

Progress on #11009, depends on #11048

Changes:

  • build_runtime + test_runtime -> build_test_runtime (overhead from repository cloning, artifact upload, and artifact download was taking longer than just running the tests from the same job)
  • build_runtime_windows -> build_test_runtime_windows
    • Runs on managed-windows-cpu (larger build machine)
    • Runs tests, instead of just builds (now that all runtime tests pass on Windows)
    • Runs on presubmit now too, instead of just postsubmit (the build appears to be stable)

Sample run: https://github.com/iree-org/iree/actions/runs/3412369869/jobs/5677798847

@ScottTodd ScottTodd added infrastructure Relating to build systems, CI, or testing platform/windows 🚪 Windows-specific build, execution, benchmarking, and deployment labels Nov 3, 2022
@ScottTodd ScottTodd force-pushed the windows-ci-runtime-tests branch 5 times, most recently from fd8cbad to 55beccb Compare November 4, 2022 18:35
@ScottTodd ScottTodd changed the title Add test_runtime_windows job, run on presubmit. Test Windows runtime on presubmit, merge build/test runtime jobs. Nov 4, 2022
@ScottTodd ScottTodd requested a review from GMNGeoffrey November 4, 2022 18:39
--env IREE_VULKAN_DISABLE=1 \
gcr.io/iree-oss/base@sha256:5d43683c6b50aebe1fca6c85f2012f3b0fa153bf4dd268e8767b619b1891423a \
./build_tools/cmake/ctest_all.sh \
"${BUILD_DIR}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe make this a single docker run call as in our other build&test workflows

Copy link
Member Author

Choose a reason for hiding this comment

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

I like having build and test in separate steps, so we get a nice timing breakdown in the action summary:

image

Duplicating the docker command in each step does repeat many characters though. Can we put our Docker image in uses? https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#example-using-a-docker-hub-action Or set a container? https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idcontainer

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not just characters repeating: it's also tearing down and relaunching the container (although I'm not sure how long that takes, in practice. Apparently less than 3s). I've steered away from GitHub actions specific ways of using Docker containers because they're more tied to the CI and therefore harder to reproduce locally or migrate to other CI systems. That includes translating all the docker run options into the GitHub Actions syntax. The script for doing this does additional setup (like creating directories to mount), that would have to be sharded out elsewhere. Overall, it just seems like a false simplification. One option we have would be to start the container running in one step and then use docker exec to run various commands in it.

Regardless, what you've got here is fine for now, though I do still have a slight preference for doing it in a single step to match how other jobs work and avoid launching a container multiple times (although that doesn't seem to have meaningful latency overhead). If you wanted to follow-up by splitting all of them, then that would be fine too :-P

@ScottTodd ScottTodd force-pushed the windows-ci-runtime-tests branch from 55beccb to 03ec4bd Compare November 4, 2022 22:38
@ScottTodd ScottTodd marked this pull request as ready for review November 4, 2022 22:38
@ScottTodd
Copy link
Member Author

Okay, it's no longer Friday afternoon and I've synced with main. @GMNGeoffrey want to comment on the usage of Docker before I merge? (I also think there's plenty of room for iteration on the Linux and Windows jobs after landing this)

--env IREE_VULKAN_DISABLE=1 \
gcr.io/iree-oss/base@sha256:5d43683c6b50aebe1fca6c85f2012f3b0fa153bf4dd268e8767b619b1891423a \
./build_tools/cmake/ctest_all.sh \
"${BUILD_DIR}"
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not just characters repeating: it's also tearing down and relaunching the container (although I'm not sure how long that takes, in practice. Apparently less than 3s). I've steered away from GitHub actions specific ways of using Docker containers because they're more tied to the CI and therefore harder to reproduce locally or migrate to other CI systems. That includes translating all the docker run options into the GitHub Actions syntax. The script for doing this does additional setup (like creating directories to mount), that would have to be sharded out elsewhere. Overall, it just seems like a false simplification. One option we have would be to start the container running in one step and then use docker exec to run various commands in it.

Regardless, what you've got here is fine for now, though I do still have a slight preference for doing it in a single step to match how other jobs work and avoid launching a container multiple times (although that doesn't seem to have meaningful latency overhead). If you wanted to follow-up by splitting all of them, then that would be fine too :-P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure Relating to build systems, CI, or testing platform/windows 🚪 Windows-specific build, execution, benchmarking, and deployment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants