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

[GLUTEN-6562][VL] Decouple BUILD_BENCHMARKS and BUILD_TESTS build options #6563

Merged
merged 5 commits into from
Jul 24, 2024

Conversation

NEUpanning
Copy link
Contributor

@NEUpanning NEUpanning commented Jul 23, 2024

What changes were proposed in this pull request?

Building Gluten tests depends on gluten benchmark , this pr decouples them.

Fixes: #6562

How was this patch tested?

I successfully built gluten tests with ./compile.sh --build_velox_backend=ON --build_tests=ON --build_examples=ON that BUILD_BENCHMARKS is not enabled

Copy link

Thanks for opening a pull request!

Could you open an issue for this pull request on Github Issues?

https://github.com/apache/incubator-gluten/issues

Then could you also rename commit message and pull request title in the following format?

[GLUTEN-${ISSUES_ID}][COMPONENT]feat/fix: ${detailed message}

See also:

@NEUpanning NEUpanning changed the title [VL]Set BUILD_BENCHMARKS=ON if BUILD_TESTS is enabled [GLUTEN-6562][VL]Set BUILD_BENCHMARKS=ON if BUILD_TESTS is enabled Jul 23, 2024
Copy link

#6562

@PHILO-HE
Copy link
Contributor

@xumingming, I note you also found this issue, could you take a review?

@xumingming
Copy link
Contributor

Is it possible to not mix BUILD_TESTS and BUILD_BENCHMARKS together? I think it is cleaner, what we need to do is do not let tests code depends on benchmark related code.

@PHILO-HE
Copy link
Contributor

Is it possible to not mix BUILD_TESTS and BUILD_BENCHMARKS together? I think it is cleaner, what we need to do is do not let tests code depends on benchmark related code.

@xumingming, I agree. @NEUpanning, we may need to extract out the shared code from benchmark module.

@NEUpanning
Copy link
Contributor Author

@PHILO-HE @xumingming I have updated the code so that the gluten tests no longer depends on the gluten benchmarks.

target_link_libraries(${TEST_EXEC} velox_benchmark_common GTest::gtest
GTest::gtest_main)
target_link_libraries(${TEST_EXEC} velox GTest::gtest GTest::gtest_main
google::glog benchmark::benchmark)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why benchmark::benchmark is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I've remove it.

@xumingming
Copy link
Contributor

Looks good to me.

@NEUpanning NEUpanning force-pushed the enable_build_benchmark branch from 7b90371 to 803b99f Compare July 24, 2024 06:24
Copy link
Contributor

@PHILO-HE PHILO-HE left a comment

Choose a reason for hiding this comment

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

Thanks!

@PHILO-HE PHILO-HE changed the title [GLUTEN-6562][VL]Set BUILD_BENCHMARKS=ON if BUILD_TESTS is enabled [GLUTEN-6562][VL] Decouple BUILD_BENCHMARKS and BUILD_TESTS build options Jul 24, 2024
@PHILO-HE PHILO-HE merged commit 8ab0b10 into apache:main Jul 24, 2024
43 checks passed
@NEUpanning NEUpanning deleted the enable_build_benchmark branch July 26, 2024 04:13
weiting-chen pushed a commit to weiting-chen/gluten that referenced this pull request Nov 12, 2024
PHILO-HE pushed a commit that referenced this pull request Nov 18, 2024
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.

Can't build gluten tests when BUILD_BENCHMARKS is not enabled
3 participants