Hi all :wave: , per the contribution overview guid...
# development
f
Hi all šŸ‘‹ , per the contribution overview guide, I was hoping to gather thoughts on this issue I opened. I'm willing to draft up a PR of what the ticket describes as the ideal behavior for multi-config mypy support if there are no major objections.
h
Hi, sounds good, thanks!
gratitude thank you 1
f
Ok, so I think I have a rough sketch and proposal for how I plan to implement this. I'm leaning towards adding a new
[mypy.resolves_to_config_file]
subsystem option, rather than fully enable mypy config auto-discovery. The proposed
[mypy.resolves_to_config_file]
option would map resolve name(s) to their corresponding intended mypy config filepath (relative to the build root). If there are any resolves omitted from that new option, they default to using the existing
[mypy].config
. My thinking is that this strategy: 1. Does not change how pants' mypy partitioning rules work 2. Shouldn't lead to as many pants + mypy caching edge cases (which I'm aware is a known, difficult problem). 3. effectively follows a "subset" of the official mypy configuration features, since you can always call
mypy --config-file <foo> path/to/src
, so it does mimic an existing mypy invocation pattern. 4. Should be fully backwards compatible:with no performance or graph construction differences given the fallback to
[mypy].config
. 5. Also leaves the existing
[mypy].config_discovery
option unchanged, and similar to the
[mypy].config
option, this new option would be mutually exclusive with
[mypy].config_discovery
. One slight semantic downside: The
[mypy].config_discovery
option is still somewhat different than what mypy's "config discovery" definition is, but I think a simple update to that option's docs is reasonable way to address that.
^ cc @curved-manchester-66006 I would be curious to hear your thoughts here if you have a moment! I'm torn about 50% either the proposed way, or by adding the discovered config as a partition dimension.
c
šŸ‘€ Thanks for trying to think this through. A few questions: • Do you have an example of the type of config you are trying to vary? I understand the request in the abstract, but in my limited experience at work I've seen mypy's per module overrides be sufficient if some team wants a different value of
disallow_untyped_defs
or whatnot. • In your sketch, the config is bound to the resolves, so for targets that are parameterized by resolve it works because the targets will be checked by both configs A and B. Right? • Is this useful to you without also having https://github.com/pantsbuild/pants/issues/17749 ?
f
Do you have an example of the type of config you are trying to vary?
Agreed that the per-module overrides do cover a number of customization needs. The problem is more with the subset of mypy settings which can only be set at the "global section" (if you search for "may only be set in the global section" in the mypy docs, that gives a rough idea of settings which cannot be overridden per module without a separate config file (e.g. the plugins config). Another important config that can't be overridden per module, but is often necessary is the mypy_path setting.
In your sketch, the config is bound to the resolves, so for targets that are parameterized by resolve it works because the targets will be checked by both configs A and B. Right?
That's actually a helpful callout -- I didn't account for parametrized resolves, but I suppose that could work that way, assuming there's no conflicting mypy settings between config A and config B for a given parametrized source.
Is this useful to you without also having issues/17749?
I don't think that issue is a pre-req, or covers our need if I'm understanding it correctly, since that's more to do with opt-in mypy resolve dependencies? I'm more thinking about per-project and/or per-resolve dependency config settings, which otherwise can't be set via a per-module override.
Apologies for the flip-flop over here. After thinking through it more, I've opted for remaining consistent with mypy's config discovery behavior, rather than the "resolves_to_config_file". Here's the PR for it: github.com/pantsbuild/pants/pull/23633