pants: Support `with suppress(ImportError):` in python dependency inference
Is your feature request related to a problem? Please describe.
Currently the python dependency inference of pants takes into account any imports within a try/except ImportError block and marks them as “weak” dependencies, e.g. skipping the warning if they are not provided by any requirement. Using the equivalent with contextlib.suppress(ImportError):, the dependencies are marked as “strong” and a warning is printed.
Describe the solution you’d like
Detect with contextlib.suppress(ImportError): or from contextlib import suppress; ...; with suppress(ImportError): in the dependency parser and handle those dependencies as weak.
Describe alternatives you’ve considered
Alternatively, to allow more freedom for special cases in user code, make the dependency parser configurable either through options of python-infer or a plugin interface.
I experimented a bit with the latter, but could not override the default dependency parser without too much boilerplate code or wrapping of pants classes. However this might be because I’m not familiar with writing plugins.
Additional context Tested with pants 2.14.0.
Please give feedback on what solution would be preferable.
About this issue
- Original URL
- State: closed
- Created 2 years ago
- Reactions: 1
- Comments: 15 (11 by maintainers)
Commits related to this issue
- `with contextlib.suppress(ImportError)` weakens imports (#19293) fixes #17691 also: - fixes the case of nested weakenings - bumps tree-sitter-python to an unreleased version (last release was a... — committed to pantsbuild/pants by lilatomic 10 months ago
- `with contextlib.suppress(ImportError)` weakens imports (#19293) fixes #17691 also: - fixes the case of nested weakenings - bumps tree-sitter-python to an unreleased version (last release was a... — committed to pantsbuild/pants by lilatomic 10 months ago
- `with contextlib.suppress(ImportError)` weakens imports (#19293) fixes #17691 also: - fixes the case of nested weakenings - bumps tree-sitter-python to an unreleased version (last release was a year... — committed to pantsbuild/pants by lilatomic 10 months ago
- `with contextlib.suppress(ImportError)` weakens imports (Cherry-pick of #19293) (#19788) fixes #17691 and fixes #19751 also: - fixes the case of nested weakenings - bumps tree-sitter-python to a... — committed to pantsbuild/pants by WorkerPants 10 months ago
- `with contextlib.suppress(ImportError)` weakens imports (Cherry-pick of #19293) (#19789) fixes #17691 and fixes #19751 also: - fixes the case of nested weakenings - bumps tree-sitter-python to a... — committed to pantsbuild/pants by thejcannon 10 months ago
We would be looking for exactly this:
I think the intention there is very unambiguous. If that does not, in fact, suppress the import error, then the author is playing with fire…
This is a good suggestion! It might not be too hard to modify the dep parser to do this, if you want to take a swing at it. The crucial file is https://github.com/pantsbuild/pants/blob/68894b9918d4a1d4802888238f03cbc0c68e9bfd/src/python/pants/backend/python/dependency_inference/scripts/dependency_parser_py
It would be easy to check for
with suppress(ImportError)orwith contextlib.suppress(ImportError)locally, or even add a check forimport contextlib/from contextlib import suppressin the top-level statements.However, I think that protecting from all possible complications goes too far and would be out of scope for this issue. I mean things like making sure the correct
suppresshas been imported (e.g. if someone first doesfrom contextlib import suppressand laterfrom anotherlib import something as suppress) and that the scoping is correct (e.g. ifcontextlibwas only locally imported in some other scope). What do you think?