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.

Sign up for GitHub

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

Jump to bottom

Run all unit tests in buildkite #1321

Merged
merged 4 commits into from
Jun 16, 2023
Merged

Run all unit tests in buildkite #1321

merged 4 commits into from
Jun 16, 2023

Conversation

charleskawczynski
Copy link
Member

@charleskawczynski charleskawczynski commented Jun 13, 2023 •
edited
Loading

This PR:

  • moves all of the unit tests into buildkite.
  • Deletes the test/gpu/device.jl file, since it's from a deprecated module
  • Moves test/gpu/data.jl → test/DataLayouts/cuda.jl
  • Attempts to improve the grouping and labeling of the tests in the buildkite pipeline

I've also incorporated #1319.

This is a pretty big step towards #837. Making bors not depend on GHA would effectively close the issue.

@charleskawczynski
Copy link
Member Author

bors try

bors bot added a commit that referenced this pull request Jun 13, 2023
@bors
Try #1321:
183d3aa
@bors
Copy link
Contributor

bors bot commented Jun 13, 2023

try

Build failed:

@charleskawczynski charleskawczynski force-pushed the ck/rework_tests branch 2 times, most recently from 0de94ef to 810c229 Compare June 13, 2023 16:40
@charleskawczynski
Copy link
Member Author

bors try

bors bot added a commit that referenced this pull request Jun 13, 2023
@bors
Try #1321:
fa12b81
@charleskawczynski
Copy link
Member Author

bors try

@bors
Copy link
Contributor

bors bot commented Jun 13, 2023

try

Already running a review

@bors
Copy link
Contributor

bors bot commented Jun 13, 2023

try

Build failed:

@charleskawczynski
Copy link
Member Author

bors try

bors bot added a commit that referenced this pull request Jun 13, 2023
@bors
Try #1321:
10e2bd1
@bors
Copy link
Contributor

bors bot commented Jun 13, 2023

try

Build failed:

@charleskawczynski
Copy link
Member Author

bors try

bors bot added a commit that referenced this pull request Jun 13, 2023
@bors
Try #1321:
1cd8796
@bors
Copy link
Contributor

bors bot commented Jun 13, 2023

try

Build failed:

@charleskawczynski
Copy link
Member Author

bors try

bors bot added a commit that referenced this pull request Jun 13, 2023
@bors
Try #1321:
922d2ae
@bors
Copy link
Contributor

bors bot commented Jun 13, 2023

try

Build failed:

@charleskawczynski
Copy link
Member Author

bors try

bors bot added a commit that referenced this pull request Jun 13, 2023
@bors
Try #1321:
76f23f3
@bors
Copy link
Contributor

bors bot commented Jun 13, 2023

try

Build failed:

@charleskawczynski charleskawczynski marked this pull request as ready for review June 13, 2023 20:42
@simonbyrne
Copy link
Member

can we split out the tests which are triggered from runtests and those which are not? maybe move them to a test/extra directory?

@sriharshakandala
Copy link
Member

Is it practical to separate out tests that need to be run with --check-bounds=yes?

@charleskawczynski
Copy link
Member Author

can we split out the tests which are triggered from runtests and those which are not? maybe move them to a test/extra directory?

I was hoping to handle that more systematically in a follow-up PR. Right now in main, we don't have a clear partition what is included in runtests.jl / runtests_expensive.jl and what is not (basically, the distinction is what is included in buildkite vs GHA).

