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.

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

Merged
merged 4 commits into from
Feb 21, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
71 changes: 41 additions & 30 deletions .github/workflows/extended.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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/[email protected]
Copy link
Contributor

Choose a reason for hiding this comment

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

This may possibly be a concern - the apache org limits what 3rd party actions are allowed though I am unsure if there is a list of them. I know I've encountered actions that were not allowed in the past. If it is a blocker you may have to request an exemption from the apache devops people. I think they also may want to use a git hash vs a version # to be sure it doesn't change underneath us.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. I changed the version number to hash.
Regarding whether this action is in the approved list, this action is able to run in my cloned branch, so I guess it's allowed.

- 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:
Expand All @@ -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
Copy link
Contributor

@ozankabak ozankabak Feb 19, 2025

Choose a reason for hiding this comment

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

Why will we not run extended tests with hash collisions? We may lose some coverage here. If the only problematic thing is the memory-limited tests, we should place it under a separate flag and only skip those tests.

What I wrote above seems wrong - I did a search and the extended_tests seems to be only gating these memory tests. If that is the case, maybe the name of the flag is not that great -- it led me to think we are disabling a bunch of other tests in addition to these ones.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree the fact that the flag extended_tests and the workflow is named extended is quite confusing

Maybe as a follow on PR we can rename the extended_test flag somthing different like extra_tests or extended_suite 🤔

cargo clean

sqllogictest-sqlite:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 recompilation, and the test binary
/// size might grow indifinitely. This initilizer 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() {
Copy link
Contributor

Choose a reason for hiding this comment

The 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

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 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.
This init_once is to solve the issue that different inner test commands can trigger multiple recompilation, I think there is no existing mechanism to coordinate parallel compilation, so this is just a lock to coordinate. Once the first inner test is compiled, other inner tests can use the same test binary without recompilation.

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
Expand Down
Loading