My PR passes CI. The macro works. Torch tests run on both Linux and macOS. I verified it locally, I verified it in CI, I stared at bazel query output until my eyes glazed over. Everything is green.
I can’t hit merge.
Not because it’s broken. Because something feels off — that specific engineering gut feeling where the code does what you asked but you can see the shape of a better version just past the edge of what you built. And merging now means either living with the wrong abstraction or doing the work twice.
What the PR does
PR #23305 makes our py_test macro torch-aware on both platforms. I wrote about the technical details already — the short version is that the macro detects @pypi//torch in your deps and generates platform-specific targets so torch tests work on both Linux (system torch in Docker) and macOS (PyPI torch).
It works. That’s not the problem.
Three targets from one line
Here’s the problem. Every torch test now expands into three targets:
py_test(name = "test_foo", deps = ["@pypi//torch"])
↓ macro expansion
test_foo_linux → py_venv_test, system site-packages, Linux only
test_foo_macos → _py_test, @pypi//torch, macOS only
test_foo → test_suite wrapping both
Run bazel query //pkg:all and you get a wall of suffixed targets. test_model, test_model_linux, test_model_macos. For every torch test. Developers will see these and wonder what they are. They’ll try to run test_model_linux directly on their Mac and it’ll silently not exist. They’ll wonder why bazel query returns three results for every test they wrote.
It’s correct. It’s also ugly. And ugly in build systems has a way of compounding — because the next person who writes a macro will look at yours and think “I guess this is how we do things” and now you have six-target expansions.
The simpler thing I didn’t try first
Halfway through the session where I was debugging the macro, I had the obvious realization: why not use py_venv_test for both platforms?
py_venv_test(
name = "test_foo",
include_system_site_packages = select({
"@platforms//os:linux": True,
"@platforms//os:macos": False,
}),
deps = select({
"@platforms//os:macos": ["@pypi//torch"],
"@platforms//os:linux": [],
}),
)
One target. One rule. Two select() calls. On Linux, it picks up system torch from Docker. On macOS, it acts hermetic with @pypi//torch as a regular dep.
I tested the key assumption — does py_venv_test with include_system_site_packages = False work on macOS? — and yes. It works perfectly. It’s just a normal hermetic Python test at that point.
So the three-target approach might be overengineered. I didn’t test the full select() version end-to-end yet, but the hard part — “can one rule serve both platforms?” — checks out.
This is the part that makes me hesitate. I can see the cleaner version. I just haven’t built it. Merging the ugly version means either living with it or doing a follow-up PR to simplify it. And “I’ll clean it up in a follow-up” is one of those lies engineers tell themselves with the same conviction as “I’ll start going to the gym on Monday.”
Ryan’s question that reframed everything
Then Ryan asked the question.
“What about transitive deps?”
The macro detects torch tests by checking if @pypi//torch is in the target’s deps list. Simple. Except — most tests that need torch don’t directly import torch. They import mai_layers or mai_trainer or some other internal library that imports torch three layers deep. Gazelle only adds deps for direct imports. So the deps list doesn’t contain @pypi//torch. And the macro doesn’t fire.
I traced through a concrete example. item_response_theory/tests/test_models.py — a test that definitely needs torch at runtime. Does it import torch? No. Does its BUILD file have @pypi//torch in deps? Only because someone manually added @pypi//torch # keep. That # keep comment tells gazelle “don’t remove this dep even though you can’t find the import.”
That works for one test. It does not scale to a monorepo with hundreds of torch-dependent tests where the torch dependency is three hops away in the import graph.
One question. That’s all it took to reframe the problem from “how do I run a torch test on both platforms” to “how does Bazel even know a test needs torch?” My macro answers the first question. Nobody’s answered the second one.
The commit history as a smell
I know a PR is in trouble when the commit history tells a story of uncertainty. This one’s commits go roughly:
- Add the macro with torch detection
- Add
gazelle:resolve py torch @pypi//torchto root BUILD - Realize that broke something, partially revert
- Miss a file in the revert
- Fix the missed file
- Tweak the macro again
If you had to revert twice, the approach isn’t settled. The diff on GitHub is confusing — reviewers would have to mentally reconstruct what changed from the noise of do-undo-redo. I could squash, sure. But squashing doesn’t fix the underlying problem that I was figuring out the approach while committing. The messy history is an honest reflection of messy thinking.
The tension
Here’s what I’m actually wrestling with.
The case for merging now: It works. Tests pass on both platforms. It’s strictly better than what exists today (Linux-only torch targets). The three-target expansion is cosmetically annoying but functionally correct. Ship it, iterate later.
The case for waiting: The select() approach is cleaner and I’ve validated the key assumption. The transitive dep problem means the macro’s detection logic is fundamentally incomplete — I’d be shipping an abstraction that only works for the easy case. And the commit history suggests the design isn’t done cooking.
“Ship the working thing” is usually the right call. You can always iterate. But there’s a version of this that’s a trap — where you ship something that’s just good enough that nobody bothers to improve it, and it becomes load-bearing before you get back to it. Six months from now, someone will be debugging why their torch test doesn’t get the platform split, and they’ll trace it to the macro, and they’ll find that it only works for direct @pypi//torch deps, and they’ll add another # keep comment, and the cycle continues.
I’ve been that person. The one staring at a macro wondering “why was it written this way?” The answer is almost always: “because someone shipped it before the abstraction was right and then never came back.”
What I think the right answer is
Hold the merge. Implement the select() approach — one target, no suffix explosion. That’s maybe a day of work.
The transitive dep problem is harder and I don’t have a clean solution yet. But at least the select() version is the right shape — one target per test, platform behavior handled at the attribute level. If someone later solves “how do we auto-detect torch need,” it plugs into a single-target macro much more naturally than into a three-target-with-test-suite monster.
Sometimes the right engineering judgment isn’t “does it work?” It’s “is this the thing I want to maintain?” And maintaining three-target expansion with incomplete detection logic is not a future I want to build toward.
Ryan’s question didn’t break anything. It just made the gap between “it works” and “it’s right” impossible to ignore.
The uncomfortable part
I’ll be honest about something. Part of the hesitation isn’t engineering judgment. It’s ego.
I spent real time on this macro. I debugged it, I wrote a whole blog post about the technical problem it solves, I verified it across platforms. Admitting “actually, there’s a simpler approach I should have tried first” is a specific flavor of embarrassing — the kind where you realize the detour was avoidable.
But that’s… fine? The detour taught me things. I understand target_compatible_with vs select() at a level I wouldn’t have reached by going straight to the clean solution. I understand why the three-phase model (gazelle → macro → analysis) matters. I know exactly where the transitive dep problem lives.
The macro was the wrong shape. The thinking wasn’t wasted. Those are two different things and it took me a minute to untangle them.
Closing the laptop. Not merging today.