[native] Convert CircleCI jobs for PrestoC++ to Github actions #24236

czentgr · 2024-12-10T16:51:03Z

This pull request converts the CircleCI workflows to GitHub actions workflows.
The workflows use the standard runners with Ubuntu 22.04.

Some modifications were made to allow a build with the limited resources available:

name	build type	comments	expected runtime (w/out cache)	unit tests	E2E tests
prestocpp-linux-build	debug	All adapters enabled	45m	N	N
prestocpp-linux-build-and-unit-test	release	JWT, remote function, Parquet enabled	90m	Y	Y
prestocpp-macos-build	debug	Setup script plus build	up to 160m	N	N
prestocpp-linux-adapters-build	release	Adapters only build	5m	N	N

Caching is currently enabled for the prestocpp-linux-build-and-unit-test. If the cache can be used, the time to run tests drops to 50m.

Additional workflows are prestocpp-format-check and prestocpp-header-check
That run in a few seconds.

For the original conversion see: #21545

Resolves: #24023

Test runs can be seen in the Actions tab of the prestodb/presto repo.

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== NO RELEASE NOTE ==

czentgr · 2024-12-12T22:51:26Z

The Spark E2E pipeline is currently failing.
It turns out that CircleCI was not running all tests. It only ran a subset of tests and this exposes test issues in the Github workflow run.

The CircleCI test classes run

TESTCLASSES=TestPrestoSparkExpressionCompiler,TestPrestoSparkNativeJoinQueries,TestPrestoSparkSqlFunctions

The Github workflow test classes run:

TESTCLASSES = TestPrestoSparkExpressionCompiler,TestPrestoSparkNativeBitwiseFunctionQueries,TestPrestoSparkNativeAggregations,TestPrestoSparkNativeTpchConnectorQueries,TestPrestoSparkNativeSimpleQueries,TestPrestoSparkSqlFunctions,TestPrestoSparkNativeJoinQueries,TestPrestoSparkNativeWindowQueries,TestPrestoSparkNativeTpchQueries,TestPrestoSparkNativeArrayFunctionQueries,TestPrestoSparkNativeGeneralQueries

Issues created:
#24253
#24254
#24255

czentgr · 2024-12-12T22:57:06Z

@amitkdutta @tdcmeehan Here is the PR with the new github action workflows running on standard runners. The jobs are running as part of this PR.

There are Spark test failures. In CircleCI not all tests are apparently run and so we are running new tests that come back with errors and need fixing. I've created issues for them.

There are certainly options for more optimization within the workflows but this should be able to tide us over.

majetideepak

@czentgr thanks for pulling this off quickly and timely. Some comments.

.github/workflows/prestocpp-header-check.yml

majetideepak · 2024-12-12T23:35:07Z

.github/workflows/prestocpp-linux-adapters-build.yml

+        run: |
+          cd presto-native-execution
+          make submodules
+      - name: Build all adapter dependencies


We can remove this job. I am adding the adapters to the dependency image.
#24251

This job is here so we can check if changes in scripts trigger a failure with the adapters. The triggers is only changes in the script dir.
I can change it to specifically only trigger on changes to setup-adapter.sh. We don't want to find out issues when building the image after having changed the setup-adapters. I think it is useful to have. It won't run very often.

Actually, lets make it to run when we update velox or setup-adapters.sh.
The job is one of the quicker ones. That way we don't get nasty surprises.

But we don't need to install /velox/scripts/setup-adapters.sh since that is tested on the Velox CI.

We also don't need this on a Velox update then.

I was thinking if we could have a situation that isn't tested in Velox but might be used in PrestoC++. But I guess we'd need a full adapters build and PrestoC++ build. Which was what one CircleCI job originally did.

.github/workflows/prestocpp-linux-build-and-unit-test.yml

.github/workflows/prestocpp-macos-build.yml

presto-native-execution/scripts/setup-centos.sh

