-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Fix CI fail for extended test (by freeing up more disk space in CI runner) #14745
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,43 +39,54 @@ jobs: | |
linux-build-lib: | ||
name: linux build test | ||
runs-on: ubuntu-latest | ||
container: | ||
image: amd64/rust | ||
steps: | ||
- uses: actions/checkout@v4 | ||
- name: Setup Rust toolchain | ||
uses: ./.github/actions/setup-builder | ||
with: | ||
rust-version: stable | ||
submodules: true | ||
fetch-depth: 1 | ||
- name: Install Rust | ||
run: | | ||
curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y | ||
source $HOME/.cargo/env | ||
rustup default stable | ||
- name: Install Protobuf Compiler | ||
run: sudo apt-get install -y protobuf-compiler | ||
- name: Prepare cargo build | ||
run: | | ||
cargo check --profile ci --all-targets | ||
cargo clean | ||
|
||
# # Run extended tests (with feature 'extended_tests') | ||
# # Disabling as it is running out of disk space | ||
# # see https://github.com/apache/datafusion/issues/14576 | ||
# linux-test-extended: | ||
# name: cargo test 'extended_tests' (amd64) | ||
# needs: linux-build-lib | ||
# runs-on: ubuntu-latest | ||
# container: | ||
# image: amd64/rust | ||
# steps: | ||
# - uses: actions/checkout@v4 | ||
# with: | ||
# submodules: true | ||
# fetch-depth: 1 | ||
# - name: Setup Rust toolchain | ||
# uses: ./.github/actions/setup-builder | ||
# with: | ||
# rust-version: stable | ||
# - name: Run tests (excluding doctests) | ||
# run: cargo test --profile ci --exclude datafusion-examples --exclude datafusion-benchmarks --workspace --lib --tests --bins --features avro,json,backtrace,extended_tests | ||
# - name: Verify Working Directory Clean | ||
# run: git diff --exit-code | ||
# - name: Cleanup | ||
# run: cargo clean | ||
# Run extended tests (with feature 'extended_tests') | ||
linux-test-extended: | ||
name: cargo test 'extended_tests' (amd64) | ||
needs: linux-build-lib | ||
runs-on: ubuntu-latest | ||
steps: | ||
- uses: actions/checkout@v4 | ||
with: | ||
submodules: true | ||
fetch-depth: 1 | ||
- name: Free Disk Space (Ubuntu) | ||
uses: jlumbroso/free-disk-space@54081f138730dfa15788a46383842cd2f914a1be | ||
- name: Install Rust | ||
run: | | ||
curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y | ||
source $HOME/.cargo/env | ||
rustup default stable | ||
- name: Install Protobuf Compiler | ||
run: sudo apt-get install -y protobuf-compiler | ||
# For debugging, test binaries can be large. | ||
- name: Show available disk space | ||
run: | | ||
df -h | ||
- name: Run tests (excluding doctests) | ||
env: | ||
RUST_BACKTRACE: 1 | ||
run: cargo test --profile ci --exclude datafusion-examples --exclude datafusion-benchmarks --workspace --lib --tests --bins --features avro,json,backtrace,extended_tests | ||
- name: Verify Working Directory Clean | ||
run: git diff --exit-code | ||
- name: Cleanup | ||
run: cargo clean | ||
|
||
# Check answers are correct when hash values collide | ||
hash-collisions: | ||
|
@@ -95,7 +106,7 @@ jobs: | |
- name: Run tests | ||
run: | | ||
cd datafusion | ||
cargo test --profile ci --exclude datafusion-examples --exclude datafusion-benchmarks --exclude datafusion-sqllogictest --workspace --lib --tests --features=force_hash_collisions,avro,extended_tests | ||
cargo test --profile ci --exclude datafusion-examples --exclude datafusion-benchmarks --exclude datafusion-sqllogictest --workspace --lib --tests --features=force_hash_collisions,avro | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
What I wrote above seems wrong - I did a search and the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree the fact that the flag Maybe as a follow on PR we can rename the |
||
cargo clean | ||
|
||
sqllogictest-sqlite: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,12 +21,14 @@ | |
//! This file is organized as: | ||
//! - Test runners that spawn individual test processes | ||
//! - Test cases that contain the actual validation logic | ||
use std::{process::Command, str}; | ||
|
||
use log::info; | ||
use std::sync::Once; | ||
use std::{process::Command, str}; | ||
|
||
use crate::memory_limit::memory_limit_validation::utils; | ||
|
||
static INIT: Once = Once::new(); | ||
|
||
// =========================================================================== | ||
// Test runners: | ||
// Runners are splitted into multiple tests to run in parallel | ||
|
@@ -67,10 +69,35 @@ fn sort_with_mem_limit_2_cols_2_runner() { | |
spawn_test_process("sort_with_mem_limit_2_cols_2"); | ||
} | ||
|
||
/// `spawn_test_process` might trigger multiple recompilations and the test binary | ||
/// size might grow indefinitely. This initializer ensures recompilation is only done | ||
/// once and the target size is bounded. | ||
/// | ||
/// TODO: This is a hack, can be cleaned up if we have a better way to let multiple | ||
/// test cases run in different processes (instead of different threads by default) | ||
fn init_once() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't undersrtand how this avoids recompilation It seems like recompilation would happen if the options / features were different There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe there are some subtle differences between the outer/inner test command to cause the recompilation, I tried to match the option and features but still can't work. |
||
INIT.call_once(|| { | ||
let _ = Command::new("cargo") | ||
.arg("test") | ||
.arg("--no-run") | ||
.arg("--package") | ||
.arg("datafusion") | ||
.arg("--test") | ||
.arg("core_integration") | ||
.arg("--features") | ||
.arg("extended_tests") | ||
.env("DATAFUSION_TEST_MEM_LIMIT_VALIDATION", "1") | ||
.output() | ||
.expect("Failed to execute test command"); | ||
}); | ||
} | ||
|
||
/// Helper function that executes a test in a separate process with the required environment | ||
/// variable set. Memory limit validation tasks need to measure memory resident set | ||
/// size (RSS), so they must run in a separate process. | ||
fn spawn_test_process(test: &str) { | ||
init_once(); | ||
|
||
let test_path = format!( | ||
"memory_limit::memory_limit_validation::sort_mem_validation::{}", | ||
test | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a little worried about using a third-party action
It seems from the soure we could put a few shell command and get the same effect:
https://github.com/jlumbroso/free-disk-space/blob/54081f138730dfa15788a46383842cd2f914a1be/action.yml#L149
https://github.com/jlumbroso/free-disk-space/blob/54081f138730dfa15788a46383842cd2f914a1be/action.yml#L161-L162
https://github.com/jlumbroso/free-disk-space/blob/54081f138730dfa15788a46383842cd2f914a1be/action.yml#L175-L185
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the idea is to review the third-party script, and extract safe deletions to an in-house script. Filed #14803 to address this idea.