Skip to content

feat(targethasher): collapse bzlmod external targets to repo-level hash - #312

Open
yushan8 wants to merge 23 commits into
mainfrom
bzlmod-collapse
Open

feat(targethasher): collapse bzlmod external targets to repo-level hash#312
yushan8 wants to merge 23 commits into
mainfrom
bzlmod-collapse

Conversation

@yushan8

@yushan8 yushan8 commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Summary

For legacy WORKSPACE repos, tango collapses external source/generated files to a single //external:repo hash so HashRecursively doesn't visit each file individually. Bzlmod repos (@@repo+...//...) don't have //external: rules, so this collapse was skipped — every external source file got hashed one by one during the DFS. On a python bzlmod repo (2.49M targets, 82% external pip wheels), this made HashRecursively take ~15 minutes single-threaded.

This PR adds the equivalent collapse for bzlmod using Bazel's marker file content hashes from $(output_base)/external/@repo.marker. Each marker file contains a hash of the repository rule's inputs (URL, sha256, patches, etc.) that changes on any version bump — even for repos like protobuf+ whose canonical name stays the same across versions.

How it works

  1. After bazel query, read marker files from $(output_base)/external/
  2. For each bzlmod external source/generated file, look up its repo's marker hash
  3. If found, pre-hash the target with sha1(marker_content_hash) before the DFS
  4. If no marker exists, skip collapsing — let HashRecursively hash the actual file content

Why marker files instead of repo name hashing

Bzlmod canonical repo names fall into two categories:

  • With content hash suffix (e.g. rules_python++pip+..._a6ebbe51): 10,585 repos, 1.87M files — repo name changes on upgrade
  • Without content hash suffix (e.g. protobuf+, grpc+, zlib+): 455 repos, 104K files — repo name stays the same on upgrade

Hashing just the repo name string would miss version bumps for the second category. Marker files solve this because they always reflect the repository rule's inputs.

Benchmark (2.49M targets)

Metric Before After
HashRecursively 899s ~120s
Total compute 969s (16.2 min) ~182s (3 min)

Test plan

  • Unit tests for bzlmodRepoName, collapseBzlmodExternalTargets (marker lookup, no-marker fallback, excluded regex, fullHashRepos skip)
  • End-to-end test: fromProto with excluded bzlmod targets gets empty hash, not marker hash
  • Benchmarked on uber-one with --bypass-cache
  • Verified target set is identical (2,493,050 targets) between original and collapsed
  • Repos without markers fall through to HashRecursively for file content hashing

