Custom dependency parser for python

[dbari]

Discussion started in #17691:

@dbari wrote:

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 wrote:

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.

Also relevant, but going in another direction: #17477 (comment)

[thejcannon]

I wouldn't be against this, but I'd reeeeally hesitate to approve it. The use-case for such a thing I think falls into one of two categories:

• Useful for custom shenanigans: In that case, defining a plugin that alters the inferred deps is likely the solutionh
• Useful for everyone: In that case it should go into Pants proper

Otherwise, we either need to provide stability in the script, or tell people that its another thing maybe breaking with each version. Additionally, I fear the support aspect might be more difficult as well.

[dbari]

Valid point, if this is made configurable, its interface is public and needs to provide at least some degree of stability.

How about making it more configurable for everyone, by covering some cases without resorting to allowing a replacement?
E.g. having a parameter under python-infer to specify which imports to ignore completely or set to weak?

Pretty unrefined, but it could look something like this as a python dict/list, using ast names as keys to signify "any import under such a node":

"weak_imports": {
    "With": ["suppress(ImportError)", "contextlib.suppress(ImportError)"],  # meaning any import in a body of a with statement with those `withitems`
    "Try": ["ImportError"],  # any import in a body of a try statement with a handler for this exception
    "FunctionDef": None,  # any import within a function
}

[thejcannon]

Well the first two cases should be handled by the builtin script. The third one is interesting, but could also be reduced to a single toggle. Additionally I could see the case those should also be weak as well, in general.

[dbari]

My point is that with this kind of configurability, also some of the custom shenanigans would be covered, which I'm also interested in :)

So I would set "With": ["suppress(ImportError)", "contextlib.suppress(ImportError)", "OptionalImport()"], for my custom OptionalImport context manager, without having to change the script. As for the FunctionDef, you could also potentially provide names of functions as a list if you wanted. If it's all or nothing, a switch would be better.

[thejcannon]

At that level of customizability I'd rather we just handed over the keys to the car 😂

However, what I'm trying to get at is if you have a need for a custom dep parser than either you're doing something truly unique (so the plugin route would be sufficient) or you're not and maybe Pants should be beefed up, for the benefit of everyone.

---

I'm not against additions/customizations. I just want to tread lightly and make sure we're taking a path that benefits the most people, by default without breaking our own backs.

[dbari]

Yes, you're right on this, too much configurability brings additional maintenance and it does not make much sense in this case.

So at least for me, this concludes the conversation. I'll look into implementing just the support for suppress (where we started from) without any additional configurability.

Thanks for the input!