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

Change rust.yml to run benchmark #7708

Merged
merged 2 commits into from
Oct 1, 2023
Merged

Change rust.yml to run benchmark #7708

merged 2 commits into from
Oct 1, 2023

Conversation

sarutak
Copy link
Member

@sarutak sarutak commented Oct 1, 2023

Which issue does this PR close?

Closes #7707

Rationale for this change

Run benchmarks as intended.

What changes are included in this PR?

This PR changes rust.yml to run benchmark.
This PR also changes the cargo command running benchmark to narrow the package to be built, which can reduce the build time.

Are these changes tested?

Done by CI.

Are there any user-facing changes?

No.

@sarutak
Copy link
Member Author

sarutak commented Oct 1, 2023

Comment on lines 229 to 239
- name: Cache Cargo
uses: actions/cache@v3
with:
path: |
~/.cargo/bin/
~/.cargo/registry/index/
~/.cargo/registry/cache/
~/.cargo/git/db/
./target/
# this key equals the ones on `linux-build-lib` for re-use
key: cargo-cache-benchmark-${{ hashFiles('datafusion/**/Cargo.toml', 'benchmarks/Cargo.toml') }}
Copy link
Member

Choose a reason for hiding this comment

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

Why remove this? Related to the issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, this change is mixed.

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.

👏

Screenshot 2023-10-01 at 6 34 03 AM

Very nice -- thank you @sarutak

Also, I would like to thank you again for your efforts to make the DataFusion CI work better and more efficiently. It is most appreciated.

I actually spent some more time looking into what plan_q does and it is not at all clear to me it needs to be built in release mode either

https://github.com/apache/arrow-datafusion/blob/46cdb8c2dc495e8063a0adc5c3f9ac82b136e72e/benchmarks/src/tpch/run.rs#L297-L453

It seems like it just verifies the plans (rather than actually running them). I'll see what happens if I change to using debug (rather than release) mode which compiles faster.

@alamb alamb merged commit 14cdf72 into apache:main Oct 1, 2023
22 checks passed
Ted-Jiang pushed a commit to Ted-Jiang/arrow-datafusion that referenced this pull request Oct 7, 2023
* Change rust.yml to run benchmark

* Restore unrelated change
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.

Benchmark CI job doesn't actually run benchmark
3 participants