-
Notifications
You must be signed in to change notification settings - Fork 155
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
build: Separate and speedup TPC-DS benchmark #130
Conversation
This might be reviewed and merged after #109 is merged. I tested on my local repo, it now take ~36min to finish TPC-DS benchmarks. |
with: | ||
rust-version: ${{env.RUST_VERSION}} | ||
jdk-version: 11 | ||
- name: Cache Maven dependencies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reuse Maven cache.
repository: databricks/tpcds-kit | ||
path: ./tpcds-kit | ||
- name: Build Comet | ||
run: make release |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's quite slow to build under release profile. We may find a way to speed up rust build.
But it's sufficient for now, we can do that later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is mostly because we uses lto = "thin"
.
- name: Run TPC-DS queries (Sort merge join) | ||
if: matrix.join == 'sort_merge' | ||
run: | | ||
SPARK_HOME=`pwd` SPARK_TPCDS_DATA=`pwd`/tpcds-sf-1 ./mvnw -B -Prelease -Dsuites=org.apache.spark.sql.CometTPCDSQuerySuite test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-Prelease
to include the release build lib.
And I noticed that the java tests are also executed but they are executed quickly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Merged, thanks! |
# specific language governing permissions and limitations | ||
# under the License. | ||
|
||
name: Run TPC-DS Benchmark |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not actually a benchmark, but correctness check (with Spark result).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, let me submit a follow up PR to fix it then..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about this too when reviewing this PR, but decided it is fine since we could re-purpose this workflow in future for both benchmarking and correctness check. It's fine for me if we decide to change it or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay
/root/.m2/repository | ||
key: ${{ runner.os }}-java-maven-${{ hashFiles('**/pom.xml') }} | ||
restore-keys: | | ||
${{ runner.os }}-java-maven- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder why we have a -
postfix there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The key is ${{ runner.os }}-java-maven-${{ hashFiles('**/pom.xml') }}
, so the restored keys should be a prefix of that key.
See cache action's restore-keys(https://github.com/actions/cache) for explaination:
restore-keys - An ordered list of prefix-matched keys to use for restoring stale cache if no cache hit occurred for key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The -
could be removed, but it would be natural to include that since it's in the prefix of the key
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder why we use this prefix restore key? If hashFiles
is different, I think we don't want to restore it, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If hashFiles is different, I think we don't want to restore it, right?
For maven dependencies, even if hashFiles
is different, we may want to restore it as the previous cache should contains most of needed dependency. mvn will download missing jars automatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
path: | | ||
~/.m2/repository | ||
/root/.m2/repository | ||
key: ${{ runner.os }}-java-maven-${{ hashFiles('**/pom.xml') }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does Java version affect this maven cache?
We have os in key but seems Java version isn't there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it doesn't matter that much as most jars are java version agnostic?
And this cache will share the PR build's maven dependency..
Another thing to add: github's cache has quota limit: 10GB. The cached maven dependencies are already quite big(~1GB). If we add the java version to the key, it may create too much maven cache dependencies.
- name: Restore TPC-DS generated data | ||
id: cache-tpcds-sf-1 | ||
uses: actions/cache@v4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just restore it using restore action?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just restore it using restore action?
Could you point me the link to the restore action? I searched github but didn't find one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thanks. Will fix it.
Which issue does this PR close?
Partially resolves #67
Rationale for this change
Reduce CI runtime and improve developer efficiency
What changes are included in this PR?
How are these changes tested?
Verified with CI runs