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

Do wheel installs from VCS/directories using ephemeral caching #4764

Merged
merged 36 commits into from
Jan 23, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
c624090
Try to build wheels for directories/VCS
takluyver May 19, 2017
a32ebea
Add news file
takluyver May 19, 2017
fe5e9cc
Fix list comp
takluyver May 19, 2017
30109c3
Code style fixes
takluyver May 19, 2017
ee67867
Allow temp files for another test
takluyver May 19, 2017
46c2c54
Fix cleanup on 'pip install' with no args
takluyver May 19, 2017
9fdcfaf
Cleanup wheel cache after 'pip freeze'
takluyver May 19, 2017
9109322
Expect temp files in another test
takluyver May 19, 2017
ad242d1
Update test of install output
takluyver May 19, 2017
a51843a
Expect temp files on another test
takluyver May 19, 2017
73440ed
Update another install output test
takluyver May 19, 2017
5610636
Update test to check for dist-info
takluyver May 19, 2017
1c03c1b
Remove string formatting operation
takluyver May 19, 2017
ad50ac9
Install 'wheel' for test that now requires it
takluyver May 20, 2017
d045cc2
Fix some changes that I screwed up in the rebase
alex Jun 22, 2017
01d226b
Switch to using the existing tempdir abstraction
alex Jun 26, 2017
d5e3082
flake8
alex Jun 26, 2017
1997456
fix
alex Jul 6, 2017
328f607
not a thing anymore
alex Jul 6, 2017
8ef8400
these are probably needed
alex Jul 6, 2017
cb56bb3
flake8
alex Jul 6, 2017
5f88252
fix
alex Jul 6, 2017
ebf06ce
Merge master
pradyunsg Oct 4, 2017
901f529
Fix test that broke in the merge
pradyunsg Oct 4, 2017
b96618b
Revert unrelated change
pradyunsg Oct 4, 2017
165b10b
Rework Wheel Caching
pradyunsg Oct 4, 2017
16cda30
Cleanup and rename test
pradyunsg Oct 4, 2017
1c6a450
Merge branch 'master' of github.com:pypa/pip into cache/ephem-wheel-c…
pradyunsg Oct 20, 2017
3ec643c
Fix message printing for local directories
pradyunsg Oct 20, 2017
d4e2d7e
Merge branch 'master' into cache/ephem-wheel-cache
pradyunsg Oct 22, 2017
e36bb6d
Merge branch 'master' into cache/ephem-wheel-cache
pradyunsg Oct 24, 2017
a39f36a
:art:
pradyunsg Oct 25, 2017
01d97e7
Merge branch 'master' into cache/ephem-wheel-cache
pradyunsg Nov 17, 2017
cd14240
Merge branch 'master' into cache/ephem-wheel-cache
pradyunsg Dec 1, 2017
9b6f1d7
Merge branch 'master' into cache/ephem-wheel-cache
pradyunsg Dec 1, 2017
4c14739
Merge branch 'master' into cache/ephem-wheel-cache
pradyunsg Jan 23, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions news/4501.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Installing from a local directory or a VCS URL now builds a wheel to install,
rather than running ``setup.py install``. Wheels from these sources are not
cached.
60 changes: 56 additions & 4 deletions src/pip/_internal/cache.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
from pip._internal import index
from pip._internal.compat import expanduser
from pip._internal.download import path_to_url
from pip._internal.utils.temp_dir import TempDirectory
from pip._internal.wheel import InvalidWheelFilename, Wheel

logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -102,13 +103,18 @@ def _link_for_candidate(self, link, candidate):

return index.Link(path_to_url(path))

def cleanup(self):
Copy link
Member Author

@pradyunsg pradyunsg Oct 4, 2017

Choose a reason for hiding this comment

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

I should note that @xoviat had proposed in #4714 to make Caches into context managers, to avoid using this cleanup() pattern.

