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

#47689 improve run-speed of pip package state #47734

Merged
merged 12 commits into from Jul 31, 2018

Conversation

Projects
None yet
4 participants
@OrlandoArcapix
Contributor

OrlandoArcapix commented May 18, 2018

What does this PR do?

Only load the current package list once when checking multiple packages, rather than once for each package.

What issues does this PR fix or reference?

#47689

Previous Behavior

pip freeze is executed once for each package in a list when checking multiple pip packages.

New Behavior

pip freeze is only executed once, and the result is re-used for each package check.

Tests written?

No

Commits signed with GPG?

No

#47698 improve run-speed of pip package state checks by only loading …
…the current package list once when checking multiple packages
@gtmanfred

This comment has been minimized.

Contributor

gtmanfred commented May 18, 2018

There are some lint failures that will need to be cleaned up https://jenkins.saltstack.com/job/PR/job/salt-pr-docs-n/17344/violations/

@OrlandoArcapix

This comment has been minimized.

Contributor

OrlandoArcapix commented May 19, 2018

Lint errors sorted

@cachedout

Some docs, please

@@ -186,16 +186,19 @@ def _check_pkg_version_format(pkg):
def _check_if_installed(prefix, state_pkg_name, version_spec, ignore_installed,
force_reinstall, upgrade, user, cwd, bin_env, env_vars,
**kwargs):
pip_list=False, **kwargs):

This comment has been minimized.

@cachedout

cachedout May 21, 2018

Contributor

IMHO we need to have some documentation about what this kwarg does and the type of data expected here.

This comment has been minimized.

@OrlandoArcapix

OrlandoArcapix May 21, 2018

Contributor

I was surprised by that kwargs too! It wasn't in the release code I initially patched on my systems, but it was there when I pulled the 2017.7 branch to do the pull request against.

Looks like it came from here: 7c46d9d

This comment has been minimized.

@rallytime

rallytime May 25, 2018

Contributor

@OrlandoArcapix I think we just want some documentation for your new kwarg, pip_list. I know the terminology was a little confusing there. :)

@OrlandoArcapix

This comment has been minimized.

Contributor

OrlandoArcapix commented Jun 5, 2018

Ahh, I see @rallytime !

Not sure where to put that - the extra argument I've added is only used in an internal function, and it's arguments are not currently documented anywhere that I can see - can you point me to the right place to add the extra argument info?

def _check_if_installed(prefix, state_pkg_name, version_spec, ignore_installed,
                        force_reinstall, upgrade, user, cwd, bin_env, env_vars,
                        pip_list=False, **kwargs):
    # result: None means the command failed to run
    # result: True means the package is installed
    # result: False means the package is not installed
@rallytime

This comment has been minimized.

Contributor

rallytime commented Jun 5, 2018

@OrlandoArcapix Ah, you're right. I misread the code and thought it was an exposed variable, but it is not. I suppose it would be nice to have it documented somewhere maybe inline what the point of the variable is? I know none of the other kwargs are documented in _check_if_installed, but adding this would be a good place to start.

You can either do a doc string, or just a comment. The doc string won't populate the API documentation, but at least there will be something in the code to reference.

OrlandoArcapix added some commits Jun 15, 2018

@OrlandoArcapix

This comment has been minimized.

Contributor

OrlandoArcapix commented Jun 15, 2018

Docblock added with new kwarg described!

@rallytime

This comment has been minimized.

Contributor

rallytime commented Jun 15, 2018

@OrlandoArcapix

This comment has been minimized.

Contributor

OrlandoArcapix commented Jun 15, 2018

Oops - sorry! All sorted.

@rallytime

This comment has been minimized.

Contributor

rallytime commented Jun 18, 2018

@OrlandoArcapix This change is failing the following test:

  • integration.states.test_pip_state.PipStateTest.test_46127_pip_env_vars

https://jenkins.saltstack.com/job/PR/job/salt-pr-linode-ubuntu14-n/23706/

Can you take a look?

@rallytime

This comment has been minimized.

Contributor

rallytime commented Jul 6, 2018

Hi @OrlandoArcapix - Just a bump here. Were you able to take a look at that failing test?

@OrlandoArcapix

This comment has been minimized.

Contributor

OrlandoArcapix commented Jul 19, 2018

Hi @rallytime, sorry, I've not had much of a chance to dig into it. I had a brief look at first, and I thought it might just need a change to the string checks in the tests. I'd worry that there was something more fundamental though, in case the test really is flagging a bad behaviour - I'm not familiar with the pip module code to know for sure that my quick fix was sensible.

Having said that, I've been using it in production now for hundreds of system builds and it hasn't let me down yet!

I can have a shot at changing the test's string check if you think that's a good way to proceed - let me know.

@gtmanfred

This comment has been minimized.

Contributor

gtmanfred commented Jul 20, 2018

As long as the test still is valid, that should be fine to change the test string.

OrlandoArcapix added some commits Jul 29, 2018

@OrlandoArcapix

This comment has been minimized.

Contributor

OrlandoArcapix commented Jul 29, 2018

I've updated the test to match the string currently returned.

For what it's worth, I have been using it for a couple of months now, across a lot of different pip installation situations, and it's been working fine - but I wouldn't say no to someone with knowledge of the code workings doing a review of the change!

@rallytime rallytime merged commit c8e6943 into saltstack:2017.7 Jul 31, 2018

5 of 8 checks passed

continuous-integration/jenkins/pr-merge This commit cannot be built
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has failed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has failed
Details
WIP ready for review
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint The lint job has passed
Details
jenkins/pr/py2-centos-7 The py2-centos-7 job has passed
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment