You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/07/28 16:29:54 UTC
[GitHub] [arrow-rs] alamb opened a new pull request, #2212: remove redundant CI benchmark check, cleanups
alamb opened a new pull request, #2212:
URL: https://github.com/apache/arrow-rs/pull/2212
# Which issue does this PR close?
re https://github.com/apache/arrow-rs/issues/2149
# Rationale for this change
Clean up CI so we don't have to deal with jobs that are not adding value
# What changes are included in this PR?
1. Remove redundeant benchmark check job (it is already covered by other jobs -- I will comment inline)
2. Remove unecessary environment variables from win/mac jobs
# Are there any user-facing changes?
No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on a diff in pull request #2212: remove redundant CI benchmark check, cleanups
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #2212:
URL: https://github.com/apache/arrow-rs/pull/2212#discussion_r932597841
##########
.github/workflows/parquet.yml:
##########
@@ -76,6 +76,17 @@ jobs:
uses: ./.github/actions/setup-builder
with:
rust-version: stable
+
+ # Run different tests for the library on its own as well as
Review Comment:
I tried to clarify the intent of the tests
This also makes it clear (to me) that there should be 8 combinations, and so I also added the missing combination (`--all-targets --all-features`)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on a diff in pull request #2212: remove redundant CI benchmark check, cleanups
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #2212:
URL: https://github.com/apache/arrow-rs/pull/2212#discussion_r932598031
##########
.github/workflows/parquet.yml:
##########
@@ -72,26 +72,17 @@ jobs:
with:
rust-version: stable
- name: Check compilation
- run: |
Review Comment:
Fixed in https://github.com/apache/arrow-rs/pull/2212/commits/05986065ce5cdf6a76de646157cc2198f9515028
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on a diff in pull request #2212: remove redundant CI benchmark check, cleanups
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #2212:
URL: https://github.com/apache/arrow-rs/pull/2212#discussion_r932598588
##########
.github/workflows/parquet.yml:
##########
@@ -91,12 +102,15 @@ jobs:
- name: Check compilation --all-targets
run: |
cargo check -p parquet --all-targets
- - name: Check compilation --no-default-features --all-targets
+ - name: Check compilation --all-targets --no-default-features
Review Comment:
I moved `--all-targets` to the first part the command line to make it easier to verify that all the combinations were checked
##########
.github/workflows/parquet.yml:
##########
@@ -91,12 +102,15 @@ jobs:
- name: Check compilation --all-targets
run: |
cargo check -p parquet --all-targets
- - name: Check compilation --no-default-features --all-targets
+ - name: Check compilation --all-targets --no-default-features
+ run: |
+ cargo check -p parquet --all-targets --no-default-features
+ - name: Check compilation --all-targets --no-default-features --features-arrow
run: |
- cargo check -p parquet --no-default-features --all-targets
- - name: Check compilation --no-default-features --features-arrow --all-targets
+ cargo check -p parquet --all-targets --no-default-features --features arrow
+ - name: Check compilation --all-targets --all-features
run: |
- cargo check -p parquet --no-default-features --features arrow --all-targets
+ cargo check -p parquet --all-targets --all-features
Review Comment:
This is a the missing combination
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on a diff in pull request #2212: remove redundant CI benchmark check, cleanups
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #2212:
URL: https://github.com/apache/arrow-rs/pull/2212#discussion_r932573195
##########
.github/workflows/parquet.yml:
##########
@@ -72,26 +72,17 @@ jobs:
with:
rust-version: stable
- name: Check compilation
- run: |
Review Comment:
Got it -- will fix
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] codecov-commenter commented on pull request #2212: remove redundant CI benchmark check, cleanups
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #2212:
URL: https://github.com/apache/arrow-rs/pull/2212#issuecomment-1198443296
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/2212?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#2212](https://codecov.io/gh/apache/arrow-rs/pull/2212?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3a4c624) into [master](https://codecov.io/gh/apache/arrow-rs/commit/bc493d92c2a032a95d92dab81642f05182f20d81?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (bc493d9) will **increase** coverage by `0.00%`.
> The diff coverage is `n/a`.
> :exclamation: Current head 3a4c624 differs from pull request most recent head d9d04cd. Consider uploading reports for the commit d9d04cd to get more accurate results
```diff
@@ Coverage Diff @@
## master #2212 +/- ##
=======================================
Coverage 82.56% 82.57%
=======================================
Files 239 239
Lines 62269 62269
=======================================
+ Hits 51415 51416 +1
+ Misses 10854 10853 -1
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/2212?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [parquet\_derive/src/parquet\_field.rs](https://codecov.io/gh/apache/arrow-rs/pull/2212/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldF9kZXJpdmUvc3JjL3BhcnF1ZXRfZmllbGQucnM=) | `65.75% <0.00%> (-0.23%)` | :arrow_down: |
| [parquet/src/encodings/encoding.rs](https://codecov.io/gh/apache/arrow-rs/pull/2212/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvZW5jb2RpbmdzL2VuY29kaW5nLnJz) | `93.62% <0.00%> (+0.19%)` | :arrow_up: |
| [...rquet/src/arrow/record\_reader/definition\_levels.rs](https://codecov.io/gh/apache/arrow-rs/pull/2212/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvYXJyb3cvcmVjb3JkX3JlYWRlci9kZWZpbml0aW9uX2xldmVscy5ycw==) | `89.02% <0.00%> (+0.42%)` | :arrow_up: |
Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #2212: remove redundant CI benchmark check, cleanups
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2212:
URL: https://github.com/apache/arrow-rs/pull/2212#discussion_r932523305
##########
.github/workflows/parquet.yml:
##########
@@ -72,26 +72,17 @@ jobs:
with:
rust-version: stable
- name: Check compilation
- run: |
Review Comment:
This breaks the validation that libraries compile on their own in the absence of features that might be enabled by dev-dependencies - see #1822
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #2212: remove redundant CI benchmark check, cleanups
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2212:
URL: https://github.com/apache/arrow-rs/pull/2212#discussion_r932603855
##########
.github/workflows/parquet.yml:
##########
@@ -91,12 +102,15 @@ jobs:
- name: Check compilation --all-targets
run: |
cargo check -p parquet --all-targets
- - name: Check compilation --no-default-features --all-targets
+ - name: Check compilation --all-targets --no-default-features
+ run: |
+ cargo check -p parquet --all-targets --no-default-features
+ - name: Check compilation --all-targets --no-default-features --features-arrow
Review Comment:
```suggestion
- name: Check compilation --all-targets --no-default-features --features arrow
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] ursabot commented on pull request #2212: remove redundant CI benchmark check, cleanups
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #2212:
URL: https://github.com/apache/arrow-rs/pull/2212#issuecomment-1198586327
Benchmark runs are scheduled for baseline = 8139f7bcd147a931fbbb5599327ae2b36a424925 and contender = 48cc6c3e9fea0027055990f9b398eb4b341d0697. 48cc6c3e9fea0027055990f9b398eb4b341d0697 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/3eee06f7b27840968d463c5d2380fd6e...4fbe7fcbbe5442db9915e15f1d65e726/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/889092e8f690425280ae740d96f72fa6...9279c662fcd946e796a6b9c7efa476fc/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/ff5e8e46cefe4828936729d72df1b617...f5fd3611325e4281950fc20f5d50a203/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/5f978ae483b24430b1bae8d7480bf1a0...1a1ae0992c5e4e4dab6cbf540bbde01f/)
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #2212: remove redundant CI benchmark check, cleanups
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2212:
URL: https://github.com/apache/arrow-rs/pull/2212#discussion_r932523305
##########
.github/workflows/parquet.yml:
##########
@@ -72,26 +72,17 @@ jobs:
with:
rust-version: stable
- name: Check compilation
- run: |
Review Comment:
This breaks the validation that libraries compile on their own - see #1822
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on a diff in pull request #2212: remove redundant CI benchmark check, cleanups
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #2212:
URL: https://github.com/apache/arrow-rs/pull/2212#discussion_r932464002
##########
.github/workflows/rust.yml:
##########
@@ -44,37 +44,10 @@ jobs:
- name: Run tests
shell: bash
run: |
- export ARROW_TEST_DATA=$(pwd)/testing/data
- export PARQUET_TEST_DATA=$(pwd)/parquet-testing/data
# do not produce debug symbols to keep memory usage down
export RUSTFLAGS="-C debuginfo=0"
cargo test
- check_benches:
Review Comment:
Here is some evidence that this is already covered
When I broke an arrow benchmark:
```shell
echo "fff" >> arrow/benches/string_dictionary_builder.rs
```
Then I ran the check that is part of arrow: https://github.com/apache/arrow-rs/blob/bc493d92c2a032a95d92dab81642f05182f20d81/.github/workflows/arrow.yml#L88
```shell
cargo check -p arrow --all-targets
Checking arrow v19.0.0 (/Users/alamb/Software/arrow-rs2/arrow)
error: expected one of `!` or `::`, found `<eof>`
--> arrow/benches/string_dictionary_builder.rs:71:1
|
71 | fff
| ^^^ expected one of `!` or `::`
error: could not compile `arrow` due to previous error
warning: build failed, waiting for other jobs to finish...
```
Similarly, when I broke parquet
```shell
echo "ggg" >> parquet/benches/arrow_reader.rs
```
A check run by the parquet tests also finds it
```shell
cargo check -p parquet --all-features --all-targets
Compiling parquet v19.0.0 (/Users/alamb/Software/arrow-rs2/parquet)
error: expected one of `!` or `::`, found `<eof>`
--> parquet/benches/arrow_reader.rs:695:1
|
695 | ggg
| ^^^ expected one of `!` or `::`
```
##########
.github/workflows/parquet.yml:
##########
@@ -72,26 +72,17 @@ jobs:
with:
rust-version: stable
- name: Check compilation
- run: |
Review Comment:
I didn't see any reason to run checks that did not have `--all-targets` -- so this change ensures that we test with the following combinations:
1. default features,
2. `--all-features`
3. `--no-default-features --features=arrow`
With all targets
Previously, `--all-features --all-targets` was not run checked (and thus the becnhmarks were not checked)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb merged pull request #2212: remove redundant CI benchmark check, cleanups
Posted by GitBox <gi...@apache.org>.
alamb merged PR #2212:
URL: https://github.com/apache/arrow-rs/pull/2212
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org