I'm still confused about how I feel about that. FWIW, we can do that in a later PR. I have some other ideas too but I'd prefer to experiment with that after this PR is done.

Copy link

Choose a reason for hiding this comment

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

@rhettinger What do you think about using context managers for cleanup? You're the person that I got this from.

Copy link
Member

Choose a reason for hiding this comment

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

Just to be clear, I definitely think using a context manager is a good idea. But I agree with @pradyunsg that it's something we should defer until a followup PR. Let's get the functionality landed then address the cleanup.

Copy link

Choose a reason for hiding this comment

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

I agree with that; I was just wondering what others thought in principle.

pass

class WheelCache(Cache):

class SimpleWheelCache(Cache):
"""A cache of wheels for future installs.
"""

def __init__(self, cache_dir, format_control):
super(WheelCache, self).__init__(cache_dir, format_control, {"binary"})
super(SimpleWheelCache, self).__init__(
cache_dir, format_control, {"binary"}
)

def get_path_for_link(self, link):
"""Return a directory to store cached wheels for link
Expand All @@ -127,8 +133,7 @@ def get_path_for_link(self, link):
"""
parts = self._get_cache_path_parts(link)

# Inside of the base location for cached wheels, expand our parts and
# join them all together.
# Store wheels within the root cache_dir
return os.path.join(self.cache_dir, "wheels", *parts)

def get(self, link, package_name):
Expand All @@ -148,3 +153,50 @@ def get(self, link, package_name):
return link

return self._link_for_candidate(link, min(candidates)[1])


class EphemWheelCache(SimpleWheelCache):
"""A SimpleWheelCache that creates it's own temporary cache directory
"""

def __init__(self, format_control):
self._temp_dir = TempDirectory(kind="ephem-wheel-cache")
self._temp_dir.create()

super(EphemWheelCache, self).__init__(
self._temp_dir.path, format_control
)

def cleanup(self):
self._temp_dir.cleanup()


class WheelCache(Cache):
"""Wraps EphemWheelCache and SimpleWheelCache into a single Cache

This Cache allows for gracefully degradation, using the ephem wheel cache
when a certain link is not found in the simple wheel cache first.
"""

def __init__(self, cache_dir, format_control):
super(WheelCache, self).__init__(
cache_dir, format_control, {'binary'}
)
self._wheel_cache = SimpleWheelCache(cache_dir, format_control)
self._ephem_cache = EphemWheelCache(format_control)
Copy link
Member

Choose a reason for hiding this comment

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

Would it be worth building the ephemeral wheel cache lazily? The majority of installs won't be installing from git or a local directory, so why waste time creating and deleting a temporary directory that we never use?

This may be a premature optimisation, of course. Feel free to take the view that we can do this later if it proves to be a problem.

Copy link
Member Author

Choose a reason for hiding this comment

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

$ python -c "
import time
import pip._internal.utils.temp_dir as tmp
t = tmp.TempDirectory()
print(time.time())
t.create()
t.cleanup()
print(time.time())"
1507128804.431749
1507128804.43445

At about 0.003 seconds, I don't think it's a major issue. Even if it's 10x as costly, I'm tempted to say it's not worth it to optimise.

Copy link
Member

Choose a reason for hiding this comment

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

OK, agreed. Thanks for testing this. I was concerned that Windows might be slower (not an uncommon problem...) but I ran your test script on my machine, and it's fine. I'm convinced - leave it as it is.


def get_path_for_link(self, link):
return self._wheel_cache.get_path_for_link(link)

def get_ephem_path_for_link(self, link):
return self._ephem_cache.get_path_for_link(link)

def get(self, link, package_name):
retval = self._wheel_cache.get(link, package_name)
if retval is link:
retval = self._ephem_cache.get(link, package_name)
return retval

def cleanup(self):
self._wheel_cache.cleanup()
self._ephem_cache.cleanup()
7 changes: 5 additions & 2 deletions src/pip/_internal/commands/freeze.py
Original file line number Diff line number Diff line change
Expand Up @@ -89,5 +89,8 @@ def run(self, options, args):
exclude_editable=options.exclude_editable,
)

