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

Lock markers and groups #9427

Merged
merged 3 commits into from
Nov 15, 2024

Conversation

radoering
Copy link
Member

@radoering radoering commented May 20, 2024

Pull Request Check List

Requires: #9345
Requires: python-poetry/poetry-core#733

  • Added tests for changed code.
  • Updated documentation for changed code.

First commit

Write information about the resulting groups that require a locked package and the resulting marker to the lock file. (Information is only written, not used yet.)

Second commit

Introduce a new config option installer.re-resolve (default true); when set to false the marker information from the lock file is used, there is no re-resolving anymore when installing from the lock file.

Motivation

What triggered me was the discussion at https://discuss.python.org/t/lock-files-again-but-this-time-w-sdists and especially https://github.com/brettcannon/peps/blob/lock-file/peps/pep-9999.rst, which might become a draft for a general lock file. To support this lock file format, we have to lock the resulting markers for packages so that re-resolving is not required for installation.

Further

Performance

We have to distinguish between locking and installing.

Locking

There is an additional step to calculate the resulting markers so this change can only make things slower. However, I could not measure a significant increase in time required for locking for several projects. I suppose that's because what we are doing in the additional step are mostly marker operations, which have probably been cached during dependency resolution before.

Installing

Evaluating markers instead of re-resolving could be faster or slower. However, I also could not measure a significant effect here. I suppose determining the exact packages to install is just not a bottleneck compared to the actual installation. Even when all packages were already installed (and installation is a noop), I could not measure a difference. However, there is a side effect:

Side effect on installation process

With re-resolving we are installing batches of packages in parallel. The batches are determined by the depth in the dependency graph (same depth -> same batch). Since we do not lock the depth, without re-resolving all packages are in the same batch.

On the one hand, this can have a positive effect on performance because we do not have to wait for the last package of a batch.
(I could observe the complete installation of the dependencies of a largish project with all packages in the cache going down from 70s to 60s.)

On the other hand, this could have a negative effect on robustness (due to race conditions). I suppose, creating batches of packages with the same depth results in only installing packages in parallel that are (more or less) independent of each other.
(If a package depends on another package, they will normally have a different depth.) Parallel installation might be an issue if two packages write to the same directory (name space packages) and try to create it at the same time.

Conclusion

Locking the resulting groups and markers opens up some new possibilities, especially for poetry-plugin-export. Installation without re-resolving is not a game changer on its own. Only the side effect will probably decrease the time for installation in some cases.

For now, I decided to make installation without re-resolving opt-in. We can also make it opt-out if we think it is safe enough and want to get more feedback.

@radoering radoering changed the title Lock markers and groups3a Lock markers and groups May 20, 2024
@radoering radoering mentioned this pull request May 27, 2024
radoering added a commit to radoering/poetry that referenced this pull request Jun 2, 2024
radoering added a commit to radoering/poetry that referenced this pull request Jun 2, 2024
…ting locked markers) (python-poetry#9427)

- introduce "installer.re-resolve" config option (default: True)
- if the config option is set to False and the lock file is at least version 2.1, the installer will not re-resolve but evaluate locked markers
radoering added a commit to radoering/poetry that referenced this pull request Jun 16, 2024
radoering added a commit to radoering/poetry that referenced this pull request Jun 16, 2024
…ting locked markers) (python-poetry#9427)

- introduce "installer.re-resolve" config option (default: True)
- if the config option is set to False and the lock file is at least version 2.1, the installer will not re-resolve but evaluate locked markers
radoering added a commit to radoering/poetry that referenced this pull request Jun 16, 2024
…ting locked markers) (python-poetry#9427)

- introduce "installer.re-resolve" config option (default: True)
- if the config option is set to False and the lock file is at least version 2.1, the installer will not re-resolve but evaluate locked markers
@radoering radoering added the impact/docs Contains or requires documentation changes label Jun 16, 2024
Copy link

github-actions bot commented Jun 16, 2024

Deploy preview for website ready!

✅ Preview
https://website-cj4e1qve6-python-poetry.vercel.app

Built with commit bf1270b.
This pull request is being automatically deployed with vercel-action

radoering added a commit to radoering/poetry that referenced this pull request Jul 14, 2024
radoering added a commit to radoering/poetry that referenced this pull request Jul 14, 2024
…ting locked markers) (python-poetry#9427)

- introduce "installer.re-resolve" config option (default: True)
- if the config option is set to False and the lock file is at least version 2.1, the installer will not re-resolve but evaluate locked markers
radoering added a commit to radoering/poetry that referenced this pull request Jul 29, 2024
radoering added a commit to radoering/poetry that referenced this pull request Jul 29, 2024
…ting locked markers) (python-poetry#9427)

- introduce "installer.re-resolve" config option (default: True)
- if the config option is set to False and the lock file is at least version 2.1, the installer will not re-resolve but evaluate locked markers
radoering added a commit to radoering/poetry that referenced this pull request Aug 24, 2024
radoering added a commit to radoering/poetry that referenced this pull request Aug 24, 2024
…ting locked markers) (python-poetry#9427)

- introduce "installer.re-resolve" config option (default: True)
- if the config option is set to False and the lock file is at least version 2.1, the installer will not re-resolve but evaluate locked markers
radoering added a commit to radoering/poetry that referenced this pull request Sep 29, 2024
radoering added a commit to radoering/poetry that referenced this pull request Sep 29, 2024
…ting locked markers) (python-poetry#9427)

- introduce "installer.re-resolve" config option (default: True)
- if the config option is set to False and the lock file is at least version 2.1, the installer will not re-resolve but evaluate locked markers
radoering added a commit to radoering/poetry that referenced this pull request Oct 4, 2024
radoering added a commit to radoering/poetry that referenced this pull request Oct 4, 2024
…ting locked markers) (python-poetry#9427)

- introduce "installer.re-resolve" config option (default: True)
- if the config option is set to False and the lock file is at least version 2.1, the installer will not re-resolve but evaluate locked markers
@Secrus Secrus added this to the Poetry 2.0 milestone Oct 6, 2024
radoering added a commit to radoering/poetry that referenced this pull request Oct 19, 2024
radoering added a commit to radoering/poetry that referenced this pull request Oct 19, 2024
…ting locked markers) (python-poetry#9427)

- introduce "installer.re-resolve" config option (default: True)
- if the config option is set to False and the lock file is at least version 2.1, the installer will not re-resolve but evaluate locked markers
radoering added a commit to radoering/poetry that referenced this pull request Oct 20, 2024
radoering added a commit to radoering/poetry that referenced this pull request Oct 20, 2024
…ting locked markers) (python-poetry#9427)

- introduce "installer.re-resolve" config option (default: True)
- if the config option is set to False and the lock file is at least version 2.1, the installer will not re-resolve but evaluate locked markers
radoering added a commit to radoering/poetry that referenced this pull request Oct 20, 2024
radoering added a commit to radoering/poetry that referenced this pull request Oct 20, 2024
…ting locked markers) (python-poetry#9427)

- introduce "installer.re-resolve" config option (default: True)
- if the config option is set to False and the lock file is at least version 2.1, the installer will not re-resolve but evaluate locked markers
radoering added a commit to radoering/poetry that referenced this pull request Oct 29, 2024
radoering added a commit to radoering/poetry that referenced this pull request Oct 29, 2024
…ting locked markers) (python-poetry#9427)

- introduce "installer.re-resolve" config option (default: True)
- if the config option is set to False and the lock file is at least version 2.1, the installer will not re-resolve but evaluate locked markers
Copy link
Member

@Secrus Secrus left a comment

Choose a reason for hiding this comment

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

Not my area of expertise, so not 100% sure, but looks pretty good to me, minor comments

src/poetry/installation/installer.py Outdated Show resolved Hide resolved
src/poetry/installation/installer.py Show resolved Hide resolved
src/poetry/installation/installer.py Outdated Show resolved Hide resolved
…ting locked markers) (python-poetry#9427)

- introduce "installer.re-resolve" config option (default: True)
- if the config option is set to False and the lock file is at least version 2.1, the installer will not re-resolve but evaluate locked markers
@radoering radoering merged commit 6f6fd70 into python-poetry:main Nov 15, 2024
74 checks passed
reesehyde added a commit to reesehyde/poetry that referenced this pull request Nov 18, 2024
* origin/main:
  Upgrade minimal Cleo to 2.2.1
  make `allow-prereleases` a tri-state setting to really forbid pre-releases if the setting is `false` and keep default behavior to allow pre-releases only if necessary
  feat: add confirmation step
  feat(cli): add info messages about applied migration
  docs: add information about --migrate
  feat(cli): add support for --local for --migrate
  feat(cli): add --migration option to config command
  feat(config): add ConfigSourceMigration
  feat(config): provide method to remove empty config category
  feat(config): add get_property() to ConfigSource
  Ignore http credentials with empty usernames
  Fix regression when using empty username/password
  fix index error for yanked releases without dependencies
  ignore installed packages during solving
  vcs: use peeled ref when retrieving tag revision (python-poetry#9849)
  chore: update json fixtures and improve generate script so that it produces the same results on Linux and Windows
  installer: add option to install without re-resolving (just by evaluating locked markers) (python-poetry#9427) - introduce "installer.re-resolve" config option (default: True) - if the config option is set to False and the lock file is at least version 2.1, the installer will not re-resolve but evaluate locked markers
  locker: lock transitive marker and groups for each package (python-poetry#9427)
  fix: do not ignore local config for implicit PyPI source (python-poetry#9816)
  Cleanup, linting, typing (python-poetry#9839)

# Conflicts:
#	src/poetry/installation/installer.py
#	src/poetry/puzzle/solver.py
reesehyde added a commit to reesehyde/poetry that referenced this pull request Nov 18, 2024
* origin/main:
  Upgrade minimal Cleo to 2.2.1
  make `allow-prereleases` a tri-state setting to really forbid pre-releases if the setting is `false` and keep default behavior to allow pre-releases only if necessary
  feat: add confirmation step
  feat(cli): add info messages about applied migration
  docs: add information about --migrate
  feat(cli): add support for --local for --migrate
  feat(cli): add --migration option to config command
  feat(config): add ConfigSourceMigration
  feat(config): provide method to remove empty config category
  feat(config): add get_property() to ConfigSource
  Ignore http credentials with empty usernames
  Fix regression when using empty username/password
  fix index error for yanked releases without dependencies
  ignore installed packages during solving
  vcs: use peeled ref when retrieving tag revision (python-poetry#9849)
  chore: update json fixtures and improve generate script so that it produces the same results on Linux and Windows
  installer: add option to install without re-resolving (just by evaluating locked markers) (python-poetry#9427) - introduce "installer.re-resolve" config option (default: True) - if the config option is set to False and the lock file is at least version 2.1, the installer will not re-resolve but evaluate locked markers
  locker: lock transitive marker and groups for each package (python-poetry#9427)
  fix: do not ignore local config for implicit PyPI source (python-poetry#9816)
  Cleanup, linting, typing (python-poetry#9839)

# Conflicts:
#	src/poetry/installation/installer.py
#	src/poetry/puzzle/solver.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact/docs Contains or requires documentation changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants