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

Add --python-executable option for pip-sync #1333

Merged
merged 38 commits into from
Jun 13, 2021

Conversation

MaratFM
Copy link
Contributor

@MaratFM MaratFM commented Feb 27, 2021

Problem: build pipelines and infrastructure at big companies often targets multiple independent python projects. Currently, in order to use pip-sync, pip-tools have to be installed in every environment. When pip-compile does not have such restriction.

Solution: add the ability to pass custom python executable. When the option provided, list of installed packages will be collected from the target environment and all install/uninstall commands will be executed inside that environment.

This PR implements a proof concept for collecting early feedback on the idea and implementation. If concept and task will be accepted, sufficient test coverage will be added and PR re-submitted.

Thank you!

Added "--python-executable" option for pip-sync command. It allows targeting environments other than current:

Contributor checklist
  • Provided the tests for the changes.
  • Gave a clear one-line description in the PR (that the maintainers can add to CHANGELOG.md on release).
  • Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

@MaratFM MaratFM changed the title Support custom python-executable [WIP] Support custom python-executable Feb 27, 2021
@codecov

This comment has been minimized.

@MaratFM MaratFM marked this pull request as draft February 27, 2021 21:52
@MaratFM
Copy link
Contributor Author

MaratFM commented Mar 3, 2021

@atugushev could you please comment if this PR makes sense and maintainers agree to add such an option

@atugushev
Copy link
Member

Hello @MaratFM,

Tganks for pinging! I believe that this functionality would help for such issues as #1087. So i'm 👍 on this.

@MaratFM MaratFM changed the title [WIP] Support custom python-executable Support custom python-executable Mar 16, 2021
@MaratFM MaratFM marked this pull request as ready for review March 16, 2021 18:00
piptools/scripts/sync.py Outdated Show resolved Hide resolved
Copy link
Member

@atugushev atugushev left a comment

Choose a reason for hiding this comment

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

Generally, it looks good. One thing to consider: could we improve --python-executable to pass binary name somehow? For example: pip-sync --python-executable=python3.

@MaratFM
Copy link
Contributor Author

MaratFM commented Mar 18, 2021

Generally, it looks good. One thing to consider: could we improve --python-executable to pass binary name somehow? For example: pip-sync --python-executable=python3.

Great idea, I can try using shutil.which for resolving
https://docs.python.org/3/library/shutil.html?highlight=.which#shutil.which

Looks like it works well for aliases and full paths, so we can always use it for resolving:

In [1]: import shutil

In [2]: import sys

In [3]: shutil.which('python')
Out[3]: 'D:\\Envs\\test\\python.EXE'

In [4]: shutil.which(sys.executable)
Out[4]: 'd:\\envs\\test\\python.exe'

Copy link
Member

@atugushev atugushev left a comment

Choose a reason for hiding this comment

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

I've caught a trace when a given python executable is not found:

$ pip-sync -a --python-executable=python3.9
Traceback (most recent call last):
  File "/Users/albert/Projects/pip-tools/.venv/bin/pip-sync", line 11, in <module>
    load_entry_point('pip-tools', 'console_scripts', 'pip-sync')()
  File "/Users/albert/Projects/pip-tools/.venv/lib/python3.8/site-packages/click/core.py", line 764, in __call__
    return self.main(*args, **kwargs)
  File "/Users/albert/Projects/pip-tools/.venv/lib/python3.8/site-packages/click/core.py", line 717, in main
    rv = self.invoke(ctx)
  File "/Users/albert/Projects/pip-tools/.venv/lib/python3.8/site-packages/click/core.py", line 956, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/Users/albert/Projects/pip-tools/.venv/lib/python3.8/site-packages/click/core.py", line 555, in invoke
    return callback(*args, **kwargs)
  File "/Users/albert/Projects/pip-tools/piptools/scripts/sync.py", line 126, in cli
    _validate_python_executable(python_executable)
  File "/Users/albert/Projects/pip-tools/piptools/scripts/sync.py", line 200, in _validate_python_executable
    log.error(msg, python_executable, pip_version, required_pip_specification)
  File "/Users/albert/Projects/pip-tools/piptools/logging.py", line 40, in error
    self.log(message, *args, **kwargs)
  File "/Users/albert/Projects/pip-tools/piptools/logging.py", line 24, in log
    click.secho(prefix + message, *args, **kwargs)
TypeError: secho() got multiple values for argument 'err'

@MaratFM
Copy link
Contributor Author

MaratFM commented Jun 9, 2021

My bad, did not re-run tests locally (on a side note tox -p all never produced all green result for me, even on master failing with coverage access denied errors).

I reverted logging message passing, was confused by the recommendation to delegate message formatting to the logging system (#1333 (comment)). Seems like pip-tools uses a custom logging wrapper that has incompatible method contracts.

@atugushev
Copy link
Member

I reverted logging message passing, was confused by the recommendation to delegate message formatting to the logging system (#1333 (comment)). Seems like pip-tools uses a custom logging wrapper that has incompatible method contracts.

Thanks @MaratFM! Let's address that in a following up PR.


@mock.patch("piptools.scripts.sync.get_pip_version_for_python_executable")
@mock.patch("piptools.scripts.sync.get_sys_path_for_python_executable")
@mock.patch("piptools.scripts.sync.get_installed_distributions")
Copy link
Member

Choose a reason for hiding this comment

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

There are too much of monkeypatches. Ideally, there should be only @mock.patch("piptools.sync.run").

Copy link
Contributor Author

Choose a reason for hiding this comment

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

refactored

piptools/utils.py Outdated Show resolved Hide resolved
@MaratFM MaratFM requested a review from atugushev June 10, 2021 22:15
piptools/utils.py Outdated Show resolved Hide resolved
MaratFM and others added 2 commits June 10, 2021 21:28
@MaratFM MaratFM requested a review from atugushev June 11, 2021 22:21
@MaratFM
Copy link
Contributor Author

MaratFM commented Jun 11, 2021

@atugushev seems like with "# pragma: no branch" it stills shows partial coverage, so I reverted back to "# pragma: no cover"

piptools/utils.py Outdated Show resolved Hide resolved
@atugushev atugushev closed this Jun 12, 2021
@atugushev atugushev reopened this Jun 12, 2021
@MaratFM
Copy link
Contributor Author

MaratFM commented Jun 12, 2021

@atugushev sorry for changing tests back and forth, I could not run tests for all these platforms locally and have to wait when someone will trigger the GitHub actions pipeline

Copy link
Member

@atugushev atugushev left a comment

Choose a reason for hiding this comment

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

Thanks @MaratFM for your patience. Awesome work! 👍🏻

@atugushev atugushev added enhancement Improvements to functionality cli Related to command line interface things labels Jun 13, 2021
@atugushev atugushev added this to the 6.2.0 milestone Jun 13, 2021
@atugushev atugushev merged commit b40eb52 into jazzband:master Jun 13, 2021
@atugushev atugushev changed the title Support custom python-executable Add --python-executable option for pip-sync Jun 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Related to command line interface things enhancement Improvements to functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants