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

remove redundant CI benchmark check, cleanups #2212

Merged
merged 5 commits into from
Jul 28, 2022
Merged

remove redundant CI benchmark check, cleanups #2212

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6be18e2
remove redundant CI benchmark check, cleanups
alamb Jul 28, 2022
d9d04cd
cleanup
alamb Jul 28, 2022
6cd6fa1
Merge remote-tracking branch 'apache/master' into alamb/cleanup_ci
alamb Jul 28, 2022
0598606
Clarify the intent of parquet library compilation check, and add miss…
alamb Jul 28, 2022
2c36c68
Update .github/workflows/parquet.yml
alamb Jul 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 18 additions & 4 deletions .github/workflows/parquet.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

# all targets to ensure that it still works in the absence of
# features that might be enabled by dev-dependencies of other
# targets.
#
# This for each of (library and all-targets), check
# 1. compiles with default features
# 1. compiles with no default features
# 3. compiles with just arrow feature
# 3. compiles with all features
- name: Check compilation
run: |
alamb marked this conversation as resolved.
Show resolved Hide resolved
cargo check -p parquet
Expand All @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved --all-targets to the first part the command line to make it easier to verify that all the combinations were checked

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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a the missing combination


clippy:
name: Clippy
Expand Down
33 changes: 0 additions & 33 deletions .github/workflows/rust.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,36 +47,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:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is some evidence that this is already covered

When I broke an arrow benchmark:

echo "fff" >> arrow/benches/string_dictionary_builder.rs

Then I ran the check that is part of arrow:

arrow-rs/.github/workflows/arrow.yml

Line 88 in bc493d9

cargo check -p arrow --all-targets 

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

echo "ggg" >> parquet/benches/arrow_reader.rs

A check run by the parquet tests also finds it

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 `::`

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: Setup Rust toolchain
uses: ./.github/actions/setup-builder
with:
rust-version: ${{ matrix.rust }}
- name: Check benchmarks
run: |
cargo check --benches --workspace --features test_common,prettyprint,async,experimental

# Run cargo fmt for all crates
lint:
Expand Down Expand Up @@ -113,16 +87,9 @@ jobs:
uses: actions/cache@v3
with:
path: /home/runner/.cargo
# this key is not equal because the user is different than on a container (runner vs github)
key: cargo-coverage-cache3-
- name: Run coverage
run: |
export CARGO_HOME="/home/runner/.cargo"
export CARGO_TARGET_DIR="/home/runner/target"

export ARROW_TEST_DATA=$(pwd)/testing/data
export PARQUET_TEST_DATA=$(pwd)/parquet-testing/data

rustup toolchain install stable
rustup default stable
cargo install --version 0.18.2 cargo-tarpaulin
Expand Down