Support with suppress(ImportError): in python dependency inference

[dbari]

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.

[benjyw]

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

[thejcannon]

The important thing to me is that we'd support both import contextlib + contextlib.suppress and from contextlib import suppress + suppress

[thejcannon]

Something I suggested in slack is if we just start using kbcst or equivalent

[dbari]

The important thing to me is that we'd support both import contextlib + contextlib.suppress and from contextlib import suppress + suppress

It would be easy to check for with suppress(ImportError) or with contextlib.suppress(ImportError) locally, or even add a check for import contextlib / from contextlib import suppress in 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 suppress has been imported (e.g. if someone first does from contextlib import suppress and later from anotherlib import something as suppress) and that the scoping is correct (e.g. if contextlib was only locally imported in some other scope). What do you think?

[dbari]

By the way, I'd prefer to also add an advanced option to python-infer to allow for custom dependency parsers. I believe this would be helpful in larger codebases where you might want to have custom rules, e.g. local imports in functions can be strong or weak dependencies etc.

Would an option dependency_parser (default None -> use the pants internal) where one could provide a path to a python script be ok for this, or would you prefer a different naming or a completely different solution?

[benjyw]

That's a really interesting idea! I'd be open to such an option. The main thing would be that the script has to be completely self-contained (no dependencies outside the stdlib). But that seems like a reasonable constraint, at least initially.

[thejcannon]

We should probably move that discussion somewhere else. Like a discussion 😄

[dbari]

We should probably move that discussion somewhere else. Like a discussion 😄

Opened discussion #17702 for this. Although the support for suppress and the custom parser are more or less independent points, I'd prefer to wait for the result of the discussion before proceeding with this issue.

[thejcannon]

Well everyone could benefit from the suppress support.

[dbari]

Well everyone could benefit from the suppress support.

Yes, of course. I only thought that if a custom parser can/should be implemented, it would also make the development for this issue a bit easier. If the discussion on the details takes too long, it's easier to solve this independently.

[benjyw]

I agree that it wouldn't be necessary to track what the suppress name is bound to. The name itself says what it does (e.g., if you implement your own "suppress" we would want to treat it the same!)

[benjyw]

So I think adding support for suppress in the standard script makes sense.

[thejcannon]

it would also make the development for this issue a bit easier.

You can invoke the script directly (as of today), which is how I test this quickly. The comment at the top mentions this a bit.

[thejcannon]

if you implement your own "suppress" we would want to treat it the same!

I'm not sure I agree. suppress is a pretty generic name. Although I imagine only a subset is semantically valid to contain an import. I suppose if we look for specifically suppress containing a single-argument ImportError that'd be OK

[benjyw]

We would be looking for exactly this:

with suppress(ImportError):
  ...

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...