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

try to build wheels for directories/vcs #4714

Closed
wants to merge 30 commits into from
Closed

try to build wheels for directories/vcs #4714

wants to merge 30 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Sep 7, 2017

I think that the rebase is OK, but let me know whether I made any errors.


Closes #4562.

takluyver and others added 24 commits September 7, 2017 13:06
Follow up from gh-4144

To allow build system abstractions, we want installation to go through
wheels in more cases. In particular, installing packages from a local
directory or a VCS URL currently runs 'setup.py install'. The aim of
this PR is to have it build a wheel, which is stored in an ephemeral
cache directory, used for installation, and then discarded.

We can't cache it permanently based on the path/URL, because the code
there might change, but our cache wouldn't be invalidated.
When there's a previous build directory, no_clean is set
@pfmoore
Copy link
Member

pfmoore commented Sep 7, 2017

Is this a rebase of a different PR? Can you at least provide a link back to the original?

@ghost
Copy link
Author

ghost commented Sep 7, 2017

#4562

@pfmoore
Copy link
Member

pfmoore commented Sep 7, 2017

Ah, OK. That's someone else's PR, and it appears to be getting worked on by them. Not sure why you've picked this up, but a comment on the original PR by the author confirming that you've taken over the PR would be useful, if that's what's happened.

@ghost
Copy link
Author

ghost commented Sep 7, 2017

The last comment from the author was:

@pradyunsg if you'd like to take over this PR, that'd be great.

I'm actually the third person to take over this PR. If you don't feel that my actions were appropriate given the circumstances then feel free to close this.

@pfmoore
Copy link
Member

pfmoore commented Sep 7, 2017

I assume from this that @pradyunsg is working on it (or maybe plans on doing so). Anyway, I understand the situation now, which is all I wanted - thanks for clarifying.

@pradyunsg
Copy link
Member

pradyunsg commented Sep 8, 2017

@pfmoore I had actually started working on it. Then, I think I discussed it with @dstufft who said that this something along the lines of this should probably be lower priority than the resolver for me. So, I moved it down my todo list.

I got swamped with a lot of stuff in the past few weeks and have had basically much less free time.

I won't mind if someone else does take the PR forward - it's not exactly a trivial PR and it affects a critical code path, so I wanna be a little careful on that.

IIRC, there's some discussion to be had on it too.

(edit: fleshed out sentence)

@@ -117,7 +117,7 @@ class WheelCache(Cache):
def __init__(self, cache_dir, format_control):
super(WheelCache, self).__init__(cache_dir, format_control, {"binary"})

def get_path_for_link(self, link):
def get_path_for_link(self, link, ephem=False):
Copy link
Member

Choose a reason for hiding this comment

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

Instead of having a boolean in the signature, I'd suggest you implement a new method WheelCache.get_ephem_path_for_link.

Relevant Blog Post: https://martinfowler.com/bliki/FlagArgument.html

@@ -0,0 +1,3 @@
Installing from a local directory or a VCS URL now builds a wheel to install,
Copy link
Member

Choose a reason for hiding this comment

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

Could you rephrase this to:

pip now builds a wheel when installing from a local directory or a VCS URL. Wheels from these sources are not cached.

?

@@ -741,6 +741,7 @@ def build(self, session, autobuilding=False):

buildset = []
for req in reqset:
ephem_cache = False
Copy link
Member

Choose a reason for hiding this comment

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

bike-shedding: I'll suggest painting this as use_ephem_cache; I won't push if anyone disagrees.

def test_install_curdir_usersite(self, script, virtualenv, data):
"""
Test installing current directory ('.') into usersite
"""
virtualenv.system_site_packages = True
script.pip("install", "wheel")
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure this is actually needed?

Copy link
Author

Choose a reason for hiding this comment

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

I have not validated this myself but from experience with related tests, I would strongly suspect that it is.

def test_install_curdir_usersite(self, script, virtualenv, data):
"""
Test installing current directory ('.') into usersite
"""
virtualenv.system_site_packages = True
script.pip("install", "wheel")
Copy link
Member

Choose a reason for hiding this comment

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

nit: use the common_wheels fixture.

@@ -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,
Copy link
Member

Choose a reason for hiding this comment

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

The cache should be cleaning up after itself even if this happens?

Copy link
Author

Choose a reason for hiding this comment

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

Ditto here for what I said below.

Copy link
Member

Choose a reason for hiding this comment

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

Ditto here for what I said below. ;)

@@ -132,7 +132,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,
Copy link
Member

Choose a reason for hiding this comment

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

The cache should be cleaning up after itself even if this happens?

Copy link
Author

Choose a reason for hiding this comment

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

@takluyver Can you explain this?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I can't remember the details. At some point some tests failed, and that seemed to be the right thing to make them work.

Copy link
Author

Choose a reason for hiding this comment

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

Perhaps the PR does not clean up the cache as advertised? What is the affect of this parameter?

Copy link
Member

Choose a reason for hiding this comment

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

AFAICT, this parameter means that the script run is expected to leave some temporary files, which is not what we want.

Copy link
Author

Choose a reason for hiding this comment

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

What we can do it turn the following line into a with statement and set the wheelcache as a context manager:

wheel_cache = WheelCache(options.cache_dir, options.format_control)

Is there any other location where the wheelcache is initialized?

Copy link
Member

Choose a reason for hiding this comment

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

@xoviat -- a simple grep tells me there are 6 places.

Copy link
Author

Choose a reason for hiding this comment

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

@pradyunsg So this test is named cleanup_prevented_on_build_dir_exception, and if you look at the control flow on install, it makes sense why the directory would not cleaned up. I attempted to fix that with the contextmanager, but your proposed solution necessitates that temporary files will be created here.

Copy link
Member

Choose a reason for hiding this comment

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

Cool. That means that this change in tests corresponds to a change in the expected behaviour. :)

PS: 41b83f8 was not the change I was suggesting. I thought there was no call to cleanup in finally.

@@ -30,7 +30,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,
Copy link
Member

Choose a reason for hiding this comment

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

The cache should be cleaning up after itself even if this happens?

Copy link
Author

Choose a reason for hiding this comment

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

This was controlled by the no_clean option but I have disabled that behavior.

@@ -16,9 +16,6 @@ def __init__(self,
require_hashes=False, target_dir=None, use_user_site=False,
pycompile=True):
"""Create a RequirementSet.

Copy link
Member

Choose a reason for hiding this comment

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

This is an unrelated (albeit obvious) change. It'd be nice if you revert this hunk and make a new PR for it.

Copy link
Author

Choose a reason for hiding this comment

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

This was 396abfe done by @alex. I would need to get his consent before removing it.

Copy link
Member

Choose a reason for hiding this comment

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

Uhhh, no clue, feel free to ignore whatever I did.

Copy link
Author

Choose a reason for hiding this comment

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

I will remove this then.

@ghost
Copy link
Author

ghost commented Sep 8, 2017

So I've addressed some of the concerns. Unfortunately travis seems to be having problems so I'm not going to proceed further until I can get tests passing.

with self._build_session(options) as session:
with self._build_session(options) as session, \
WheelCache(options.cache_dir, options.format_control) as \
wheel_cache:
Copy link
Author

Choose a reason for hiding this comment

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

This looks super ugly but it is the best that I could come up with. If anyone else has any better ideas, I am open to them.

Copy link
Member

Choose a reason for hiding this comment

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

Why not put a cleanup call in the finally of the try clause starting below at line 261 or 251?

I don't see why this needs to be a context manager. It doesn't make sense to me.

Copy link
Author

Choose a reason for hiding this comment

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

It needs to be a context manager because the files were not cleaned up correctly before, as you pointed out.

Copy link
Member

Choose a reason for hiding this comment

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

needs to be a context manager

I think just calling the cleanup method in the existing finally clause should be just fine.

I don't want to add one-more-way to use a WheelCache, especially since this behaviour is not really a thing in Cache.

@pradyunsg
Copy link
Member

pradyunsg commented Sep 9, 2017

@xoviat -- please revert 41b83f8.

I'm down with a headache right now. I'll get back to this at a later time.

@ghost
Copy link
Author

ghost commented Sep 9, 2017

@pradyunsg The contextmanager idea came directly from here. I will drop the three commits that address the cleanup issues.

@ghost
Copy link
Author

ghost commented Sep 9, 2017

I hope your headache gets better.

@pradyunsg
Copy link
Member

STATUS: BLOCKED UNTIL #4715 IS ADDRESSED

@xoviat - you can cherry-pick the fix from #4719.

@ghost
Copy link
Author

ghost commented Sep 15, 2017

I'm not going to play these games. Master should always pass, and if it doesn't it should be fixed as soon as possible. Maybe Paul has time to review #4719?

@ghost ghost closed this Sep 15, 2017
@ghost ghost reopened this Sep 15, 2017
@ghost
Copy link
Author

ghost commented Sep 15, 2017 via email

@pradyunsg
Copy link
Member

I haven't looked at this since the last time. I'll try to find time to do it sometime next week but I genuinely don't know.

@@ -102,6 +106,10 @@ def _link_for_candidate(self, link, candidate):

return index.Link(path_to_url(path))

def cleanup(self):
Copy link
Member

Choose a reason for hiding this comment

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

This should be on WheelCache. Also, since it only clears the ephemeral cache, I'd suggest renaming this to cleanup_ephem.

Copy link
Author

Choose a reason for hiding this comment

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

After thinking about this some more, I don't agree. I think that first of all, cleanup methods have very little support in the Python ecosystem. Second of all, when there is a cleanup method, it should be defined on the base class so that it's always called after using the object and specific implementations should go on derived classes.

Copy link
Author

Choose a reason for hiding this comment

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

So, specifically, I propose making this pass, and then moving the guts to WheelCache.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed.

@@ -131,6 +139,18 @@ def get_path_for_link(self, link):
# join them all together.
return os.path.join(self.cache_dir, "wheels", *parts)

def get_ephem_path_for_link(self, link):
"""Return a driectory to store cached wheels for link
Copy link
Member

Choose a reason for hiding this comment

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

Typo: directory

@pradyunsg
Copy link
Member

There's probably more; this is all I could look at in the 5 minutes I had. 🤷‍♂️

@pradyunsg
Copy link
Member

Also worth noting, I'm on the fence about adding the ephemeral cache stuff to WheelCache. Maybe adding an EphemeralWheelCache that inherits from WheelCache would be cleaner? Idk myself.

API design is hard. This looks about okay to me.

Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

Oh well. I forgot to submit this review. :/

@@ -102,6 +106,10 @@ def _link_for_candidate(self, link, candidate):

return index.Link(path_to_url(path))

def cleanup(self):
Copy link
Member

Choose a reason for hiding this comment

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

Agreed.

@pradyunsg
Copy link
Member

@xoviat I took this forward in #4764. I hope you don't mind. :)

@ghost
Copy link
Author

ghost commented Oct 4, 2017

No problem.

@ghost ghost closed this Oct 4, 2017
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants