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

Changed indirect access to more direct access methods to improve performance #2149

Closed

Conversation

glitchedcat
Copy link

When resolving libraries with large dependancy chains such as the "aws_cdk" the solve method becomes more and more expensive due to the recursive dependancy resolution. Most of this cost comes from indirect variable access (getattr() and @property). See the breakdown image below which shows getattr() accounting for 30% of the runtime.
Screenshot 2020-03-08 at 12 22 42
The change to poetry/packages/dependency_package.py results in a 30% performance improvement and the changes to poetry/packages/package.py result in a further 10% improvement (workings below).

Method

  1. Install initial 31 dependancies (see list below)
  2. Time the addition of a further dependancy time poetry add aws_cdk.aws_codestar (see original timings)
  3. Time the removal of the dependancy time poetry remove aws_cdk.aws_codestar (see original timings)
  4. Add @property accessors in poetry/packages/dependency_package.py
  5. Repeat add and remove timing (see V1 timings)
  6. Add direct access for name variable in poetry/packages/package.py
  7. Repeat add and remove timing (see V2 timings)

Timings

*Timings in seconds

Run # Original add Original remove V1 Property Add V1 Property Remove V2 Direct Name Add V2 Direct Name Remove
1 225.36 220.3 151.27 146.52 134.32 132.09
2 223.19 219.73 149.62 149.45 133.40 132.49
3 223.23 220.61 148.55 147.52 134.08 131.68
4 224.63 222.46 150.95 150.28 133.96 131.23
5 224.72 223.38 150.42 150.00 132.97 130.42
Average 224.23 221.30 150.16 148.75 133.75 131.58

Initial installed dependancies

python = "^3.7"
"aws_cdk.aws-accessanalyzer" = "^1.27.0"
"aws_cdk.aws-acmpca" = "^1.27.0"
"aws_cdk.aws-amazonmq" = "^1.27.0"
"aws_cdk.aws-amplify" = "^1.27.0"
"aws_cdk.aws-apigateway" = "^1.27.0"
"aws_cdk.aws-apigatewayv2" = "^1.27.0"
"aws_cdk.aws-appconfig" = "^1.27.0"
"aws_cdk.aws-applicationautoscaling" = "^1.27.0"
"aws_cdk.aws-appmesh" = "^1.27.0"
"aws_cdk.aws-appstream" = "^1.27.0"
"aws_cdk.aws-appsync" = "^1.27.0"
"aws_cdk.aws-athena" = "^1.27.0"
"aws_cdk.aws-autoscaling" = "^1.27.0"
"aws_cdk.aws-autoscaling-common" = "^1.27.0"
"aws_cdk.aws-autoscaling-hooktargets" = "^1.27.0"
"aws_cdk.aws-autoscalingplans" = "^1.27.0"
"aws_cdk.aws-backup" = "^1.27.0"
"aws_cdk.aws-batch" = "^1.27.0"
"aws_cdk.aws-budgets" = "^1.27.0"
"aws_cdk.aws-certificatemanager" = "^1.27.0"
"aws_cdk.aws-cloud9" = "^1.27.0"
"aws_cdk.aws-cloudformation" = "^1.27.0"
"aws_cdk.aws-cloudfront" = "^1.27.0"
"aws_cdk.aws-cloudtrail" = "^1.27.0"
"aws_cdk.aws-cloudwatch" = "^1.27.0"
"aws_cdk.aws-cloudwatch-actions" = "^1.27.0"
"aws_cdk.aws-codebuild" = "^1.27.0"
"aws_cdk.aws-codecommit" = "^1.27.0"
"aws_cdk.aws-codedeploy" = "^1.27.0"
"aws_cdk.aws-codepipeline" = "^1.27.0"
"aws_cdk.aws-codepipeline-actions" = "^1.27.0"```

yamagen0915 and others added 11 commits January 23, 2020 15:52
* export: fix exporting extras sub-dependencies (python-poetry#1294)

* Support POETRY_HOME for install (python-poetry#794)

Allow the `POETRY_HOME` environment variable to be passed during installation to change the default installation directory of `~/.poetry`:

```
POETRY_HOME=/etc/poetry python get-poetry.py
```

* * check if relative filename is in excluded file list (python-poetry#1459)

* * check if relative filename is in excluded file list
* removed find_excluded_files() method from wheel.py

* added test for excluding files in wheels

* creating an own test data folder, for testing excluding files by pyproject.toml

* use as_posix() to respect windows file path delimiters

* Exclude nested items (python-poetry#784) (python-poetry#1464)

* This PR impliments the feature request python-poetry#784.

When a folder is explicit defined in `pyproject.toml` as excluded, all nested data, including subfolder, are excluded. It is no longer neccessary to use the glob `folder/**/*`

* use `Path` instead of `os.path.join` to create string for globbing

* try to fix linting error

* create glob pattern string by concatenating and not using Path

* using `os.path.isdir()`` for checking of explicit excluded name is a folder, because pathlib's `is_dir()` raises in exception under windows of name contains globing characters

* Remove nested data when wildcards where used.

Steps to do this are:
1. expand any wildcard used
2. if expanded path is a folder append  **/* and expand again

* fix linting

* only glob a second time if path is dir

* implement @sdispater 's suggestion for better readability

* fix glob for windows?

* On Windows, testing if a path with a glob is a directory will raise an OSError

* pathlibs  glob function doesn't return the correct case (https://bugs.python.org/issue26655). So switching back to  glob.glob()

* removing obsolete imports

* Update dependencies

* Deprecate allows-prereleases in favor of allow-prereleases for consistency

* Fix tests for Python 2.7

* Fix linting

* Fix linting

* Fix linting

* Fix typing import

* Correct a couple typos in get-poetry.py (python-poetry#573)

* Docs: `self:update` changed to `self update` (python-poetry#1588)

* Fix GitHub actions cache issues on develop (python-poetry#1918)

* Fix Github actions cache issues

* Fix Github Actions cache issues (python-poetry#1928)

* Add --source option to "poetry add" (python-poetry#1912)

* Add --source option to 'poetry add'

* Add tests for 'poetry add --source'

* Merge master into develop (python-poetry#2070)

* Fix Github actions cache issues (python-poetry#1908)

* Fix case of `-f` flag

* Make it clearer what options to pass to `--format`

* fix (masonry.api): `get_requires_for_build_wheel` must return additional list of requirements for building a package, not listed in `pyproject.toml` and not dependencies for the package itself (python-poetry#1875)

fix (tests): adopted tests

* Lazy Keyring intialization for PasswordManager (python-poetry#1892)

* Fix Github Actions cache issues (python-poetry#1928)

* Avoid nested quantifiers with overlapping character space on git url parsing (python-poetry#1902 (python-poetry#1913)

* fix (git): match for `\w` instead of `.` for getting user

* change (vcs.git): hold pattern of the regex parts in a dictionary to be consistent over all regexs

* new (vcs.git): test for `parse_url` and some fixes for the regex pattern

* new (vcs.git): test for `parse_url` with string that should fail

* fix (test.vcs.git): make flake8 happy

* fix: correct parsing of wheel version with regex. (python-poetry#1932)

The previous regexp was only taking the first integer of the version number,
this presented problems when the major version number reached double digits.

Poetry would determine that the version of the dependency is '1', rather than,
ie: '14'. This caused failures to solve versions.

* Fix errors when using the --help option (python-poetry#1910)

* Fix how repository credentials are retrieved from env vars (python-poetry#1909)

# Conflicts:
#	poetry/utils/password_manager.py

* Fix downloading packages from Simplepypi (python-poetry#1851)

* fix downloading packages from simplepypi

* unused code removed

* remove unused imports

* Upgrade dependencies for the 1.0.3 release (python-poetry#1965)

* Bump version to 1.0.3 (python-poetry#1966)

* Fix non-compliant Git URL matching

RFC 3986 § 2.3 permits more characters in a URL than were matched. This
corrects that, though there may be other deficiencies. This was a
regression from v1.0.2, where at least “.” was matched without error.

* Update README.md "Updating Poetry"

Currently the note in "Updating Poetry" is different from the one below in "Enable tab completion for Bash, Fish, or Zsh". This MR is to make them more consistent.

* init: change dev dependency prompt

* Fix CI issues (python-poetry#2069)

Co-authored-by: brandonaut <brandon@hubermx.com>
Co-authored-by: finswimmer <finswimmer77@gmail.com>
Co-authored-by: Yannick PÉROUX <yannick.peroux@gmail.com>
Co-authored-by: Edward George <edwardgeorge@gmail.com>
Co-authored-by: Jan Škoda <skoda@jskoda.cz>
Co-authored-by: Andrew Marshall <andrew@johnandrewmarshall.com>
Co-authored-by: Andrew Selzer <andrewfselzer@gmail.com>
Co-authored-by: Andrii Maletskyi <andrii.maletskyi@gmail.com>

* pre-commit: replace isort mirror with isort upstream (python-poetry#2118)

The isort pre-commit mirror has been deprecated. This change updates
configuration to use the upstream package repository instead of the
mirror.

* Add cache list command (python-poetry#1187)

* Add poetry.locations.REPOSITORY_CACHE_DIR

The repository cache directory is used in multiple places in the
codebase. This change ensures that the value is reused.

* Add cache list command

This introduces a new cache sub-command that lists all available
caches.

Relates-to: python-poetry#1162

Co-authored-by: Tom Milligan <tommilligan@users.noreply.github.com>
Co-authored-by: David Cramer <dcramer@users.noreply.github.com>
Co-authored-by: finswimmer <finswimmer77@gmail.com>
Co-authored-by: Kyle Altendorf <sda@fstab.net>
Co-authored-by: Justin Mayer <entroP@gmail.com>
Co-authored-by: Yannick PÉROUX <yannick.peroux@gmail.com>
Co-authored-by: brandonaut <brandon@hubermx.com>
Co-authored-by: Edward George <edwardgeorge@gmail.com>
Co-authored-by: Jan Škoda <skoda@jskoda.cz>
Co-authored-by: Andrew Marshall <andrew@johnandrewmarshall.com>
Co-authored-by: Andrew Selzer <andrewfselzer@gmail.com>
Co-authored-by: Andrii Maletskyi <andrii.maletskyi@gmail.com>
Co-authored-by: Arun Babu Neelicattu <arun.neelicattu@gmail.com>
Configuring the template chooser to include discord community server.
@kasteph kasteph requested a review from sdispater March 27, 2020 13:18
@kasteph kasteph added the area/solver Related to the dependency resolver label Mar 27, 2020
sdispater and others added 17 commits March 27, 2020 20:10
If multiple distributions have `tests` as a top-level package, they'll conflict whenever both are installed. (Examples [here]((https://github.com/python-poetry/poetry/issues/1905) and [here](NixOS/nixpkgs#81482). Two common alternative strategies are:

1. not distributing tests (as [here](https://github.com/pypa/sampleproject)), or 
2. placing tests in a subdirectory of the main package, rather than adjacent (as [here](http://blog.habnab.it/blog/2013/07/21/python-packages-and-you/) and [here](http://as.ynchrono.us/2007/12/filesystem-structure-of-python-project_21.html)). Each of these strategies will avoid this issue. 

Users may fall into this trap because of the [package documentation](https://python-poetry.org/docs/pyproject/#packages) page, which gives an example:

```
packages = [
    { include = "my_package" },
    { include = "tests", format = "sdist" },
]
```

Having two top-level packages in a distribution is relatively unusual, but does have some use cases.  Using `"tests"` as a top-level package name in the example is likely to lead to conflicts, however. The alternate package in the documentation example could have a unique name like `"my_other_package"`, which would reduce the likelihood of this kind of overlap.
* Add fish shell tab completion for mac

* Remove macOS ref as brew should work linux as well
…poetry#1653)

Co-authored-by: finswimmer77@gmail.com <GiWahnsinn07!>
When testing the behavior for git packages with a setup.py file, the Provider class would create a temporary virtual environment and execute python setup.py egg_info. Both actions are costly and take time. By mocking them, we can reduce the tests time by a factor of 4 or 5.
finswimmer and others added 5 commits June 19, 2020 18:17
Keep empty in-project venv dir when recreating venv to allow for docker volumes
These small changes allow `poetry shell` to properly activate a python virtual environment when the user's shell is `tcsh`.
Link with 'www' was being redirected wrong to 'https://spdx.org//licenses/'
Adds ways to specify a package to poetry add --help section.
@finswimmer finswimmer added the kind/enhancement Not a bug or feature, but improves usability or performance label Jun 20, 2020
@finswimmer finswimmer requested a review from abn June 20, 2020 16:08
@alextriaca
Copy link

@finswimmer @abn is it possible to get this change green-lit? Some of our builds are now taking in excess of 20 minutes. A 40% improvement on that is now significant time. Especially when compared to pipenv as mentioned by @jacques- in #2094. Would be great if we could get a couple of these performance boosters in!

@abn
Copy link
Member

abn commented Jun 24, 2020

@alextriaca sorry this one fell through the gaps, we might need to port this to develop let me see if we intend to do another patch release before freezing master.

I'll get back to you this week. Feel free to poke at me if I do not.

@alextriaca
Copy link

Hey @abn did you possibly make any progress with this last week? If not here's that poke you asked for 😉

@abn
Copy link
Member

abn commented Jun 29, 2020

@alextriaca have not forgotten :) Thanks for the poke. We might release a new 1.0.x after the next preview release. No guarantees though. Would be great if these changes can be ported to both develop and poetry-core as well.

For this PR; please rebase on current master.

@alextriaca
Copy link

@abn rebased and ready to go.

@Secrus
Copy link
Member

Secrus commented May 21, 2022

@glitchedcat hi, sorry it fell through the gaps. Are you still interested in bringing this to Poetry?

@Secrus Secrus added the status/waiting-on-response Waiting on response from author label May 21, 2022
@Secrus
Copy link
Member

Secrus commented Jul 25, 2022

Closing in favor of #5805

@Secrus Secrus closed this Jul 25, 2022
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/solver Related to the dependency resolver kind/enhancement Not a bug or feature, but improves usability or performance status/waiting-on-response Waiting on response from author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet