Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Config inheritance #4366

Closed
smackesey opened this issue Dec 21, 2022 · 12 comments
Closed

Config inheritance #4366

smackesey opened this issue Dec 21, 2022 · 12 comments
Labels
addressed in next version Issue is fixed and will appear in next published version enhancement request New feature or request

Comments

@smackesey
Copy link

Is your feature request related to a problem? Please describe.

I work with pyright in a large monorepo with ~70 python packages (Dagster). Because some packages have dependency conflicts with one another, they can't all be installed in the same Python environment. This prevents us from having a single pyright config in a root pyproject.toml that configures a single pyright instance (which would be the ideal scenario). Instead, @erictraut recommended here that we:

  • place individual pyright configurations in different packages
  • use VS-code multi-root workspace to run them simultaneously during development
  • run pyright once for each package/configuration in CI

This is a feasible solution, but it is arduous to maintain given the way pyright is currently configured. The only settings we need to vary across packages are import resolution-related settings like venv, venvPath, extraPaths, include, exclude. All other settings should be constant across packages in the repo. Currently, there is no way to factor out these common settings without some kind of build step for pyright config-- they have to be duplicated in each individual pyright configuration file.

Describe the solution you'd like

Support an extends key in config that takes a path to another config file. Use this to merge the settings of the two configuration files:

### pyproject.toml (root)

pythonVersion = "3.7"
useLibraryCodeFortypes = false
reportInvalidStringEscapeSequences = false
...

### some/package/pyproject.toml

extends = "../../pyproject.toml"

include = [ "some/src" ]
venvPath = "./tox"
venv = "pyright"

Additional context

This sort of feature is very useful for monorepo development and is available in many other linter tools. It is also now supported by the super-fast Python linter Ruff.

@smackesey smackesey added the enhancement request New feature or request label Dec 21, 2022
@erictraut
Copy link
Collaborator

This is something we've discussed (and rejected) previously in this issue. The config mechanism in pyright is already very complex because it composes configuration information from many sources, and the rules for how each setting composes is already complex to maintain and describe. For that reason, I'm reluctant to add yet more complexity to this.

It's something we might consider if there's sufficient demand, but so far, you're only the second person to request it, and the other issue hasn't received any further upvotes.

@dmvinson
Copy link

dmvinson commented Sep 6, 2023

I don't want to open a new issue, but would just like to file a +1 for this feature. Duplicating pyright configs for a monorepo is becoming more and more unwieldy, and we can't merge projects unfortunately

@jamestrousdale
Copy link

jamestrousdale commented May 22, 2024

@erictraut Could you elaborate on your statement regarding the complexity of the request?

From a user point of view, it seems that we have (and are forced to have) a single source of truth for our pyright configuration - this is either the pyrightconfig.json, or if it's not available, the pyproject.toml section. So where does the complexity arise from allowing a single additional field, and doing what amounts to effectively a JSON-style merge (much like what the aforementioned ruff does) to "extend" a base configuration, and consider that merged set as a the effective configuration?

I know you mentioned in the other issue that it needs to merge things like language server and CLI settings, but to me it seems kind of besides the point? Ultimately, we are just talking about the file configuration, and allowing replacement/modulation/merge behavior specifically on the file config.

I am tech lead in a relatively small (30ish engineers) software organization, but despite our size, our software stack is spread across close to 250 repositories in GitHub, roughly a quarter to a third of which at least are Python.

We manage common linter configurations via dev containers and NPM package installs as distributable artifacts, and we aim for uniformity and standardization in our software, which of course means uniformity and standardization in our linter configurations. I'm sure this is not an unusual circumstance in enterprise settings. The ability to share/merge/inherit configurations would be tremendously valuable in this setting.

Thanks for all the work on pyright - we are in the process of adopting it and giving mypy the boot right now, and it's very clear the care that's been taken in developing this product and adhering to the typing standards.

@erictraut
Copy link
Collaborator

@jamestrousdale, I think this is more complex than simply merging two JSON documents. These configurations are not simple key/value pairs. They involve lists, dictionaries, and nested objects. I think that doing a merge at the top level of the namespace will produce problematic results. The internal logic that we have for merging CLI options, language server settings, and config file settings is quite complex.

I'm interested in understanding your use case in more detail. You mentioned that you have close to 250 github repos. That's very different than the OP for this issue. In his case, he had a single monorepo. I can understand how a common config file would work for a monorepo, but it's less clear how it would work for independent repos. Where would the common config be checked in, and how would it be applied to each of the repos?

If the answer is that you programmatically generate a dev container for each project, then I would think that you could easily write a script that merges a shared pyright config file with project-specific configs and applies the merge behaviors that you desire. Is that a viable solution?

@jamestrousdale
Copy link

@erictraut Sure - like I mentioned in passing in my post, we manage shared configurations in a couple of different ways - either by packing them into dev containers or distributing them as NPM packages (which have no actual code in them, but give us a systematic way to "install" the configurations).

For example, we do this with ruff, where a core ruff configuration is installed via the NPM route, then referenced in the extend field within the pyproject.toml of each project, pointing to a location inside the node modules folder within the dev container. Projects can make selective overrides if they need to, but often they don't need to.

