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

ci improvements, update protoc #13876

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

Omega359
Copy link
Contributor

@Omega359 Omega359 commented Dec 21, 2024

Which issue does this PR close?

Closes #13846, part of #13845

Rationale for this change

Improve ci run time

What changes are included in this PR?

  • disable intel mac job
  • switch to nextest where possible
  • improved cached
  • checkout with depth=1
  • ci cargo profile that disables debug, etc in dependencies

Are these changes tested?

yes

Are there any user-facing changes?

No

@github-actions github-actions bot added the development-process Related to development process of DataFusion label Dec 21, 2024
@Omega359 Omega359 marked this pull request as ready for review December 21, 2024 20:01
@comphead
Copy link
Contributor

comphead commented Dec 21, 2024

Thanks @Omega359 is it a draft? I dont see tests in checks

@Omega359
Copy link
Contributor Author

Thanks @Omega359 is it a draft? I dont see tests in checks

No, but it should be. I can't seem to make it draft but apparently taiki-e/install-action@nextest is not an allowed action in apache repo's.

@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Dec 22, 2024
@Omega359
Copy link
Contributor Author

I've reverted the nextest changes for now as the github action to install the binary of it is not approved and building nextest from source would take longer than the time it saves when running tests.

Also noted that some of the non-apache/github actions used in datafusion's ci use versions vs githash of a validated version which is against the apache github action policy.

For example: mozilla-actions/[email protected]

We likely should file another ticket to see change all third-party actions to be githash vs version

@Omega359
Copy link
Contributor Author

@alamb
Copy link
Contributor

alamb commented Dec 23, 2024

I can not wait to test / review this but I think I will run out of time today -- hopefully either later today or tomorrow

@Omega359
Copy link
Contributor Author

Thanks for the update - no rush on my side

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.

Thanks @Omega359 -- this looks quite cool

I am a little surprised about the debug and incremental changes as in theory we already did them, but if it wasn't working is is great that this PR improves the situation

@@ -34,5 +34,6 @@ runs:
echo "RUSTC_WRAPPER=sccache" >> $GITHUB_ENV
echo "SCCACHE_GHA_ENABLED=true" >> $GITHUB_ENV
echo "RUST_BACKTRACE=1" >> $GITHUB_ENV
echo "CARGO_INCREMENTAL=false" >> $GITHUB_ENV
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like this may be redundant with the line below (or if the line below isn't working. perhaps we could remove it 🤔 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably. I wasn't sure at one point so I put it there to test.

@@ -288,17 +318,20 @@ jobs:
mv *.tbl ../datafusion/sqllogictest/test_files/tpch/data
- name: Verify that benchmark queries return expected results
run: |
# increase stack size to fix stack overflow
export RUST_MIN_STACK=20971520
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 we should have fixed this now with the recursive protection feature -- do we still need to set the min stack size?

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 know I needed it when I tried running this the other day. When was that fixed?

Copy link
Contributor

Choose a reason for hiding this comment

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

It was in #13310

.github/workflows/rust.yml Show resolved Hide resolved

# ci turns off debug info, etc for dependencies to allow for smaller binaries making caching more effective
[profile.ci.package."*"]
debug = false
Copy link
Contributor

Choose a reason for hiding this comment

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

We also found this setting particularly effective for speeding up our CI in influxdb_iox: https://doc.rust-lang.org/cargo/reference/profiles.html#debug

.github/actions/setup-builder/action.yaml Outdated Show resolved Hide resolved
@alamb alamb changed the title ci improvements ci improvements, update protoc Dec 26, 2024
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 @Omega359 -- I am so excited to get these tests running faster

@alamb
Copy link
Contributor

alamb commented Dec 26, 2024

Screenshot 2024-12-26 at 11 24 17 AM

@Omega359
Copy link
Contributor Author

I am going to continue working on the individual jobs - I think for many the only way to improve their performance is either to build with nightly, somehow make caching effective, run on something that is more powerful than my watch, or break them up into multiple jobs.

Currently working on getting the hash collision job to run only on merges to main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development-process Related to development process of DataFusion sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Contemplate stop CI testing on intel mac
3 participants