Skip to content

[CI] Improve windows machine CI test time#8730

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

Merged
alamb merged 10 commits into from
Jan 3, 2024
Merged

[CI] Improve windows machine CI test time #8730

Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ea3a46a
Test WIN64 CI
comphead Jan 3, 2024
9342986
Test WIN64 CI
comphead Jan 3, 2024
d057232
Test WIN64 CI
comphead Jan 3, 2024
86ab449
Test WIN64 CI
comphead Jan 3, 2024
14e65fa
Adding incremental compilation
comphead Jan 3, 2024
f36935a
Adding codegen units
comphead Jan 3, 2024
194f289
Try without opt-level
comphead Jan 3, 2024
3c6b5bb
set opt level only for win machines
comphead Jan 3, 2024
8df6786
set opt level only for win machines. remove incremental compile
comphead Jan 3, 2024
3041701
update comments
comphead Jan 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 21 additions & 3 deletions .github/workflows/rust.yml
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -99,6 +99,14 @@ jobs:
rust-version: stable
- name: Run tests (excluding doctests)
run: cargo test --lib --tests --bins --features avro,json,backtrace
env:
# do not produce debug symbols to keep memory usage down
Comment thread
comphead marked this conversation as resolved.
# hardcoding other profile params to avoid profile override values
# More on Cargo profiles https://doc.rust-lang.org/cargo/reference/profiles.html?profile-settings#profile-settings
RUSTFLAGS: "-C debuginfo=0 -C opt-level=0 -C incremental=false -C codegen-units=256"

@JefffreyJefffreyJan 3, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

https://github.com/apache/arrow-datafusion/blob/1179a76567892b259c88f08243ee01f05c4c3d5c/.github/actions/setup-builder/action.yaml#L45-L50

I think some of these flags are already enabled, technically? Just for Linux though, not Mac/Windows

(Well debuginfo was set to 0, not sure on incremental)

@compheadcompheadJan 3, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Thats good point, I would like to have configuration in a single place which is rust.yml, I'll create a followup to remove CARGO_INCREMENTAL and backtraces and other rust conf from action.yaml

RUST_BACKTRACE: "1"
# avoid rust stack overflows on tpc-ds tests
RUST_MINSTACK: "3000000"
- name: Verify Working Directory Clean
run: git diff --exit-code

Expand DownExpand Up@@ -290,6 +298,7 @@ jobs:
# with a OS-dependent path.
- name: Setup Rust toolchain
run: |
rustup update stable

@JefffreyJefffreyJan 3, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is the update here strictly necessary? Or should already be covered as part of the toolchain install below?

@compheadcompheadJan 3, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

i'll check it, maybe its not needed.

rustup toolchain install stable
rustup default stable
rustup component add rustfmt
Expand All@@ -302,9 +311,13 @@ jobs:
cargo test --lib --tests --bins --all-features
env:
# do not produce debug symbols to keep memory usage down
RUSTFLAGS: "-C debuginfo=0"
# use higher optimization level to overcome Windows rust slowness for tpc-ds
# and speed builds: https://github.com/apache/arrow-datafusion/issues/8696
# Cargo profile docs https://doc.rust-lang.org/cargo/reference/profiles.html?profile-settings#profile-settings
RUSTFLAGS: "-C debuginfo=0 -C opt-level=1 -C target-feature=+crt-static -C incremental=false -C codegen-units=256"
Comment thread
comphead marked this conversation as resolved.
RUST_BACKTRACE: "1"

# avoid rust stack overflows on tpc-ds tests
RUST_MINSTACK: "3000000"
macos:
name: cargo test (mac)
runs-on: macos-latest
Expand All@@ -327,6 +340,7 @@ jobs:
# with a OS-dependent path.
- name: Setup Rust toolchain
run: |
rustup update stable
rustup toolchain install stable
rustup default stable
rustup component add rustfmt
Expand All@@ -338,8 +352,12 @@ jobs:
cargo test --lib --tests --bins --all-features
env:
# do not produce debug symbols to keep memory usage down
RUSTFLAGS: "-C debuginfo=0"
# hardcoding other profile params to avoid profile override values
# More on Cargo profiles https://doc.rust-lang.org/cargo/reference/profiles.html?profile-settings#profile-settings
RUSTFLAGS: "-C debuginfo=0 -C opt-level=0 -C incremental=false -C codegen-units=256"
RUST_BACKTRACE: "1"
# avoid rust stack overflows on tpc-ds tests
RUST_MINSTACK: "3000000"
Comment on lines +359 to +360

@JefffreyJefffreyJan 3, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This feels like something we should fix in tpc-ds tests, instead of accounting for it after the fact (I know I've had some trouble running those tests locally due to stack issue, so could happen to others)

@compheadcompheadJan 3, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

that is true, during testing I saw spontaneous stack overflow issue on physical q54 TPC-DS test, that was the reason to bump the stack size. But I agree its good to create a follow up ticket and figure out why q54 is not the same as others, maybe the bigger problem is behind it.


test-datafusion-pyarrow:
name: cargo test pyarrow (amd64)
Expand Down