Add --python-infer-string-imports
#10609
Conversation
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
| result = ( | ||
| owner.address | ||
| for owner in owner_per_import | ||
| if ( | ||
| owner.address | ||
| and owner.address.maybe_convert_to_base_target() != request.sources_field.address |
There was a problem hiding this comment.
Originally, we would not infer for a base target any dependencies on its own generated subtargets. That used to make sense. Now, though, with the model change, it makes sense for a base target to have a dependency on its generated subtarget.
|
@asherf I added you as an FYI, and if you have any feedback on the decision to not add a global option for now. No worries about reviewing the code. |
|
honestly, I don't like this way of doing things. Specifying stuff on a repo level won't scale very well and is confusing to a random user trying to add a new file with some string references to python file, I have been down this road and ended up having a regex that looks like this: UGH... I think it is much better if we can add something at the python_library() target level. |
|
@asherf, to clarify, we are not adding global excludes like you had to use with Toolchain. The PR description justifies that decision, and explains that we can add it down the road if we want. Right now, the only option is to use decentralized excludes like |
|
What we had in Toolchain is not a global exclude, but a global include, i.e. a regex for filenames in which we should be looking for strings that looks like python imports. OTOH, treating every string that looks like The other way is bad too because it will miss imports of external packages (used with django). https://django-csp.readthedocs.io/en/latest/installation.html#installing-django-csp Needing to specify those in the pants config is also not ideal. The only thing that makes sense to have in the top level pants config is the default file names (regex) in which imports should be looked for (which is usually conftest.py) everything else should only be specified on the target level. |
This would be perfectly fine. While Pants will think that it's an import, it will fail to find an owning target for that domain name, so Pants will no-op. That owning target also must be a Python target (like Pants only infers a dependency if a) there is an owning target, b) that owning target is a Python target, and c) that is the sole owning target, i.e. there are not >1 owners.
This would be achieved by adding a normal explicit dependency to the target like you would without inference enabled. That won't change. |
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
Certain Python projects, such as Django apps, make frequent use of dynamic imports. Dep inference needs to be able to understand those to work well with those code bases.
We turn string imports inference off by default because it could be surprising and it's much more likely to have false positives.
Rejected design (for now) - global ignores
With Toolchain's buildgen (which inspired dep inference), false positives were handled via a subsystem option that allowed users to put regex strings to ignore globally. At the time, there was no
!ignore syntax in thedependenciesfield, so this was the only option.Now that we have
!ignores, we start with the simplest solution to only use that for false positives. If we get user feedback that they want a decentralized mechanism, we can easily add a subsystem.(Also note that we will only infer dependencies on Python targets, whereas Toolchain's Buildgen would infer on
files()andresources(). Many of the ignores in Toolchain were for static assets, like.cssfiles.)[ci skip-rust]
[ci skip-build-wheels]