Add blacklist handling for skipping requirements in pex resolver #457
Conversation
CMLivingston
changed the title
pex/resolver.py
Outdated
| def resolvable_is_blacklisted(self, resolvable_name): | ||
| if not resolvable_name in self._blacklist: | ||
| return False | ||
| return self._interpreter.identity.matches(self._blacklist[resolvable_name]) |
There was a problem hiding this comment.
A possible one-liner
return (
resolvable_name in self._blacklist and
self._interpreter.identity.matches(self._blacklist[resolvable_name])
)
pex/resolver.py
Outdated
| self._interpreter = interpreter or PythonInterpreter.get() | ||
| self._platform = platform or Platform.current() | ||
| self._allow_prereleases = allow_prereleases | ||
| self._blacklist = pkg_blacklist or {} |
There was a problem hiding this comment.
Not sure if it matters here. This line carries risk of pkg_blacklist being updated outside of the function and impact execution inside since pkg_blacklist becomes a shared object. Maybe self._blacklist = {}.update(pkg_blacklsit or {}) will be safer?
pex/resolver.py
Outdated
| @@ -193,6 +199,12 @@ def resolve(self, resolvables, resolvable_set=None): | |||
| if resolvable in processed_resolvables: | |||
| continue | |||
| packages = self.package_iterator(resolvable, existing=resolvable_set.get(resolvable.name)) | |||
|
|
|||
| # TODO: Remove blacklist strategy in favor of smart requirement handling | |||
There was a problem hiding this comment.
maybe
if not self.resolvable_is_blacklisted(resolvable.name):
resolvable_set.merge(resolvable, packages, parent)
processed_resolvables.add(resolvable)
seems cleaner to me.
| @@ -329,6 +342,10 @@ def resolve(requirements, | |||
| ``context``. | |||
| :keyword allow_prereleases: (optional) Include pre-release and development versions. If | |||
| unspecified only stable versions will be resolved, unless explicitly included. | |||
| :keyword pkg_blacklist: (optional) A blacklist dict (str->str) that maps package name to | |||
There was a problem hiding this comment.
would love to have a better documentation on why a package needed to be blacklisted
pex/resolver.py
Outdated
| @@ -180,6 +181,12 @@ def build(self, package, options): | |||
| 'Could not get distribution for %s on platform %s.' % (package, self._platform)) | |||
| return dist | |||
|
|
|||
| def resolvable_is_blacklisted(self, resolvable_name): | |||
There was a problem hiding this comment.
this should probably be private, especially since this solution is meant to be temporary.
| @@ -193,7 +200,11 @@ def resolve(self, resolvables, resolvable_set=None): | |||
| if resolvable in processed_resolvables: | |||
| continue | |||
| packages = self.package_iterator(resolvable, existing=resolvable_set.get(resolvable.name)) | |||
| resolvable_set.merge(resolvable, packages, parent) | |||
|
|
|||
| # TODO: Remove blacklist strategy in favor of smart requirement handling | |||
There was a problem hiding this comment.
this TODO is good, but it'd be great to make the temporary nature of this change more evident in the docstring(s) below where developers are more likely to look. Would be good to include the ticket in there as well.
tests/test_resolver.py
Outdated
| @@ -349,3 +352,32 @@ def test_resolvable_set_built(): | |||
| updated_rs.merge(rq, [binary_pkg]) | |||
| assert updated_rs.get('foo') == set([binary_pkg]) | |||
| assert updated_rs.packages() == [(rq, set([binary_pkg]), None, False)] | |||
|
|
|||
| def test_resolver_blacklist(): | |||
| project = make_sdist(name='project', version='1.0.0') | |||
There was a problem hiding this comment.
this probably needs a setup.py-sourced transitive dep to blacklist as a complete test case?
tests/test_resolver.py
Outdated
| blacklist = {'project2': '>3'} | ||
| required_project = "project2;python_version<'3'" | ||
|
|
||
| project1 = make_sdist(name='project1', version='1.0.0', install_reqs=["project2;python_version<'3'"]) |
There was a problem hiding this comment.
required_project is unused here.
Problem
When resolving for a specific interpreter version, the pex resolver fails to resolve universal requirements that transitively require libraries with interpreter constraints that conflict with the target interpreter version (see: https://github.com/Julian/jsonschema/blob/master/setup.py#L43 ).
Solution
The short term solution is to allow the caller of pex.resolver#resolve to supply a blacklist dict that maps package name -> interpreter constraint, enabling the resolver to skip blacklisted requirement names at resolve time if the target interpreter conforms with the interpreter constraint. The long-term solution is to be addressed by #456.
Result
It is now possible for systems that build pex files (Pants) via the pex api to blacklist certain requirements that are redundant (backports) or otherwise should not be included in the output pex.