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

Enable blake3 for Bazel builds #565

Merged
merged 1 commit into from
Jan 16, 2024

Conversation

aaronmondal
Copy link
Member

@aaronmondal aaronmondal commented Dec 21, 2023

This change is Reviewable

Copy link
Member Author

@aaronmondal aaronmondal left a comment

Choose a reason for hiding this comment

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

+@allada +@adam-singer

Reviewable status: 0 of 2 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / ubuntu-22.04, Local / ubuntu-22.04, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), pre-commit-checks, publish-image, ubuntu-20.04, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable (waiting on @adam-singer and @allada)

Copy link
Member

@adam-singer adam-singer left a comment

Choose a reason for hiding this comment

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

Based on bazelbuild/bazel@0a8380b && bazelbuild/remote-apis#235 should we be considering SHA256TREE? Seems like some existing research has been done, at a high level Blake3 might be better for x86/x64 and slower than SHA256 on Arm, SHA256TREE is mentioned in the thread better on both platforms.. Not sure if its worth evaluating which digest to use based on platform, or just keep it simple, pick one for now and revisit later.

:lgtm:

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: 1 of 2 LGTMs obtained, and pending CI: docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04) (waiting on @allada)

Copy link
Member Author

@aaronmondal aaronmondal left a comment

Choose a reason for hiding this comment

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

@adam-singer Interesting. Is that used in the same way as the other ones though? If something other than blake3 is faster on arm we could set the digest function to different values for different configurations to get the most out of all CI runners.

cross-ref #566

Reviewable status: 1 of 2 LGTMs obtained, and pending CI: docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04) (waiting on @allada)

Copy link
Member

@allada allada left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 2 of 2 LGTMs obtained, and pending CI: docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04)

Copy link
Member

@allada allada left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 2 LGTMs obtained, and pending CI: docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04)

a discussion (no related file):
verify store does not yet support blake3:


Copy link

vercel bot commented Jan 11, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nativelink-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 14, 2024 7:07pm

Copy link
Member

@allada allada left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: 2 of 2 LGTMs obtained, and pending CI: Remote / large-ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04)

Copy link
Member

@allada allada left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1.
Reviewable status: 2 of 2 LGTMs obtained, and pending CI: Remote / large-ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04)

allada
allada previously requested changes Jan 14, 2024
Copy link
Member

@allada allada left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 2 LGTMs obtained, and pending CI: Remote / large-ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04)


deployment-examples/docker-compose/local-storage-cas.json line 28 at r2 (raw file):

        },
        "verify_size": true,
        "hash_verification_function": "blake3"

Hmmm, lets remove verification store here all together. We use it in testing and by default bazel uses sha256, so it'll make this not work "out-of-the-box" unless the user adds cli flags (and has clients that support blake3).

If we remove verification store it should just work.

Copy link
Member Author

@aaronmondal aaronmondal left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 2 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), publish-image, windows-2022 / stable


deployment-examples/docker-compose/local-storage-cas.json line 28 at r2 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

Hmmm, lets remove verification store here all together. We use it in testing and by default bazel uses sha256, so it'll make this not work "out-of-the-box" unless the user adds cli flags (and has clients that support blake3).

If we remove verification store it should just work.

Done.

Copy link
Member

@allada allada left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3.
Reviewable status: :shipit: complete! 2 of 2 LGTMs obtained

Copy link
Member

@adam-singer adam-singer left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: :shipit: complete! 2 of 2 LGTMs obtained

@aaronmondal aaronmondal dismissed allada’s stale review January 16, 2024 17:28

Broken in reviewable.

@aaronmondal aaronmondal merged commit 5744813 into TraceMachina:main Jan 16, 2024
22 checks passed
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