And yes, you are right, we can write a script to generate our own flavor of a merged pyright configuration, and indeed we may end up doing this, but for us it's an extra layer of tooling/abstraction/maintenance we'd prefer to avoid, obviously, if what we needed came "batteries included" with the tool in question.

There are always edge cases on type checking and there's certainly no one size fits all when it comes to type checker configuration (for example, we have to turn off certain things when we run on projects which use a lot of pandas because their stubs aren't and never will be complete enough). If the configuration merge is supported at the tool level, it allows us to avoid doing complex things like decisions on whether to override the project-local configuration, or wrap pyright in additional scripts which create a temporary merged configuration.

I understand it may not be a priority given, as you've mentioned a couple of times, the lack of apparent popularity of the feature and the complexity you've explained - I just wanted to explain our use case in enterprise software development and our group's particular way of working. Thanks!

@erictraut
Copy link
Collaborator

Thanks for the details. This issue has received a moderate number of upvotes, so perhaps it's worth considering.

I'll discuss with the maintainers of pylance to see if they have any opinions and/or concerns about an "extends" feature.

Here are some design considerations that we'll need to consider.

  1. Would the feature merge only at the top-most level of the JSON format, always replacing the extended key if there's a duplicate? Presumably the answer is yes. That will produce odd results in some cases, but maybe it's OK to tell users "it's up to you to ensure this doesn't result in an invalid or odd configuration".
  2. Would this work in a recursive manner? Presumably so. There would need to be protection against circularities.
  3. Would any relative paths be resolved relative to each configuration? Presumably so.
  4. Would this work with just pyrightconfig.json files, or would it work with both json and toml files? Should it be possible for one type to refer to another? I think it's unlikely that users would want to intermix the two formats, but if it's straightforward to support, I don't see a reason not to do so.

@erictraut erictraut reopened this May 23, 2024
@jamestrousdale
Copy link

jamestrousdale commented May 23, 2024

Hi @erictraut - thanks for the consideration.

My thoughts on your questions

  1. Yes - I think it's OK to defer responsibility to the end user and perform a relatively simple merge at the top level. At least in our case, I see the most frequent use case of this is to selectively disable certain rules like reportUnknownMemberType when the libraries we're using won't support strict mode (pandas is the most prominent example here)
  2. I am unsure how this would work for the monorepo case mentioned above but in our cases we always have at most 2 configuration files and we always execute from project root, so relative paths being relative to the current working directory, or the top level file, or whatever the case may be ends up being all the same. In ruff, all relative paths in nested configurations are resolved relative to the current working directory, which seems to me to be a reasonable behavior. My perception is that relative paths to non-root configurations would be difficult to manage.
  3. Addressed in 2.
  4. I think it would be nice if it worked for both, and mix. This is again how ruff works, which we've found to be a nice experience. It allows us to publish a dedicated ruff.toml file and make it commonly evailable, but then extend that inside our pyproject.toml. That being said, mixing the two formats seems more of a nice to have if it presents difficulties, from my perspective.

@debonte
Copy link
Collaborator

debonte commented May 24, 2024

defineConstant

If a config file and the config file it extends both include defineConstant, I think it's likely the user intended to concatenate those values rather than overwriting.

Relative paths

I'm confused about what the proposal is for resolving relative paths.

@jamestrousdale said:

In ruff, all relative paths in nested configurations are resolved relative to the current working directory, which seems to me to be a reasonable behavior.

From this section of the Ruff documentation, my impression is that Ruff only resolves relative to the current working directory if you run it with the --config option. In other cases, which I assume also includes running within an IDE such as VS Code, it looks up the directory hierarchy from each Python file until it discovers the closest config file, and then resolve relative paths relative to that config file.

This seems reasonable to me. So if I have a root pyrightconfig.json file that is extended by all of my project-specific pyrightconfig.json files, I could have include = ["./src"] in the root file and that would be resolved relative to my project-specific config files.

However, when @erictraut said the following above, it's not clear to me that that's what he meant.

Would any relative paths be resolved relative to each configuration? Presumably so.

Eric, what did you mean by "relative to each configuration"? Were you saying that relative paths should be resolved relative to the config file in which they are found?

@erictraut
Copy link
Collaborator

Were you saying that relative paths should be resolved relative to the config file in which they are found?

Yeah, that's what I meant. Relative paths within a config file would be resolved relative to the location of that config file.

@erictraut
Copy link
Collaborator

As for "defineConstant", it's not clear that the user intends to concatenate. You could make the same case for "include", "exclude", "ignore", "strict", "extraPaths", or "executionEnvironments". Rather than guessing the user's intent, I'd prefer to be consistent and always overwrite. This provides the most flexibility. It's also the simplest to explain in the documentation and get correct in the implementation.

@erictraut
Copy link
Collaborator

This feature will be included in the next release of pyright.

@erictraut erictraut added the addressed in next version Issue is fixed and will appear in next published version label May 25, 2024
@erictraut
Copy link
Collaborator

This is included in pyright 1.1.365.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addressed in next version Issue is fixed and will appear in next published version enhancement request New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants