improve rust workflows without cache #1275
Conversation
Huy1Ng
force-pushed
the
improve-github-actions/rust-workflows
branch
from
d848873 to
f352906
Compare
|
@milenkovicm can you rerun the CI? I pushed 2 commits too close together so some jobs failed because they didn't get an available machine. The CI run on my fork succeeded: https://github.com/Huy1Ng/datafusion-ballista/tree/improve-github-actions/rust-workflows |
|
Have no access to computer at the moment. Can you please push change, it should trigger new job Two questions
|
|
|
You don't have to if you think this is better solution. I don't know much about this topic. What do you think ? I guess we need to publish docker, cancel job is your call |
Huy1Ng
force-pushed
the
improve-github-actions/rust-workflows
branch
from
0689d70 to
54080a7
Compare
milenkovicm
left a comment
milenkovicm
left a comment
There was a problem hiding this comment.
To best of my knowledge this make sense, few minor comments
| shell: bash | ||
| run: | | ||
| echo "Installing ${{ inputs.rust-version }}" | ||
| rustup toolchain install ${{ inputs.rust-version }} |
There was a problem hiding this comment.
I wonder should we use GitHub action such as dtolnay/rust-toolchain@stable to setup rather than use bash script for it?
There was a problem hiding this comment.
I tried but the current repo settings doesn't allow dtolnay/rust-toolchain@stable. datafusion repo seems to have the same limitations: apache/datafusion#15315.
P/s: my bad. I was using different toolchain. Still, I think we should mirror datafusion CI workflows as much as possible (offload the thinking and decision to the bigger community).
|
|
||
| concurrency: | ||
| group: ${{ github.repository }}-${{ github.workflow }} | ||
| cancel-in-progress: true |
| path: | | ||
| /github/home/target/release/ballista-scheduler | ||
| /github/home/target/release/ballista-executor | ||
| # Adding `--locked` here to assert that the `Cargo.lock` file is up to |
| path: /github/home/.cargo | ||
| # this key equals the ones on `linux-build-lib` for re-use | ||
| key: cargo-cache- | ||
| - name: Cache Rust dependencies |
There was a problem hiding this comment.
I'm fine to remove cache is it helps, there are another two locations where cache is used cargo-toml-formatting-checks and Clippy. Would it make sense to remove it there as well or we keep it?
There was a problem hiding this comment.
I will remove them. Those are fast to run so cache wouldn't do much anyway.
There was a problem hiding this comment.
I also wonder if benchmark test is where we should leave the caching. This is the bottleneck of rust workflows, so any optimization would help.
|
this should be ready for rust. I will tackle Python workflow next. This is going to be spitted into 3 small PRs. |
|
Thanks @Huy1Ng! As python binding has its problems I wonder if we should keep just basic python CI active |
|
thanks againn @Huy1Ng |
2 participants
Which issue does this PR close?
Part of #1128.
Rationale for this change
In the issue
What changes are included in this PR?
Improve the rust workflows by following the practices in parent datafusion repo (with couple inspiration from popular rust project like uv or polars). Changes are:
Are there any user-facing changes?
No