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

[nfc] remove off-by-default bench tag #1081

Merged
merged 3 commits into from
Aug 30, 2023
Merged

[nfc] remove off-by-default bench tag #1081

merged 3 commits into from
Aug 30, 2023

Conversation

mikea
Copy link
Collaborator

@mikea mikea commented Aug 29, 2023

bazel doesn't build/run it otherwise. It doesn't hurt if there's none, since run target are not run by bazel test.

@mikea mikea requested a review from fhanau August 29, 2023 14:51
@fhanau
Copy link
Collaborator

fhanau commented Aug 29, 2023

If we remove the tag, this will be included in bazel build //..., which builds all targets (but does not run tests), causing all benchmarks to be built and run on CI as bazel build //... is the first CI build step and building the csv output file causes the benchmark to be run. This could be avoided by adding --build_tests_only, but that might also end up not building things that we want to be built (e.g. the api_encoder, which is not a dependency of any tests but we still want to be able to see if any ).
Running benchmarks on CI by default is something we can do, but it merits prior discussion.

Copy link
Collaborator

@fhanau fhanau left a comment

Choose a reason for hiding this comment

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

See comment above

@mikea
Copy link
Collaborator Author

mikea commented Aug 29, 2023

If we remove the tag, this will be included in bazel build //..., which builds all targets (but does not run tests), causing all benchmarks to be built and run on CI as bazel build //... is the first CI build step and building the csv output file causes the benchmark to be run. This could be avoided by adding --build_tests_only, but that might also end up not building things that we want to be built (e.g. the api_encoder, which is not a dependency of any tests but we still want to be able to see if any ). Running benchmarks on CI by default is something we can do, but it merits prior discussion.

hm... My thinking was that "off-by-default" on csv will still prevent us from generating csv on CI. If I take a look into build logs I don't see any evidence of us running benchmarks either.

building benchmarks during CI is actually a good thing, we don't wan't them to be stale.

@fhanau
Copy link
Collaborator

fhanau commented Aug 29, 2023

Ok, I see that "off-by-default" still applies for the benchmark csv. In that case we can go ahead, building benchmarks on CI should be acceptable.

@mikea mikea merged commit 6000b1e into main Aug 30, 2023
@mikea mikea deleted the mikea-patch-1 branch August 30, 2023 22:27
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.

3 participants