@yushan8
yushan8 requested review from a team as code owners September 10, 2026 17:41
@yushan8
yushan8 marked this pull request as draft September 10, 2026 17:44
For bzlmod repos (@@repo+...//...), source file and generated file targets
are now pre-hashed using a single hash derived from the canonical repo name
before the DFS traversal in HashRecursively. This mirrors the existing
//external:repo collapsing for legacy WORKSPACE repos but adapted for
bzlmod's @@repo//... naming convention.

The canonical bzlmod repo name encodes the module version and content hash
(e.g. "rules_python++pip+third_party_python_base_311_torch_..._a6ebbe51"),
so hashing the name produces a stable, content-aware representative hash
that changes when the repo content changes.

On uber-one (2.49M targets, 82% external bzlmod source files):
- Collapsed 2,042,237 targets in 1.84s
- HashRecursively: 899s → 38s (23.7x speedup), single-threaded
- Total compute: 969s → 108s (9x speedup)

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
yushan8 and others added 14 commits September 10, 2026 12:06
The collapse ran before HashRecursively and didn't check excludedRegex,
so excluded external targets (e.g. rules_python sdist/whl patterns)
got a repo-name hash instead of the standard empty-hash treatment.
Now excluded targets are skipped by the collapse and fall through to
HashRecursively for proper exclusion handling.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…d tests

Move collapseBzlmodExternalTargets into the else branch of the
!useBzlmod check since the two paths are mutually exclusive.
Remove the collapsed count return value. Add unit tests for
bzlmodRepoName and collapseBzlmodExternalTargets covering:
collapse of source/generated files, rule target skip, fullHashRepos
skip, excluded regex skip, different repos get different hashes,
already-hashed skip, and an end-to-end fromProto test verifying
excluded bzlmod targets get empty hash.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Instead of hashing the repo name string (which doesn't change for repos
like protobuf+ when the version is bumped), read the content hash from
Bazel's marker files at $(output_base)/external/@repo.marker. The marker
hash is derived from the repository rule's inputs (URL, sha256, patches)
and changes on any upgrade.

If no marker file exists for a repo, skip collapsing entirely and let
HashRecursively hash the actual file content — this ensures correctness
for repos we can't identify as content-addressed.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
If marker file reading fails entirely (output_base unreachable, external
dir missing), that's a real problem — silently falling back to no
collapsing would cause a 16-minute run with no indication why.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Propagate errors from marker file reads instead of silently skipping
- Wrap errors with context (fmt.Errorf %w) per guide's error handling
- Use defer func() { _ = f.Close() }() to signal intent on read-only fd
- Handle scanner.Err() nil case explicitly for empty marker files
- Early return from collapseBzlmodExternalTargets when no markers provided
- Remove oversized capacity hint on hashes map

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The output base is a runtime value resolved by the CLI, not a concern of
the targethasher or the Bazel query interface. Pass it through
NativeGraphRunnerParams so the graph runner reads marker files directly
from the filesystem without coupling to the Bazel client.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Without this, a missing OutputBase silently disables collapsing and the
run takes 16 minutes with no indication why.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
No need to parse the marker file format. Just sha1(file_content) — the
content is deterministic for a given repo rule invocation, so the hash
is stable as long as the dependency doesn't change.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The first line is the repo rule input hash, stable across runs for the
same dependency version. The remaining lines include ENV variables that
could differ between CI environments without a dependency change,
causing false positive hash changes.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The output base is now provided by the bazel client module via
Bazel.OutputBase(ctx), using the same resolved bazel command and
workspace configuration as ExecuteQuery. The graph runner calls it
after the query completes and passes the path to readRepoMarkerHashes.

Removes OutputBase from NativeGraphRunnerParams — callers no longer
need to resolve it themselves.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Simplify the guard clauses in collapseBzlmodExternalTargets into a
single shouldCollapse predicate. Update copyright to 2026 for new file.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- shouldCollapse: describe marker-based pre-hashing, not repo name hashing
- collapseBzlmodExternalTargets: document marker file usage and fallback
- RepoMarkerHashes: clarify these are repo rule input hashes, not content hashes
- readRepoMarkerHashes: clarify it reads the first line, not the whole file
- fromProto call site: update inline comment to reflect marker-based collapse
- Separate shouldCollapse return into explicit if statements

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- shouldCollapse: nil-check target pointer before accessing fields
- OutputBase: return error if bazel info returns empty path
- readRepoMarkerHashes: validate outputBase is non-empty

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Marker file reading belongs with the hashing logic in core/targethasher.
The graph runner calls targethasher.ReadRepoMarkerHashes which resolves
the bazel command via bazel.DetectBazelExecutable (now exported) and
runs bazel info output_base itself — no changes to the Bazel interface.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@yushan8
yushan8 marked this pull request as ready for review September 11, 2026 17:27
yushan8 and others added 5 commits September 11, 2026 10:37
OutputBase resolves the bazel executable internally via
DetectBazelExecutable, so callers no longer need to handle
detection separately. Updated BUILD.bazel files via gazelle.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Uses a fake bazel script and marker files on disk to exercise
the end-to-end bzlmod collapse without a real bazel installation.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Comment thread graphrunner/native.go
markerStart := time.Now()
repoMarkerHashes, err = targethasher.ReadRepoMarkerHashes(ctx, ws.Path(), g.config.BazelCommandPath)
g.emitter.DurationHistogram(_opCompute, "marker_read_duration", metrics.FastDurationBuckets).RecordDuration(time.Since(markerStart))
if err != nil {

@xytan0056 xytan0056 Sep 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
if err != nil {
if err != nil || len(repoMarkerHashes) <= 0{

when bzlmodEnable==true, do you think we must require repo hashes to be non-empty? or fallback to hashing external files if repo hash is not found?

@yushan8 yushan8 Sep 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actually I think there should be a .marker file for every external repo as long as it's in the dependency graph of the repo.
bazel query 'deps(//...all-targets)' should download the external dependencies + their marker files.
Updated HashExternalTargetsBzlmod to return an error if a marker file doesn't exist. I tested it against the python repo and it finished running successfully. This means every external target that is in the target graph will have a marker file associated with it.

Comment thread core/targethasher/graph.go Outdated
}

func fromProto(ctx context.Context, r *buildpb.QueryResult, hasher SourceHasher, workspaceroot string, fullHashRepos set.Set[string], sequentialHashTargets set.Set[string], excludedRegex []*regexp.Regexp, useBzlmod bool) (Result, error) {
func fromProto(ctx context.Context, r *buildpb.QueryResult, hasher SourceHasher, workspaceroot string, fullHashRepos set.Set[string], sequentialHashTargets set.Set[string], excludedRegex []*regexp.Regexp, useBzlmod bool, repoMarkerHashes map[string][]byte) (Result, error) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

next PR, let's make these params a struct

Comment thread core/targethasher/graph.go Outdated
Comment thread core/targethasher/graph.go Outdated
// hash is content-aware even for repos whose canonical name stays the same
// across versions (e.g. "protobuf+"). Repos without a marker are skipped
// and fall through to HashRecursively for per-file content hashing.
func collapseBzlmodExternalTargets(targets map[string]*Target, fullHashRepos set.Set[string], excludedRegex []*regexp.Regexp, repoMarkerHashes map[string][]byte) {

@xytan0056 xytan0056 Sep 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
func collapseBzlmodExternalTargets(targets map[string]*Target, fullHashRepos set.Set[string], excludedRegex []*regexp.Regexp, repoMarkerHashes map[string][]byte) {
// TODO: hash dependency/attributes of repo too
func collapseBzlmodExternalTargets(targets map[string]*Target, fullHashRepos set.Set[string], excludedRegex []*regexp.Regexp, repoMarkerHashes map[string][]byte) {

I just realized that in HashExternalTargets, we needed to call hashRecursively for external repos too because they have have patches. If patches change, the hash of the external repo should change too.
This is done via single_version_override in bzlmod, could you verify bazel query/marker file include that info and we hash that too?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point. Synced offline, the marker file provides the patch file and the hash of the contents.

0550009ebfa63439c1ddfc210c82aded99c7acfa211fa75808937f24ef3ba690
ENV:BAZEL_HTTP_RULES_URLS_AS_DEFAULT_CANONICAL_ID \0
FILE:@@//third_party/patches/rules_proto_grpc_legacy_python_grpc.patch 149f920174b2d81db398ec9d0f8d3a2031b0be577c41c240fabba2538b69e28b

Updated the PR to hash the whole marker file instead of just the first line.

The first line of a marker file only hashes the repo rule's
declarative inputs (URL, version, patch paths) but not the actual
patch file contents. FILE: lines contain per-file SHA-256 content
hashes that change when patches are modified. Hash all stable
lines (first line + FILE lines, skip ENV lines) so patch content
changes are detected.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@yushan8
yushan8 marked this pull request as draft September 11, 2026 21:28
@yushan8
yushan8 marked this pull request as ready for review September 11, 2026 21:40
yushan8 and others added 2 commits September 11, 2026 14:51
- Rename shouldCollapse → shouldCollapseToBzlmodRepo
- Rename collapseBzlmodExternalTargets → HashExternalTargetsBzlmod
  to mirror legacy HashExternalTargets
- Error when bzlmod enabled but no marker hashes found
- Update doc comments with patch handling details

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
After bazel query completes, every referenced external repo should
have a marker file. A missing marker indicates something is wrong
with the output base rather than a normal fallback scenario.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.

2 participants