My thought was that we could parse the buildkite yaml file and include all unit tests (based on Unit: groups) in runtests.jl. We could also make sure to use the same environment variables, so that the tests are actually doing the same thing (they're not, at the moment) except for running on a different OS.

@charleskawczynski
Copy link
Member Author

Is it practical to separate out tests that need to be run with --check-bounds=yes?

Ah, good catch, I wasn't consistent with that. I think all unit tests should have --check-bounds=yes and all non-unit tests can omit --check-bounds. Does that sound okay?

@simonbyrne
Copy link
Member

It is possible that #1322 may fix the --check-bounds issue.

@charleskawczynski
Copy link
Member Author

It is possible that #1322 may fix the --check-bounds issue.

What's the issue with --check-bounds?

juliasloan25
juliasloan25 approved these changes Jun 15, 2023
View reviewed changes
Copy link
Member

@juliasloan25 juliasloan25 left a comment

Choose a reason for hiding this comment

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

This looks good to me

@charleskawczynski charleskawczynski force-pushed the ck/rework_tests branch 2 times, most recently from e5a4fb2 to 3537377 Compare June 16, 2023 06:23
@charleskawczynski
Copy link
Member Author

bors r+

bors bot added a commit that referenced this pull request Jun 16, 2023
@bors @charleskawczynski
Merge #1321
23b2c48 
1321: Run all unit tests in buildkite r=charleskawczynski a=charleskawczynski

This PR:
 - moves all of the unit tests into buildkite.
 - Deletes the `test/gpu/device.jl` file, since it's from a deprecated module
 - Moves `test/gpu/data.jl → test/DataLayouts/cuda.jl`
 - Attempts to improve the grouping and labeling of the tests in the buildkite pipeline

I've also incorporated #1319.

This is a pretty big step towards #837. Making bors not depend on GHA would effectively close the issue.

Co-authored-by: Charles Kawczynski <kawczynski.charles@gmail.com>
@charleskawczynski
Copy link
Member Author

bors r-

@bors
Copy link
Contributor

bors bot commented Jun 16, 2023

Canceled.

@charleskawczynski
Copy link
Member Author

bors r+

@bors
Copy link
Contributor

bors bot commented Jun 16, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 046c63f into main Jun 16, 2023
@bors bors bot deleted the ck/rework_tests branch June 16, 2023 07:16
@charleskawczynski charleskawczynski mentioned this pull request Jun 20, 2023
Closed
@charleskawczynski charleskawczynski mentioned this pull request Jul 17, 2023
Merged
bors bot added a commit that referenced this pull request Jul 17, 2023
@bors @charleskawczynski
Merge #1381
486b4c0 
1381: Add CUDA field tests r=charleskawczynski a=charleskawczynski

This PR adds CUDA field tests (and applies some fixes + adds some notes).

We should probably do this before #1380 so that we only need to slightly modify the test suite.

This was never actually tested. Here is a snapshot before #1321, which shows that `field.jl` was only on buildkite, and didn't have a gpu job: https://github.com/CliMA/ClimaCore.jl/tree/f38795b0769a6134afbba96ac1e66318c976ccd0

Co-authored-by: Charles Kawczynski <kawczynski.charles@gmail.com>
bors bot added a commit that referenced this pull request Jul 17, 2023
@bors @charleskawczynski
Merge #1381
69250a4 
1381: Add CUDA field tests r=charleskawczynski a=charleskawczynski

This PR adds CUDA field tests (and applies some fixes + adds some notes).

We should probably do this before #1380 so that we only need to slightly modify the test suite.

This was never actually tested. Here is a snapshot before #1321, which shows that `field.jl` was only on buildkite, and didn't have a gpu job: https://github.com/CliMA/ClimaCore.jl/tree/f38795b0769a6134afbba96ac1e66318c976ccd0

Co-authored-by: Charles Kawczynski <kawczynski.charles@gmail.com>
bors bot added a commit that referenced this pull request Jul 17, 2023
@bors @charleskawczynski
Merge #1381
b032b93 
1381: Add CUDA field tests r=charleskawczynski a=charleskawczynski

This PR adds CUDA field tests (and applies some fixes + adds some notes).

We should probably do this before #1380 so that we only need to slightly modify the test suite.

This was never actually tested. Here is a snapshot (https://github.com/CliMA/ClimaCore.jl/tree/f38795b0769a6134afbba96ac1e66318c976ccd0) before #1321, which shows that `field.jl` was only on buildkite, and didn't have a gpu job

Co-authored-by: Charles Kawczynski <kawczynski.charles@gmail.com>
bors bot added a commit that referenced this pull request Jul 17, 2023
@bors @charleskawczynski
Merge #1381
0e7a5f9 
1381: Add CUDA field tests r=charleskawczynski a=charleskawczynski

This PR adds CUDA field tests (and applies some fixes + adds some notes).

We should probably do this before #1380 so that we only need to slightly modify the test suite.

This was never actually tested. Here is a snapshot (https://github.com/CliMA/ClimaCore.jl/tree/f38795b0769a6134afbba96ac1e66318c976ccd0) before #1321, which shows that `field.jl` was only on buildkite, and didn't have a gpu job

Co-authored-by: Charles Kawczynski <kawczynski.charles@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simonbyrne simonbyrne simonbyrne approved these changes

@juliasloan25 juliasloan25 juliasloan25 approved these changes

@sriharshakandala sriharshakandala sriharshakandala approved these changes

@nefrathenrici nefrathenrici Awaiting requested review from nefrathenrici

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@charleskawczynski @simonbyrne @sriharshakandala @juliasloan25