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

fix(build): Add remote function benchmark under benchmark flags #11905

Conversation

czentgr
Copy link
Collaborator

@czentgr czentgr commented Dec 18, 2024

The remote function benchmarks are built without checking if the benchmark utilities are being built. These are only built if certain flags are set.

This causes upstream issues in Prestissimo because there remote functions are being built but the benchmarks are not.

PR introducing the breaking change:
#11539

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 18, 2024
Copy link

netlify bot commented Dec 18, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 9c4d3b9
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/676314aacea35a0008bc80dc

@@ -16,4 +16,6 @@ add_subdirectory(if)
add_subdirectory(client)
add_subdirectory(server)
add_subdirectory(utils)
if(${VELOX_ENABLE_BENCHMARKS} OR ${VELOX_ENABLE_BENCHMARKS_BASIC})
Copy link
Collaborator

Choose a reason for hiding this comment

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

${VELOX_ENABLE_BENCHMARKS} should be sufficient.

@czentgr czentgr force-pushed the cz_fix_remote_func_benchmark_build branch from 45e1240 to 2cd7668 Compare December 18, 2024 18:19
@czentgr czentgr marked this pull request as ready for review December 18, 2024 18:19
@czentgr czentgr requested a review from assignUser as a code owner December 18, 2024 18:19
The remote function benchmarks are built without checking if the benchmark
utilities are being built. These are only built if certain flags are set.

This causes upstream issues in Prestissimo because there remote functions
are being built but the benchmarks are not.

PR introducing the breaking change:
facebookincubator#11539
@czentgr czentgr force-pushed the cz_fix_remote_func_benchmark_build branch from 2cd7668 to 9c4d3b9 Compare December 18, 2024 18:30
@majetideepak majetideepak added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Dec 18, 2024
Copy link
Contributor

@kgpai kgpai left a comment

Choose a reason for hiding this comment

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

Thanks !

@facebook-github-bot
Copy link
Contributor

@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@kgpai merged this pull request in b4d1ce0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants