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 2021/12/03 22:00:37 UTC
[GitHub] [arrow-rs] carols10cents opened a new pull request #1001: Address benchmarks that aren't compiling
carols10cents opened a new pull request #1001:
URL: https://github.com/apache/arrow-rs/pull/1001
# Which issue does this PR close?
Connects to #770. I'm not sure if this PR completely fixes it or not 😅
# Rationale for this change
The parquet benchmarks use some types that are behind the test_common feature. The benchmarks don't build without turning on that feature. CI doesn't currently check that benches build, nor is this feature requirement documented anywhere.
# What changes are included in this PR?
The first commit adds a job to CI to build (but not run) all benches in the workspace. I pushed this commit up first to demonstrate that it's currently failing.
The second commit adds `--features test_common` to the CI job, and it now passes. I've also documented the need for this flag in parquet/CONTRIBUTING.md.
Ideally, it'd be nice to be able to turn on a feature in Cargo.toml `[bench]` targets, but afaik that isn't possible currently.
Another possible solution is removing the `cfg` from these types, but I wasn't sure if that was desired or not.
# Are there any user-facing changes?
Users trying to build parquet's benchmarks should be able to if they read the docs or check what CI does :)
--
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 pull request #1001: Address benchmarks that aren't compiling
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1001:
URL: https://github.com/apache/arrow-rs/pull/1001#issuecomment-991229753
@carols10cents I have worked around the issue with the internal compiler error with rust in https://github.com/apache/arrow-rs/pull/1023 so if you merge from master this PR should be green now
--
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 pull request #1001: Address benchmarks that aren't compiling
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1001:
URL: https://github.com/apache/arrow-rs/pull/1001#issuecomment-996979018
Thanks @carols10cents
--
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] carols10cents commented on a change in pull request #1001: Address benchmarks that aren't compiling
Posted by GitBox <gi...@apache.org>.
carols10cents commented on a change in pull request #1001:
URL: https://github.com/apache/arrow-rs/pull/1001#discussion_r766087322
##########
File path: .github/workflows/rust.yml
##########
@@ -241,6 +241,46 @@ jobs:
export CARGO_TARGET_DIR="/github/home/target"
cargo clippy --features test_common --all-targets --workspace -- -D warnings -A clippy::redundant_field_names
+ build_benches:
+ name: Build Benchmarks (but don't run them)
+ runs-on: ubuntu-latest
+ strategy:
+ matrix:
+ arch: [amd64]
+ rust: [stable]
+ container:
+ image: ${{ matrix.arch }}/rust
+ env:
+ # Disable full debug symbol generation to speed up CI build and keep memory down
+ # "1" means line tables only, which is useful for panic tracebacks.
+ RUSTFLAGS: "-C debuginfo=1"
+ steps:
+ - uses: actions/checkout@v2
+ with:
+ submodules: true
+ - name: Cache Cargo
+ uses: actions/cache@v2
+ with:
+ path: /github/home/.cargo
+ # this key equals the ones on `linux-build-lib` for re-use
+ key: cargo-cache2-
+ - name: Cache Rust dependencies
+ uses: actions/cache@v2
+ with:
+ path: /github/home/target
+ # this key equals the ones on `linux-build-lib` for re-use
+ key: ${{ runner.os }}-${{ matrix.arch }}-target-cache-${{ matrix.rust }}
+ - name: Setup Rust toolchain
+ run: |
+ rustup toolchain install ${{ matrix.rust }}
+ rustup default ${{ matrix.rust }}
+ rustup component add rustfmt clippy
+ - name: Build benchmarks
+ run: |
+ export CARGO_HOME="/github/home/.cargo"
+ export CARGO_TARGET_DIR="/github/home/target"
+ cargo build --benches --features test_common --workspace
Review comment:
> that's strange, i was expecting any build error to be caught by the cargo clippy --features test_common run in the clippy job above 🤔
That job enables the `test_common` feature, which fixes the problem :) The original issue (https://github.com/apache/arrow-rs/issues/770) is that `cargo bench` by itself fails to compile.
--
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 #1001: Address benchmarks that aren't compiling
Posted by GitBox <gi...@apache.org>.
alamb merged pull request #1001:
URL: https://github.com/apache/arrow-rs/pull/1001
--
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] carols10cents commented on a change in pull request #1001: Address benchmarks that aren't compiling
Posted by GitBox <gi...@apache.org>.
carols10cents commented on a change in pull request #1001:
URL: https://github.com/apache/arrow-rs/pull/1001#discussion_r766084125
##########
File path: .github/workflows/rust.yml
##########
@@ -241,6 +241,46 @@ jobs:
export CARGO_TARGET_DIR="/github/home/target"
cargo clippy --features test_common --all-targets --workspace -- -D warnings -A clippy::redundant_field_names
+ build_benches:
+ name: Build Benchmarks (but don't run them)
+ runs-on: ubuntu-latest
+ strategy:
+ matrix:
+ arch: [amd64]
+ rust: [stable]
+ container:
+ image: ${{ matrix.arch }}/rust
+ env:
+ # Disable full debug symbol generation to speed up CI build and keep memory down
+ # "1" means line tables only, which is useful for panic tracebacks.
+ RUSTFLAGS: "-C debuginfo=1"
+ steps:
+ - uses: actions/checkout@v2
+ with:
+ submodules: true
+ - name: Cache Cargo
+ uses: actions/cache@v2
+ with:
+ path: /github/home/.cargo
+ # this key equals the ones on `linux-build-lib` for re-use
+ key: cargo-cache2-
+ - name: Cache Rust dependencies
+ uses: actions/cache@v2
+ with:
+ path: /github/home/target
+ # this key equals the ones on `linux-build-lib` for re-use
+ key: ${{ runner.os }}-${{ matrix.arch }}-target-cache-${{ matrix.rust }}
+ - name: Setup Rust toolchain
+ run: |
+ rustup toolchain install ${{ matrix.rust }}
+ rustup default ${{ matrix.rust }}
+ rustup component add rustfmt clippy
+ - name: Build benchmarks
+ run: |
+ export CARGO_HOME="/github/home/.cargo"
+ export CARGO_TARGET_DIR="/github/home/target"
+ cargo build --benches --features test_common --workspace
Review comment:
Ah, I can change it to `check`. I was keeping this new job consistent with the `wasm32-build` job.
--
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] Jimexist commented on a change in pull request #1001: Address benchmarks that aren't compiling
Posted by GitBox <gi...@apache.org>.
Jimexist commented on a change in pull request #1001:
URL: https://github.com/apache/arrow-rs/pull/1001#discussion_r763075942
##########
File path: .github/workflows/rust.yml
##########
@@ -241,6 +241,46 @@ jobs:
export CARGO_TARGET_DIR="/github/home/target"
cargo clippy --features test_common --all-targets --workspace -- -D warnings -A clippy::redundant_field_names
+ build_benches:
+ name: Build Benchmarks (but don't run them)
+ runs-on: ubuntu-latest
+ strategy:
+ matrix:
+ arch: [amd64]
+ rust: [stable]
+ container:
+ image: ${{ matrix.arch }}/rust
+ env:
+ # Disable full debug symbol generation to speed up CI build and keep memory down
+ # "1" means line tables only, which is useful for panic tracebacks.
+ RUSTFLAGS: "-C debuginfo=1"
+ steps:
+ - uses: actions/checkout@v2
+ with:
+ submodules: true
+ - name: Cache Cargo
+ uses: actions/cache@v2
+ with:
+ path: /github/home/.cargo
+ # this key equals the ones on `linux-build-lib` for re-use
+ key: cargo-cache2-
+ - name: Cache Rust dependencies
+ uses: actions/cache@v2
+ with:
+ path: /github/home/target
+ # this key equals the ones on `linux-build-lib` for re-use
+ key: ${{ runner.os }}-${{ matrix.arch }}-target-cache-${{ matrix.rust }}
+ - name: Setup Rust toolchain
+ run: |
+ rustup toolchain install ${{ matrix.rust }}
+ rustup default ${{ matrix.rust }}
+ rustup component add rustfmt clippy
+ - name: Build benchmarks
+ run: |
+ export CARGO_HOME="/github/home/.cargo"
+ export CARGO_TARGET_DIR="/github/home/target"
+ cargo build --benches --features test_common --workspace
Review comment:
how about just `cargo check`?
--
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 change in pull request #1001: Address benchmarks that aren't compiling
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1001:
URL: https://github.com/apache/arrow-rs/pull/1001#discussion_r766921216
##########
File path: .github/workflows/rust.yml
##########
@@ -241,6 +241,46 @@ jobs:
export CARGO_TARGET_DIR="/github/home/target"
cargo clippy --features test_common --all-targets --workspace -- -D warnings -A clippy::redundant_field_names
+ check_benches:
+ name: Check Benchmarks (but don't run them)
+ runs-on: ubuntu-latest
+ strategy:
+ matrix:
+ arch: [amd64]
+ rust: [stable]
+ container:
+ image: ${{ matrix.arch }}/rust
+ env:
+ # Disable full debug symbol generation to speed up CI build and keep memory down
+ # "1" means line tables only, which is useful for panic tracebacks.
+ RUSTFLAGS: "-C debuginfo=1"
+ steps:
+ - uses: actions/checkout@v2
+ with:
+ submodules: true
+ - name: Cache Cargo
+ uses: actions/cache@v2
+ with:
+ path: /github/home/.cargo
+ # this key equals the ones on `linux-build-lib` for re-use
+ key: cargo-cache2-
Review comment:
```suggestion
key: cargo-cache3-
```
--
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] carols10cents commented on pull request #1001: Address benchmarks that aren't compiling
Posted by GitBox <gi...@apache.org>.
carols10cents commented on pull request #1001:
URL: https://github.com/apache/arrow-rs/pull/1001#issuecomment-990223147
Ok, revised: the [first commit fails at the new CI job](https://github.com/apache/arrow-rs/runs/4475547541?check_suite_focus=true), the [second commit passes](https://github.com/apache/arrow-rs/runs/4475642034?check_suite_focus=true).
The failing job looks like an ICE with Rust nightly that should be fixed in the next nighly; I'll try to rerun tomorrow.
--
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] houqp commented on a change in pull request #1001: Address benchmarks that aren't compiling
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #1001:
URL: https://github.com/apache/arrow-rs/pull/1001#discussion_r762395208
##########
File path: .github/workflows/rust.yml
##########
@@ -241,6 +241,46 @@ jobs:
export CARGO_TARGET_DIR="/github/home/target"
cargo clippy --features test_common --all-targets --workspace -- -D warnings -A clippy::redundant_field_names
+ build_benches:
+ name: Build Benchmarks (but don't run them)
+ runs-on: ubuntu-latest
+ strategy:
+ matrix:
+ arch: [amd64]
+ rust: [stable]
+ container:
+ image: ${{ matrix.arch }}/rust
+ env:
+ # Disable full debug symbol generation to speed up CI build and keep memory down
+ # "1" means line tables only, which is useful for panic tracebacks.
+ RUSTFLAGS: "-C debuginfo=1"
+ steps:
+ - uses: actions/checkout@v2
+ with:
+ submodules: true
+ - name: Cache Cargo
+ uses: actions/cache@v2
+ with:
+ path: /github/home/.cargo
+ # this key equals the ones on `linux-build-lib` for re-use
+ key: cargo-cache2-
+ - name: Cache Rust dependencies
+ uses: actions/cache@v2
+ with:
+ path: /github/home/target
+ # this key equals the ones on `linux-build-lib` for re-use
+ key: ${{ runner.os }}-${{ matrix.arch }}-target-cache-${{ matrix.rust }}
+ - name: Setup Rust toolchain
+ run: |
+ rustup toolchain install ${{ matrix.rust }}
+ rustup default ${{ matrix.rust }}
+ rustup component add rustfmt clippy
+ - name: Build benchmarks
+ run: |
+ export CARGO_HOME="/github/home/.cargo"
+ export CARGO_TARGET_DIR="/github/home/target"
+ cargo build --benches --features test_common --workspace
Review comment:
that's strange, i was expecting any build error to be caught by the `cargo clippy --features test_common` run in the clippy job above :thinking:
--
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] carols10cents commented on pull request #1001: Address benchmarks that aren't compiling
Posted by GitBox <gi...@apache.org>.
carols10cents commented on pull request #1001:
URL: https://github.com/apache/arrow-rs/pull/1001#issuecomment-990195835
> We might able to fix it by adding `required-features`.
OOooohhh!! TIL!! This seems to work, update incoming!
--
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 change in pull request #1001: Address benchmarks that aren't compiling
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1001:
URL: https://github.com/apache/arrow-rs/pull/1001#discussion_r766921216
##########
File path: .github/workflows/rust.yml
##########
@@ -241,6 +241,46 @@ jobs:
export CARGO_TARGET_DIR="/github/home/target"
cargo clippy --features test_common --all-targets --workspace -- -D warnings -A clippy::redundant_field_names
+ check_benches:
+ name: Check Benchmarks (but don't run them)
+ runs-on: ubuntu-latest
+ strategy:
+ matrix:
+ arch: [amd64]
+ rust: [stable]
+ container:
+ image: ${{ matrix.arch }}/rust
+ env:
+ # Disable full debug symbol generation to speed up CI build and keep memory down
+ # "1" means line tables only, which is useful for panic tracebacks.
+ RUSTFLAGS: "-C debuginfo=1"
+ steps:
+ - uses: actions/checkout@v2
+ with:
+ submodules: true
+ - name: Cache Cargo
+ uses: actions/cache@v2
+ with:
+ path: /github/home/.cargo
+ # this key equals the ones on `linux-build-lib` for re-use
+ key: cargo-cache2-
Review comment:
```suggestion
key: cargo-cache3-
```
##########
File path: .github/workflows/rust.yml
##########
@@ -241,6 +241,46 @@ jobs:
export CARGO_TARGET_DIR="/github/home/target"
cargo clippy --features test_common --all-targets --workspace -- -D warnings -A clippy::redundant_field_names
+ check_benches:
+ name: Check Benchmarks (but don't run them)
+ runs-on: ubuntu-latest
+ strategy:
+ matrix:
+ arch: [amd64]
+ rust: [stable]
+ container:
+ image: ${{ matrix.arch }}/rust
+ env:
+ # Disable full debug symbol generation to speed up CI build and keep memory down
+ # "1" means line tables only, which is useful for panic tracebacks.
+ RUSTFLAGS: "-C debuginfo=1"
+ steps:
+ - uses: actions/checkout@v2
+ with:
+ submodules: true
+ - name: Cache Cargo
+ uses: actions/cache@v2
+ with:
+ path: /github/home/.cargo
+ # this key equals the ones on `linux-build-lib` for re-use
+ key: cargo-cache2-
+ - name: Cache Rust dependencies
+ uses: actions/cache@v2
+ with:
+ path: /github/home/target
+ # this key equals the ones on `linux-build-lib` for re-use
+ key: ${{ runner.os }}-${{ matrix.arch }}-target-cache-${{ matrix.rust }}
Review comment:
```suggestion
key: ${{ runner.os }}-${{ matrix.arch }}-target-cache3-${{ matrix.rust }}
```
##########
File path: .github/workflows/rust.yml
##########
@@ -241,6 +241,46 @@ jobs:
export CARGO_TARGET_DIR="/github/home/target"
cargo clippy --features test_common --all-targets --workspace -- -D warnings -A clippy::redundant_field_names
+ check_benches:
+ name: Check Benchmarks (but don't run them)
+ runs-on: ubuntu-latest
+ strategy:
+ matrix:
+ arch: [amd64]
+ rust: [stable]
+ container:
+ image: ${{ matrix.arch }}/rust
+ env:
+ # Disable full debug symbol generation to speed up CI build and keep memory down
+ # "1" means line tables only, which is useful for panic tracebacks.
+ RUSTFLAGS: "-C debuginfo=1"
+ steps:
+ - uses: actions/checkout@v2
+ with:
+ submodules: true
+ - name: Cache Cargo
+ uses: actions/cache@v2
+ with:
+ path: /github/home/.cargo
+ # this key equals the ones on `linux-build-lib` for re-use
+ key: cargo-cache2-
Review comment:
```suggestion
key: cargo-cache3-
```
To align with cache keys added in https://github.com/apache/arrow-rs/pull/1023
--
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 change in pull request #1001: Address benchmarks that aren't compiling
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1001:
URL: https://github.com/apache/arrow-rs/pull/1001#discussion_r766921325
##########
File path: .github/workflows/rust.yml
##########
@@ -241,6 +241,46 @@ jobs:
export CARGO_TARGET_DIR="/github/home/target"
cargo clippy --features test_common --all-targets --workspace -- -D warnings -A clippy::redundant_field_names
+ check_benches:
+ name: Check Benchmarks (but don't run them)
+ runs-on: ubuntu-latest
+ strategy:
+ matrix:
+ arch: [amd64]
+ rust: [stable]
+ container:
+ image: ${{ matrix.arch }}/rust
+ env:
+ # Disable full debug symbol generation to speed up CI build and keep memory down
+ # "1" means line tables only, which is useful for panic tracebacks.
+ RUSTFLAGS: "-C debuginfo=1"
+ steps:
+ - uses: actions/checkout@v2
+ with:
+ submodules: true
+ - name: Cache Cargo
+ uses: actions/cache@v2
+ with:
+ path: /github/home/.cargo
+ # this key equals the ones on `linux-build-lib` for re-use
+ key: cargo-cache2-
+ - name: Cache Rust dependencies
+ uses: actions/cache@v2
+ with:
+ path: /github/home/target
+ # this key equals the ones on `linux-build-lib` for re-use
+ key: ${{ runner.os }}-${{ matrix.arch }}-target-cache-${{ matrix.rust }}
Review comment:
```suggestion
key: ${{ runner.os }}-${{ matrix.arch }}-target-cache3-${{ matrix.rust }}
```
##########
File path: .github/workflows/rust.yml
##########
@@ -241,6 +241,46 @@ jobs:
export CARGO_TARGET_DIR="/github/home/target"
cargo clippy --features test_common --all-targets --workspace -- -D warnings -A clippy::redundant_field_names
+ check_benches:
+ name: Check Benchmarks (but don't run them)
+ runs-on: ubuntu-latest
+ strategy:
+ matrix:
+ arch: [amd64]
+ rust: [stable]
+ container:
+ image: ${{ matrix.arch }}/rust
+ env:
+ # Disable full debug symbol generation to speed up CI build and keep memory down
+ # "1" means line tables only, which is useful for panic tracebacks.
+ RUSTFLAGS: "-C debuginfo=1"
+ steps:
+ - uses: actions/checkout@v2
+ with:
+ submodules: true
+ - name: Cache Cargo
+ uses: actions/cache@v2
+ with:
+ path: /github/home/.cargo
+ # this key equals the ones on `linux-build-lib` for re-use
+ key: cargo-cache2-
Review comment:
```suggestion
key: cargo-cache3-
```
To align with cache keys added in https://github.com/apache/arrow-rs/pull/1023
--
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 pull request #1001: Address benchmarks that aren't compiling
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1001:
URL: https://github.com/apache/arrow-rs/pull/1001#issuecomment-991229753
@carols10cents I have worked around the issue with the internal compiler error with rust in https://github.com/apache/arrow-rs/pull/1023 so if you merge from master this PR should be green now
--
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