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

Cleanup CI (#5040) #5047

Merged
merged 3 commits into from
Jan 24, 2023
Merged

Cleanup CI (#5040) #5047

merged 3 commits into from
Jan 24, 2023

Conversation

tustvold
Copy link
Contributor

@tustvold tustvold commented Jan 24, 2023

Which issue does this PR close?

Closes #5040

Rationale for this change

  • Splits out doctests into separate CI job (as they result in a lot of generated code)
  • Splits out examples into separate CI job (as they take a while to compile and run)
  • Simplifies the hash collision and pythin CI jobs
  • Replaces cancel workflow with concurrency groups
  • Miscellaneous cleanups

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@@ -32,10 +36,6 @@ jobs:
runs-on: ubuntu-latest
container:
image: amd64/rust
env:
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 has been moved into setup-builder

@@ -92,17 +87,10 @@ jobs:
uses: ./.github/actions/setup-builder
with:
rust-version: stable
- name: Build tests
run: |
export PATH=$PATH:$HOME/d/protoc/bin
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 shouldn't be necessary

@tustvold tustvold force-pushed the ci-cleanup branch 2 times, most recently from bc8bffc to dc0b00a Compare January 24, 2023 17:34
@@ -478,19 +485,14 @@ jobs:
- name: Run tests
run: |
cd datafusion
# Force all hash values to collide
cargo test --all --features=force_hash_collisions
cargo test --lib --tests --features=force_hash_collisions
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 couldn't see a compelling reason to also run the doctests here

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed -- no need to run doc tests

cd datafusion
cargo test --features=pyarrow
- name: Run datafusion-common tests
run: cargo test -p datafusion-common --features=pyarrow
Copy link
Contributor Author

@tustvold tustvold Jan 24, 2023

Choose a reason for hiding this comment

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

The only pyarrow code is in datafusion-common AFAICT

See #5048

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @tustvold -- this looks great

I also manually looked at the output of the test CI jobs and ensured all the tests ran the way I expected.

run: |
echo "Installing ${{ inputs.rust-version }}"
rustup toolchain install ${{ inputs.rust-version }}
rustup default ${{ inputs.rust-version }}
rustup component add rustfmt
- name: Disable debuginfo generation
# Disable full debug symbol generation to speed up CI build and keep memory down
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -18,6 +18,10 @@
name: Dev
on: [push, pull_request]

concurrency:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think adding rationale as comments will help avoid someone deleting this by mistake as part of some future "cleanup"

Suggested change
concurrency:
# Ensure when changes are pushed to a PR previously running jobs are canceled
concurrency:

@@ -17,6 +17,10 @@

name: Labeler

concurrency:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
concurrency:
# Ensure when changes are pushed to a PR previously running jobs are canceled
concurrency:

@@ -17,6 +17,10 @@

name: Rust

concurrency:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
concurrency:
# Ensure when changes are pushed to a PR previously running jobs are canceled
concurrency:

cd datafusion
cargo test --features=pyarrow
- name: Run datafusion-common tests
run: cargo test -p datafusion-common --features=pyarrow
Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -478,19 +485,14 @@ jobs:
- name: Run tests
run: |
cd datafusion
# Force all hash values to collide
cargo test --all --features=force_hash_collisions
cargo test --lib --tests --features=force_hash_collisions
Copy link
Contributor

Choose a reason for hiding this comment

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

agreed -- no need to run doc tests

@alamb
Copy link
Contributor

alamb commented Jan 24, 2023

I am going to merge this in so we can unblock other PRs -- we can handle comments / issues as follow on.

Thanks again @tustvold

@ursabot
Copy link

ursabot commented Jan 24, 2023

Benchmark runs are scheduled for baseline = ab00bc1 and contender = 0820eb9. 0820eb9 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI Failing with Out of Disk
3 participants