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

CI: Only run coverage jobs on master #2214

Merged
merged 2 commits into from
Aug 4, 2022

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Jul 28, 2022

Draft until #2212

Which issue does this PR close?

re #2149

Rationale for this change

The codecov job is long (example takes 20 minutes, and I haven't seen its results referred to in PRs myself.

It also has several unfortunate issues (like not accounting for coverage of doc tests), which we should probably file as a separate issue if anyone is looking at its results.

Thus, let's only run it on pushes to master

What changes are included in this PR?

  1. Only run coverage job on pushes to master

Are there any user-facing changes?

no

@alamb alamb added the development-process Related to development process of arrow-rs label Jul 28, 2022
@alamb alamb marked this pull request as ready for review July 28, 2022 18:16
@alamb alamb marked this pull request as draft July 28, 2022 18:17
@alamb alamb marked this pull request as ready for review July 28, 2022 20:06
Copy link
Contributor

@tustvold tustvold left a comment

Choose a reason for hiding this comment

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

Won't this mean we lose information about test coverage in PRs? I personally don't regular inspect this, but some people might?

@alamb
Copy link
Contributor Author

alamb commented Jul 29, 2022

Won't this mean we lose information about test coverage in PRs?

Yes that is correct.

@nevi-me @HaoYang670 @jhorstmann @viirya @sunchao do you ever use the per PR code coverage report ? I am proposing to only run coverage jobs on master to improve our CI

@nevi-me
Copy link
Contributor

nevi-me commented Jul 29, 2022

Won't this mean we lose information about test coverage in PRs?

Yes that is correct.

@nevi-me @HaoYang670 @jhorstmann @viirya @sunchao do you ever use the per PR code coverage report ? I am proposing to only run coverage jobs on master to improve our CI

An alternative could be to run coverage as part of tests, via cargo--llvm-cov, which should also be faster. I'll make changes and PR them into this PR so we can discuss it.

I locally use llvm-cov so I do benefit from Rust coverage, but I'm inactive on arrow-rs at the moment, and would be fine if the decision was to disable coverage altogether if it isn't being used.

@nevi-me
Copy link
Contributor

nevi-me commented Jul 29, 2022

On second thoughts, I believe we can't use certain actions, I wanted to add taiki-e/install-action@cargo-llvm-cov, but that might need INFRA approval.

llvm-cov is generally faster than tarpaulin though, so I'll explore getting it to run without the action above

@alamb
Copy link
Contributor Author

alamb commented Jul 29, 2022

llvm-cov is generally faster than tarpaulin though, so I'll explore getting it to run without the action above

Thanks @nevi-me !

Note there is already one simple action in the arrow-rs repository https://github.com/apache/arrow-rs/tree/master/.github/actions/setup-builder in case you wanted to add another for coverage

@alamb alamb marked this pull request as draft July 29, 2022 11:31
@alamb
Copy link
Contributor Author

alamb commented Jul 29, 2022

Marking as a draft so we don't merge this PR accidentally

@nevi-me
Copy link
Contributor

nevi-me commented Jul 29, 2022

Okay, thanks, I'll work on this on the weekend, if I can't make llvm-cov work, we can move to using tarpaulin only on master.

@alamb
Copy link
Contributor Author

alamb commented Jul 29, 2022

For what it is worth I don't think this ticket is super high priority -- I was just going through all the CI checks in general (on the occasion of the object_store crate being merged and making the current intertwined ness just a bit too much to bear).

@viirya
Copy link
Member

viirya commented Jul 29, 2022

I personally don't use it too much, so the idea of running coverage jobs on master is okay for me. But @nevi-me's proposal is awesome if it works. Thanks @nevi-me for working on it.

@tustvold
Copy link
Contributor

tustvold commented Aug 4, 2022

Shall we update and get this one in, there seems to be fairly broad consensus?

@alamb
Copy link
Contributor Author

alamb commented Aug 4, 2022

Shall we update and get this one in, there seems to be fairly broad consensus?

👍 I will do so. We'll save some CO2 and maybe help the other PRs to run a bit faster. We can always re-enable if someone wants to see it.

@alamb
Copy link
Contributor Author

alamb commented Aug 4, 2022

I don't really know how to test this other than on master, so YOLO here we go:

@alamb alamb marked this pull request as ready for review August 4, 2022 19:08
@alamb alamb merged commit 5166a08 into apache:master Aug 4, 2022
@alamb alamb deleted the alamb/split_up_coverage branch August 4, 2022 19:08
@alamb
Copy link
Contributor Author

alamb commented Aug 4, 2022

Well, this is promising -- the job is queued to run on the master commit:

5166a08

https://github.com/apache/arrow-rs/runs/7679145290?check_suite_focus=true

@ursabot
Copy link

ursabot commented Aug 4, 2022

Benchmark runs are scheduled for baseline = d87f6a4 and contender = 5166a08. 5166a08 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development-process Related to development process of arrow-rs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants