Hey folks, I think dependency inference doesn't work the way we want it to: In ExplicitlyProvidedDependencies.disambiguated, I think we don't use the explicitly provided dependencies to disambiguate. If the ambiguous address is provided in the

dependencies

, we return None, and not that address. If multiple addresses are provided in

dependencies

so that it's not actually disambiguated, we still return None. Providing excludes in the

dependencies

(ex "!foo.py") does work to disambiguate the owner, but only if there are no includes (if there are includes, we return None).

Hmm

> If the ambiguous address is provided in the dependencies, we return None, and not that address. That's because the dep should be populated via

explicitly_provided_deps

here, rather than via dep inference. The

.disambiguated()

code path is doing this: • If the ambiguous module is already covered by explicit includes, then give up on dep inference • If the ambiguous module is not covered by explicit includes, then try to disambiguate it via explicit excludes https://github.com/pantsbuild/pants/blob/954f57b2d78462fd9c687662924dd3616972410f/src/python/pants/engine/internals/graph.py#L1513-L1526 > If multiple addresses are provided in dependencies so that it's not actually disambiguated, we still return None This code path is not worried about whether there is ambiguity in general. It is solely about whether dep inference should be used or not, and making a best effort to use it when disambiguation is possible If the module has ambiguous owners due to explicitly provided owners, then dep inference should indeed give up. It is not its job to worry about if the explicitly provided includes are ambiguous or not I don't think we have code to proactively error if you explicitly provide deps for the same module multiple times. I think we were concerned that would be overreach - we try (tried) to not get in the way of you manually declaring dependencies, since you're in the territory of advanced usage and trusting the user

Thanks for explaining that. If I've gotten what you're saying: The role of the "dependency inference" stage is to use analysis of source code to determine what other targets to pull in. The dep inference begins by considering all targets, so it makes sense to also consider excludes when resolving ambiguities, since the excluding hasn't happened yet. It doesn't make sense for dependency inference to consider includes to resolve ambiguities, since (as far as dependency inference is concerned) the targets referenced are already "candidates" for dependency inference to pull in (put another way, the targets included are already members of the set of target dependency inference is considering). Dependency inference should create no links if it cannot establish an owner, and an ambiguous owner fits in this case. For example, it is intended that, even with an explicit include, an ambiguous import will be flagged as ambiguous. It is intended that the "include explicit dependencies" stage will resolve the ambiguous owner situation by including the target that the user intends. Therefore, is the solution for https://github.com/pantsbuild/pants/issues/20806 simply replicating the logic like in the "maybe warn" function to not emit the error if there are explicit includes?

The role of the "dependency inference" stage is to use analysis of source code to determine what other targets to pull in.

Yep, and these augment whatever the user explicitly said. Philosophically, explicit deps are meant to take primacy because we want to "trust the user". In general, explicit deps are intended to only be for edge cases where dep inference can't figure out what to do. We encourage people to not use explicit deps unless they have a reason This distinction goes way back to the early days when Stu, Benjy, and I were trying to decide between dependency inference vs BUILD file generation, where we list out every dep explicitly but most are auto-generated. 90%+ of deps are "boring", so it's noisy to be in a BUILD file.

It doesn't make sense for dependency inference to consider includes to resolve ambiguities, since (as far as dependency inference is concerned) the targets referenced are already "candidates" for dependency inference to pull in (put another way, the targets included are already members of the set of target dependency inference is considering).

That's sort of right. Dep inference should consider explicit inclusion for ambiguities. But basically it means "This is already resolved via explicit includes, so give up on inference and trust the user with explicit includes!" Likewise, with ambiguity, dep inference should consider explicit excludes, which are a hint from the user how they want us to infer things amidst ambiguity

Dependency inference should create no links if it cannot establish an owner

That's correct. Ambiguous owners can cause runtime errors. Dep inference would rather have false negatives than false positives if we had to choose. We really don't want inference to cause errors

Therefore, is the solution for https://github.com/pantsbuild/pants/issues/20806 simply replicating the logic like in the "maybe warn" function to not emit the error if there are explicit includes?

Yep, that sounds right to me. If the user disambiguated via explicit includes, still give up on dep inference, but don't warn/error because the user fixed the situation explicitly