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

Support config defaults using .pip-tools.toml or pyproject.toml #1863

Merged
merged 27 commits into from
Jun 13, 2023

Conversation

j00bar
Copy link
Contributor

@j00bar j00bar commented May 6, 2023

Supports use of a .pip-tools.toml, pyproject.toml, or other explicitly specified TOML file to set configuration defaults for pip-compile and pip-sync.

Fixes #604.

Contributor checklist
  • Provided the tests for the changes.
  • Assure PR title is short, clear, and good to be included in the user-oriented changelog
Maintainer checklist
  • Assure one of these labels is present: backwards incompatible, feature, enhancement, deprecation, bug, dependency, docs or skip-changelog as they determine changelog listing.
  • Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

@j00bar j00bar force-pushed the j00bar/issue-604-pyproject-toml branch 2 times, most recently from d097397 to b73731a Compare May 7, 2023 00:25
@webknjaz
Copy link
Member

webknjaz commented May 7, 2023

@j00bar you can't trigger CI until you have at least one commit in the repo..

Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this! I was able to skim through the patch from mobile and noticed a few areas that might need some love...
Though, I'm sure someone will check the logic too. Reviews are easier from an actual computer.

I'll also approve the CI in just a bit to see how it's doing...

piptools/scripts/compile.py Outdated Show resolved Hide resolved
piptools/utils.py Outdated Show resolved Hide resolved
piptools/utils.py Outdated Show resolved Hide resolved
piptools/utils.py Outdated Show resolved Hide resolved
piptools/utils.py Outdated Show resolved Hide resolved
piptools/utils.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
tests/test_utils.py Outdated Show resolved Hide resolved
tests/test_utils.py Outdated Show resolved Hide resolved
tests/test_utils.py Outdated Show resolved Hide resolved
@webknjaz
Copy link
Member

webknjaz commented May 7, 2023

I'll also approve the CI in just a bit to see how it's doing...

The PyPy failures seem unrelated... Though, they'll probably need sorting out separately before this PR could be mergeable. I wonder if bumping the interpreter version would have any effect.

@j00bar j00bar force-pushed the j00bar/issue-604-pyproject-toml branch from 9262e29 to b4e57f4 Compare May 7, 2023 19:05
@j00bar j00bar requested a review from webknjaz May 7, 2023 19:06
@j00bar
Copy link
Contributor Author

j00bar commented May 7, 2023

Thank you for the detailed feedback!

  1. I've added support and preference for a .pip-tools.toml file.
  2. I've implemented the best practices and conventions you referenced.
  3. I've made the dependency on toml apply only pre-3.11 - the tests still require the external toml library, as tomllib reads only but does not yet write.
  4. I've updated the README to document the new configurability.

@j00bar j00bar changed the title Support config defaults using pyproject.toml Support config defaults using .pip-tools.toml or pyproject.toml May 7, 2023
@j00bar j00bar force-pushed the j00bar/issue-604-pyproject-toml branch from b4e57f4 to 6734ea0 Compare May 7, 2023 19:14
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
piptools/utils.py Outdated Show resolved Hide resolved
piptools/utils.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
piptools/utils.py Outdated Show resolved Hide resolved
piptools/utils.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
piptools/utils.py Outdated Show resolved Hide resolved
piptools/utils.py Outdated Show resolved Hide resolved
piptools/utils.py Outdated Show resolved Hide resolved
piptools/utils.py Outdated Show resolved Hide resolved
piptools/utils.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
@j00bar
Copy link
Contributor Author

j00bar commented Jun 20, 2023

  1. If I have the option cert specified in the config file, and I pass --annotation-style=line on the command line, will the cert option be used?

Yes. Each parameter follows the order of precedence independently. The full configuration, like most command-line utilities, uses a mosaic of command-line set arguments, config file settings, and hardcoded defaults.

  1. If I want to run pip-compile with the program's defaults, even if there exists a hidden .pip-tools.toml file in the current directory (i.e. I want it to act as pip-compile does in the current release), is it now necessary that I change all current calls of arg-less pip-compile to pip-compile --config /dev/null? Will that work? Is that the only way to achieve this?

It might be nice to... implement a --no-config

I don't agree. Projects use these config files to set defaults that should apply to the whole project to make the preferred way of compiling/syncing requirements be the easiest way to compile/sync requirements. That encourages consistency transparently. As the steward of many projects and having to have created automation to fail a PR when it fails consistency in the compiled requirements, I want it as laborious as possible to deviate from the project's default settings. And if there are deviations I often make, I'm going to make an additional config file with a non-default config file name.

But you're a maintainer of this one - it's up to you if you want that - to me it's a foot-gun so I'm not interested in helping with that one.

@AndydeCleyre AndydeCleyre added the backwards incompatible Backwards incompatible change label Jun 20, 2023
@atugushev
Copy link
Member

@AndydeCleyre I wonder why is this backwards incompatible?

@AndydeCleyre
Copy link
Contributor

@AndydeCleyre I wonder why is this backwards incompatible?

I still don't know the answer to this question, but this is what I was thinking of:

If I want to run pip-compile with the program's defaults, even if there exists a hidden .pip-tools.toml file in the current directory (i.e. I want it to act as pip-compile does in the current release), is it now necessary that I change all current calls of arg-less pip-compile to pip-compile --config /dev/null? Will that work? Is that the only way to achieve this?

@AndydeCleyre
Copy link
Contributor

@j00bar will using --config /dev/null work, and is it the best way to have the command use the generic defaults reliably?

@atugushev
Copy link
Member

atugushev commented Jun 28, 2023

I've noticed that the --config option reveals the absolute path in the header, which could be inconvenient. It would force users to pass the --no-header option. General rule of thumb is to hide an options with default values from header.

$ pip-compile
Using pip-tools configuration defaults found in '/Users/albert/Projects/pip-tools/pyproject.toml'.
#
# This file is autogenerated by pip-compile with Python 3.11
# by the following command:
#
#    pip-compile --config=/Users/albert/Projects/pip-tools/pyproject.toml --resolver=backtracking
#
asgiref==3.6.0
    # via django
django==4.2
    # via -r requirements.in
sqlparse==0.4.3
    # via django

Also pip-compile makes noisy log even though there is no any section in pyproject.toml regarding to pip-tools' configuration. Wouldn't it make sense to show the log when using --verbose option?

@atugushev
Copy link
Member

I still don't know the answer to this question

@AndydeCleyre

I'm inclined to think that this update does not represent a breaking change. The current update does not interfere with pip-compile's default behavior, nor does it require any modifications to existing calls. Running pip-compile without any arguments will still function as it does in the current release and shouldn't break any existing pipelines, as there were no configurations before.

However, I believe we should consider adding support for --no-config (similar to how it's done in wget or ripgrep) in the upcoming version and resolve other concerns.

Are there any objections if I remove the corresponding label and proceed with the version 6.14.0 (#1892) release process?

@webknjaz
Copy link
Member

I also don't think it's a breaking change, since no configs existed in projects using pip-tools prior to this release, hence adding one must be treated as an explicit opt-in to this feature, similar to the --config CLI option. @j00bar made a great point that a project config should be applicable by default. Though, I know that there are legitimate cases for wanting the defaults for one-off commands, and a documented way of doing so is useful for inclusivity.

The absolute path is indeed unpleasant and the --no-config suggestion is a nice to have but probably isn't a blocker…
For the /dev/null, I'd like to see a test case clearly showing what the behavior expectations are. Though, --no-config sounds like a nicer interface for such thing.

@AndydeCleyre
Copy link
Contributor

Ok, remove the breaking label and status.

I would really like to know how to consistently protect myself from unwanted configs once this is released.

And yes, that absolute path is going to be a paper cut for any teams that include the header and run pip-compile on different systems. IMO all paths in the txt output should be relative to the txt file.

@atugushev
Copy link
Member

atugushev commented Jun 29, 2023

@AndydeCleyre thanks!

I've prepared two PRs to address #1863 (comment):

Let's get this merged before the release.

@atugushev atugushev removed the backwards incompatible Backwards incompatible change label Jun 29, 2023
@atugushev
Copy link
Member

Released as part of pip-tools v6.14.0.

@AndydeCleyre
Copy link
Contributor

When using .pip-tools.toml, does there need to be a [tool.pip-tools] section, or should the values be top-level with no section header? Whichever the case, can it be noted somehow in the README?

@j00bar
Copy link
Contributor Author

j00bar commented Jul 12, 2023

Hey @j00bar, FYI there's some post-release adjustment and bugfixing efforts going on, in case you wanted to keep an eye on the changes, here's some:

Thanks! I got laid off last month, which took my attention/interest/priorities in other directions - grateful for the continued stewardship of this feature.

@chrysle
Copy link
Contributor

chrysle commented Jul 13, 2023

@AndydeCleyre

When using .pip-tools.toml, does there need to be a [tool.pip-tools] section, or should the values be top-level with no section header?

See these lines:

pip-tools/piptools/utils.py

Lines 656 to 657 in 776c3fa

# In a TOML file, we expect the config to be under `[tool.pip-tools]`
piptools_config: dict[str, Any] = config.get("tool", {}).get("pip-tools", {})

Maybe this could be simplified for .pip-tools.toml.

Whichever the case, can it be noted somehow in the README?

I think it's explained properly under the configuration section.

@j00bar
Copy link
Contributor Author

j00bar commented Jul 13, 2023

Maybe this could be simplified for .pip-tools.toml.

That was the original implementation which, in review, was rejected in favor of consistency.

@AndydeCleyre
Copy link
Contributor

Thanks. I thought there was a discussion about making it like ruff, but I can't find it so maybe dreamed it. From ruff's docs:

As an alternative to pyproject.toml, Ruff will also respect a ruff.toml (or .ruff.toml) file, which implements an equivalent schema (though in the ruff.toml and .ruff.toml versions, the [tool.ruff] header is omitted).

@webknjaz
Copy link
Member

Thanks. I thought there was a discussion about making it like ruff, but I can't find it so maybe dreamed it. From ruff's docs:

As an alternative to pyproject.toml, Ruff will also respect a ruff.toml (or .ruff.toml) file, which implements an equivalent schema (though in the ruff.toml and .ruff.toml versions, the [tool.ruff] header is omitted).

FTR, that discussion that you're recalling was about the need to have the section header in all of the files or not. It was you who mentioned Ruff in that context: #1863 (comment).

And I made an argument that it's more consistent to have this header in all files: #1863 (comment). It makes it easier to copy the section over between different files. And now I think that when the end-users want to have several sections in their non-pyproject config, they can only do so when there are sections in the file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
config Related to pip-tools' configuration enhancement Improvements to functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add config file
6 participants