for line in freeze(**freeze_kwargs):
sys.stdout.write(line + '\n')
try:
for line in freeze(**freeze_kwargs):
sys.stdout.write(line + '\n')
finally:
wheel_cache.cleanup()
Copy link
Member

Choose a reason for hiding this comment

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

Style nit. I'm not very keen on the fact that the try...finally is separated from the creation of the cache. Clearly there's no way the intermediate lines could fail, so in practice it's not a problem, but it looks odd (I went and looked at the source code to verify that the cleanup was in the right place).

This is of course one place where caches being context managers would make things clearer - so I'm OK with leaving this for tidying up when we look at doing that (and I agree with your comment that making that change should be a separate PR).

26 changes: 14 additions & 12 deletions src/pip/_internal/commands/install.py
Original file line number Diff line number Diff line change
Expand Up @@ -224,10 +224,10 @@ def run(self, options, args):
global_options = options.global_options or []

with self._build_session(options) as session:

finder = self._build_package_finder(options, session)
build_delete = (not (options.no_clean or options.build_dir))
wheel_cache = WheelCache(options.cache_dir, options.format_control)

if options.cache_dir and not check_path_owner(options.cache_dir):
logger.warning(
"The directory '%s' or its parent directory is not owned "
Expand All @@ -249,18 +249,19 @@ def run(self, options, args):
use_user_site=options.use_user_site,
)

self.populate_requirement_set(
requirement_set, args, options, finder, session, self.name,
wheel_cache
)
preparer = RequirementPreparer(
build_dir=directory.path,
src_dir=options.src_dir,
download_dir=None,
wheel_download_dir=None,
progress_bar=options.progress_bar,
)
try:
self.populate_requirement_set(
requirement_set, args, options, finder, session,
self.name, wheel_cache
)
preparer = RequirementPreparer(
build_dir=directory.path,
src_dir=options.src_dir,
download_dir=None,
wheel_download_dir=None,
progress_bar=options.progress_bar,
)

resolver = Resolver(
preparer=preparer,
finder=finder,
Expand Down Expand Up @@ -349,6 +350,7 @@ def run(self, options, args):
# Clean up
if not options.no_clean:
requirement_set.cleanup_files()
wheel_cache.cleanup()

if options.target_dir:
self._handle_target_dir(
Expand Down
53 changes: 27 additions & 26 deletions src/pip/_internal/commands/wheel.py
Original file line number Diff line number Diff line change
Expand Up @@ -146,35 +146,35 @@ def run(self, options, args):
require_hashes=options.require_hashes,
)

self.populate_requirement_set(
requirement_set, args, options, finder, session, self.name,
wheel_cache
)
try:
self.populate_requirement_set(
requirement_set, args, options, finder, session,
self.name, wheel_cache
)

preparer = RequirementPreparer(
build_dir=directory.path,
src_dir=options.src_dir,
download_dir=None,
wheel_download_dir=options.wheel_dir,
progress_bar=options.progress_bar,
)
preparer = RequirementPreparer(
build_dir=directory.path,
src_dir=options.src_dir,
download_dir=None,
wheel_download_dir=options.wheel_dir,
progress_bar=options.progress_bar,
)

resolver = Resolver(
preparer=preparer,
finder=finder,
session=session,
wheel_cache=wheel_cache,
use_user_site=False,
upgrade_strategy="to-satisfy-only",
force_reinstall=False,
ignore_dependencies=options.ignore_dependencies,
ignore_requires_python=options.ignore_requires_python,
ignore_installed=True,
isolated=options.isolated_mode,
)
resolver.resolve(requirement_set)
resolver = Resolver(
preparer=preparer,
finder=finder,
session=session,
wheel_cache=wheel_cache,
use_user_site=False,
upgrade_strategy="to-satisfy-only",
force_reinstall=False,
ignore_dependencies=options.ignore_dependencies,
ignore_requires_python=options.ignore_requires_python,
ignore_installed=True,
isolated=options.isolated_mode,
)
resolver.resolve(requirement_set)

try:
# build wheels
wb = WheelBuilder(
finder, preparer, wheel_cache,
Expand All @@ -195,3 +195,4 @@ def run(self, options, args):
finally:
if not options.no_clean:
requirement_set.cleanup_files()
wheel_cache.cleanup()
4 changes: 3 additions & 1 deletion src/pip/_internal/resolve.py
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,9 @@ def _get_abstract_dist_for(self, req):
if req.satisfied_by:
should_modify = (
self.upgrade_strategy != "to-satisfy-only" or
self.force_reinstall or self.ignore_installed
self.force_reinstall or
self.ignore_installed or
req.link.scheme == 'file'
)
if should_modify:
self._set_req_to_reinstall(req)
Expand Down
21 changes: 13 additions & 8 deletions src/pip/_internal/wheel.py
Original file line number Diff line number Diff line change
Expand Up @@ -806,6 +806,7 @@ def build(self, requirements, session, autobuilding=False):

buildset = []
for req in requirements:
ephem_cache = False
if req.constraint:
continue
if req.is_wheel:
Expand All @@ -816,17 +817,17 @@ def build(self, requirements, session, autobuilding=False):
elif autobuilding and req.editable:
pass
elif autobuilding and req.link and not req.link.is_artifact:
pass
# VCS checkout. Build wheel just for this run.
ephem_cache = True
elif autobuilding and not req.source_dir:
pass
else:
if autobuilding:
link = req.link
base, ext = link.splitext()
if index.egg_info_matches(base, None, link) is None:
# Doesn't look like a package - don't autobuild a wheel
# because we'll have no way to lookup the result sanely
continue
# E.g. local directory. Build wheel just for this run.
ephem_cache = True
if "binary" not in index.fmt_ctl_formats(
self.finder.format_control,
canonicalize_name(req.name)):
Expand All @@ -835,23 +836,27 @@ def build(self, requirements, session, autobuilding=False):
"being disabled for it.", req.name,
)
continue
buildset.append(req)
buildset.append((req, ephem_cache))

if not buildset:
return True

# Build the wheels.
logger.info(
'Building wheels for collected packages: %s',
', '.join([req.name for req in buildset]),
', '.join([req.name for (req, _) in buildset]),
)
_cache = self.wheel_cache # shorter name
with indent_log():
build_success, build_failure = [], []
for req in buildset:
for req, ephem in buildset:
python_tag = None
if autobuilding:
python_tag = pep425tags.implementation_tag
output_dir = self.wheel_cache.get_path_for_link(req.link)
if ephem:
output_dir = _cache.get_ephem_path_for_link(req.link)
else:
output_dir = _cache.get_path_for_link(req.link)
try:
ensure_dir(output_dir)
except OSError as e:
Expand Down
20 changes: 9 additions & 11 deletions tests/functional/test_install.py
Original file line number Diff line number Diff line change
Expand Up @@ -994,16 +994,15 @@ def test_install_builds_wheels(script, data, common_wheels):
# and built wheels for upper and wheelbroken
assert "Running setup.py bdist_wheel for upper" in str(res), str(res)
assert "Running setup.py bdist_wheel for wheelb" in str(res), str(res)
# But not requires_wheel... which is a local dir and thus uncachable.
assert "Running setup.py bdist_wheel for requir" not in str(res), str(res)
# Wheels are built for local directories, but not cached.
assert "Running setup.py bdist_wheel for requir" in str(res), str(res)
# wheelbroken has to run install
# into the cache
assert wheels != [], str(res)
# and installed from the wheel
assert "Running setup.py install for upper" not in str(res), str(res)
# the local tree can't build a wheel (because we can't assume that every
# build will have a suitable unique key to cache on).
assert "Running setup.py install for requires-wheel" in str(res), str(res)
# Wheels are built for local directories, but not cached.
assert "Running setup.py install for requir" not in str(res), str(res)
# wheelbroken has to run install
assert "Running setup.py install for wheelb" in str(res), str(res)
# We want to make sure we used the correct implementation tag
Expand All @@ -1027,13 +1026,12 @@ def test_install_no_binary_disables_building_wheels(
assert expected in str(res), str(res)
# and built wheels for wheelbroken only
assert "Running setup.py bdist_wheel for wheelb" in str(res), str(res)
# But not requires_wheel... which is a local dir and thus uncachable.
assert "Running setup.py bdist_wheel for requir" not in str(res), str(res)
# Nor upper, which was blacklisted
# Wheels are built for local directories, but not cached across runs
assert "Running setup.py bdist_wheel for requir" in str(res), str(res)
# Don't build wheel for upper which was blacklisted
assert "Running setup.py bdist_wheel for upper" not in str(res), str(res)
# the local tree can't build a wheel (because we can't assume that every
# build will have a suitable unique key to cache on).
assert "Running setup.py install for requires-wheel" in str(res), str(res)
# Wheels are built for local directories, but not cached across runs
assert "Running setup.py install for requir" not in str(res), str(res)
# And these two fell back to sdist based installed.
assert "Running setup.py install for wheelb" in str(res), str(res)
assert "Running setup.py install for upper" in str(res), str(res)
Expand Down
4 changes: 2 additions & 2 deletions tests/functional/test_install_cleanup.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ def test_no_clean_option_blocks_cleaning_after_install(script, data):
build = script.base_path / 'pip-build'
script.pip(
'install', '--no-clean', '--no-index', '--build', build,
'--find-links=%s' % data.find_links, 'simple',
'--find-links=%s' % data.find_links, 'simple', expect_temp=True,
)
assert exists(build)

Expand Down Expand Up @@ -134,7 +134,7 @@ def test_cleanup_prevented_upon_build_dir_exception(script, data):
result = script.pip(
'install', '-f', data.find_links, '--no-index', 'simple',
'--build', build,
expect_error=True,
expect_error=True, expect_temp=True,
)

assert result.returncode == PREVIOUS_BUILD_DIR_ERROR
Expand Down
15 changes: 10 additions & 5 deletions tests/functional/test_install_user.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,24 +68,29 @@ def test_install_subversion_usersite_editable_with_distribute(
)
result.assert_installed('INITools', use_user_site=True)

def test_install_curdir_usersite(self, script, virtualenv, data):
@pytest.mark.network
def test_install_from_current_directory_into_usersite(
self, script, virtualenv, data, common_wheels):
"""
Test installing current directory ('.') into usersite
"""
virtualenv.system_site_packages = True
script.pip("install", "wheel", '--no-index', '-f', common_wheels)

run_from = data.packages.join("FSPkg")
result = script.pip(
'install', '-vvv', '--user', curdir,
cwd=run_from,
expect_error=False,
)

fspkg_folder = script.user_site / 'fspkg'
egg_info_folder = (
script.user_site / 'FSPkg-0.1.dev0-py%s.egg-info' % pyversion
)
assert fspkg_folder in result.files_created, result.stdout

assert egg_info_folder in result.files_created
dist_info_folder = (
script.user_site / 'FSPkg-0.1.dev0.dist-info'
)
assert dist_info_folder in result.files_created

def test_install_user_venv_nositepkgs_fails(self, script, data):
"""
Expand Down
2 changes: 1 addition & 1 deletion tests/functional/test_wheel.py
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ def test_pip_wheel_fail_cause_of_previous_build_dir(
result = script.pip(
'wheel', '--no-index', '--find-links=%s' % data.find_links,
'--build', script.venv_path / 'build',
'simple==3.0', expect_error=True,
'simple==3.0', expect_error=True, expect_temp=True,
)

# Then I see that the error code is the right one
Expand Down