.github/workflows/prestocpp-linux-build.yml

majetideepak · 2024-12-13T16:01:11Z

@czentgr can we remove the CircleCI files in this PR as well?

majetideepak · 2024-12-13T16:07:05Z

I wonder why the Presto e2e tests are taking much longer in the github workflow (~39 minutes ) compared to the CI. (~5 minutes)

czentgr

As for the E2E Presto test difference. I think it is the same as for Spark. I'm adding a comment about that in the PR. It is clear we are running many more tests based on the summary.

czentgr · 2024-12-13T16:19:16Z

.github/workflows/prestocpp-linux-adapters-build.yml

+        run: |
+          cd presto-native-execution
+          make submodules
+      - name: Build all adapter dependencies


I was thinking if we could have a situation that isn't tested in Velox but might be used in PrestoC++. But I guess we'd need a full adapters build and PrestoC++ build. Which was what one CircleCI job originally did.

.github/workflows/prestocpp-linux-build.yml

czentgr · 2024-12-13T16:38:48Z

I wonder why the Presto e2e tests are taking much longer in the github workflow (~39 minutes ) compared to the CI. (~5 minutes)

In an earlier comment we looked at the Spark failures and realized that CircleCI isn't actually running all the test classes that it could/that are found when simply looking at the names.
The same applies to Presto E2E tests. In CircleCI we can see only 133 tests are run
from https://app.circleci.com/pipelines/github/prestodb/presto/23370/workflows/96edda2e-37ec-410f-ac23-720b11722347/jobs/97390

[INFO] Tests run: 133, Failures: 0, Errors: 0, Skipped: 0

while the Github runners run every test:
from https://github.com/prestodb/presto/actions/runs/12307010027/job/34350125338?pr=24236

[INFO] Tests run: 594, Failures: 0, Errors: 0, Skipped: 0

For example, we can see that TestPrestoNativeTpchConnectorQueries is not run in CircleCI. This applies to a number of test classes.

czentgr · 2024-12-13T16:39:51Z

@czentgr can we remove the CircleCI files in this PR as well?

After discussion we decided to remove them separately after a few days of having both pipelines run.

This pull request converts the CircleCI workflows to GitHub actions workflows. The workflows use the standard runners with Ubuntu 22.04. Some modifications were made to allow a build with the limited resources available: |name|build type|comments|expected runtime (no cache)|unit tests|E2E tests| |----|----------|--------|----------------|----------|---------| | prestocpp-linux-build | debug | All adapters enabled | 45m | N | N | | prestocpp-linux-build-and-unit-test | release | JWT, remote function, Parquet enabled | 90m | Y | Y | | prestocpp-macos-build | debug | Setup script plus build | 160m | N | N | | prestocpp-linux-adapters-build | release | Adapters and release build | 120m | N | N | Caching is currently enabled for the prestocpp-linux-build-and-unit-test. If the cache can be used, the time to run tests drops to 50m. Additional workflows are prestocpp-format-check and prestocpp-header-check That run in a few seconds. For the original conversion see: prestodb#21545 Co-authored-by: Rob Anderson <[email protected]>

majetideepak

Thanks, @czentgr

majetideepak · 2024-12-13T21:57:44Z

@tdcmeehan can you please help merge this PR? Thanks.

prestodb-ci added the from:IBM PR from IBM label Dec 10, 2024

prestodb-ci requested review from a team, zuyu and BryanCutler and removed request for a team December 10, 2024 16:51

czentgr force-pushed the cz_test_standard_runner_build branch 25 times, most recently from f9ddbda to 68743bf Compare December 12, 2024 16:39

czentgr force-pushed the cz_test_standard_runner_build branch from ba2a256 to edeb87f Compare December 12, 2024 22:47

czentgr changed the title ~~[native] test standard runner prestocpp build~~ [native] Convert CircleCI jobs for PrestoC++ to Github actions Dec 12, 2024

czentgr force-pushed the cz_test_standard_runner_build branch from edeb87f to f662fea Compare December 12, 2024 22:49

czentgr marked this pull request as ready for review December 12, 2024 22:51

czentgr requested review from a team as code owners December 12, 2024 22:51

czentgr requested review from presto-oss, tdcmeehan and amitkdutta December 12, 2024 22:51

czentgr force-pushed the cz_test_standard_runner_build branch from f662fea to 6f71f9c Compare December 12, 2024 23:02

majetideepak reviewed Dec 12, 2024

View reviewed changes

czentgr force-pushed the cz_test_standard_runner_build branch 3 times, most recently from baa1163 to 6162673 Compare December 13, 2024 00:10

majetideepak reviewed Dec 13, 2024

View reviewed changes

.github/workflows/prestocpp-linux-build.yml Outdated Show resolved Hide resolved

czentgr commented Dec 13, 2024

View reviewed changes

czentgr force-pushed the cz_test_standard_runner_build branch 2 times, most recently from 7f08dbd to 0562187 Compare December 13, 2024 20:26

czentgr force-pushed the cz_test_standard_runner_build branch from 0562187 to 35816fd Compare December 13, 2024 21:32

majetideepak approved these changes Dec 13, 2024

View reviewed changes

tdcmeehan approved these changes Dec 13, 2024

View reviewed changes

tdcmeehan merged commit 17ac4c3 into prestodb:master Dec 14, 2024
63 of 64 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[native] Convert CircleCI jobs for PrestoC++ to Github actions #24236

[native] Convert CircleCI jobs for PrestoC++ to Github actions #24236

czentgr commented Dec 10, 2024 •

edited

Loading

czentgr commented Dec 12, 2024

czentgr commented Dec 12, 2024

majetideepak left a comment

majetideepak Dec 12, 2024

czentgr Dec 12, 2024

czentgr Dec 12, 2024 •

edited

Loading

majetideepak Dec 13, 2024

majetideepak Dec 13, 2024

czentgr Dec 13, 2024

majetideepak commented Dec 13, 2024 •

edited

Loading

majetideepak commented Dec 13, 2024

czentgr left a comment

czentgr Dec 13, 2024

czentgr commented Dec 13, 2024 •

edited

Loading

czentgr commented Dec 13, 2024 •

edited

Loading

majetideepak left a comment

majetideepak commented Dec 13, 2024

[native] Convert CircleCI jobs for PrestoC++ to Github actions #24236

[native] Convert CircleCI jobs for PrestoC++ to Github actions #24236

Conversation

czentgr commented Dec 10, 2024 • edited Loading

Resolves: #24023

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

czentgr commented Dec 12, 2024

czentgr commented Dec 12, 2024

majetideepak left a comment

Choose a reason for hiding this comment

majetideepak Dec 12, 2024

Choose a reason for hiding this comment

czentgr Dec 12, 2024

Choose a reason for hiding this comment

czentgr Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

majetideepak Dec 13, 2024

Choose a reason for hiding this comment

majetideepak Dec 13, 2024

Choose a reason for hiding this comment

czentgr Dec 13, 2024

Choose a reason for hiding this comment

majetideepak commented Dec 13, 2024 • edited Loading

majetideepak commented Dec 13, 2024

czentgr left a comment

Choose a reason for hiding this comment

czentgr Dec 13, 2024

Choose a reason for hiding this comment

czentgr commented Dec 13, 2024 • edited Loading

czentgr commented Dec 13, 2024 • edited Loading

majetideepak left a comment

Choose a reason for hiding this comment

majetideepak commented Dec 13, 2024

czentgr commented Dec 10, 2024 •

edited

Loading

czentgr Dec 12, 2024 •

edited

Loading

majetideepak commented Dec 13, 2024 •

edited

Loading

czentgr commented Dec 13, 2024 •

edited

Loading

czentgr commented Dec 13, 2024 •

edited

Loading