A task to resolve python requirements. #4065

Merged
merged 4 commits into from Nov 18, 2016

Conversation

Projects
None yet
2 participants
@benjyw
Contributor

benjyw commented Nov 16, 2016

Creates an (unzipped) PEX on disk, containing all
the resolved requirements.

Future changes will merge this PEX with PEXes created from
in-repo sources and generated sources, to create a python
environment suitable for running the relevant code.

Note: This will first require changes to PEX to support
merging.

A task to resolve python requirements.
Creates an (unzipped) PEX on disk, containing all
the resolved requirements.

Future changes will merge this PEX with PEXes created from
in-repo sources and generated sources, to create a python
environment suitable for running the relevant code.

Note: This will first require changes to PEX to support
      merging.

@benjyw benjyw assigned benjyw and jsirois and unassigned benjyw Nov 16, 2016

@benjyw

This comment has been minimized.

Show comment
Hide comment
@benjyw

benjyw Nov 16, 2016

Contributor

Note that this copies some code from PythonChroot, which can go away once we're entirely on the new python pipeline. The whole point is not to have a single chroot that knows how to do everything from codegen to requirements...

Contributor

benjyw commented Nov 16, 2016

Note that this copies some code from PythonChroot, which can go away once we're entirely on the new python pipeline. The whole point is not to have a single chroot that knows how to do everything from codegen to requirements...

- register('--platforms', advanced=True, type=list, default=['current'],
- help='The wheel version for this python environment.')
+ register('--platforms', advanced=True, type=list, metavar='<platform>', default=['current'],
+ help='A list of platforms to be supported by this python environment. Each playform'

This comment has been minimized.

@jsirois

jsirois Nov 17, 2016

Member

s/playform/platform/

@jsirois

jsirois Nov 17, 2016

Member

s/playform/platform/

This comment has been minimized.

@benjyw

benjyw Nov 18, 2016

Contributor

Fixed.

@benjyw

benjyw Nov 18, 2016

Contributor

Fixed.

- 'src/python/pants/base:exceptions',
- 'tests/python/pants_test/tasks:task_test_base',
- ]
+ name='resolve_requirements',

This comment has been minimized.

@jsirois

jsirois Nov 17, 2016

Member

Any reason not to just use default name and globs and leverage py.test passthrough, ie: ./pants test ... -- -k<pattern>?

@jsirois

jsirois Nov 17, 2016

Member

Any reason not to just use default name and globs and leverage py.test passthrough, ie: ./pants test ... -- -k<pattern>?

This comment has been minimized.

@benjyw

benjyw Nov 18, 2016

Contributor

No reason other than convention - we seem to like having a separate target per test module everywhere else. But I am 100% in favor of what you suggest and am happy to start the pushback here... :)

@benjyw

benjyw Nov 18, 2016

Contributor

No reason other than convention - we seem to like having a separate target per test module everywhere else. But I am 100% in favor of what you suggest and am happy to start the pushback here... :)

This comment has been minimized.

@benjyw

benjyw Nov 18, 2016

Contributor

Done.

@benjyw

benjyw Nov 18, 2016

Contributor

Done.

+ def compute_fingerprint(self, req_lib):
+ hash_elements_for_target = []
+ hash_elements_for_target.extend([req.cache_key() for req in req_lib.requirements])
+ if not hash_elements_for_target:

This comment has been minimized.

@jsirois

jsirois Nov 17, 2016

Member

Its too bad PythonRequirementLibrary allows 0 requirements since that case is of no practical use that I can tell, and it just complicates code like this here.

@jsirois

jsirois Nov 17, 2016

Member

Its too bad PythonRequirementLibrary allows 0 requirements since that case is of no practical use that I can tell, and it just complicates code like this here.

This comment has been minimized.

@benjyw

benjyw Nov 18, 2016

Contributor

Yep, but here we are...

@benjyw

benjyw Nov 18, 2016

Contributor

Yep, but here we are...

+ for running the relevant python code.
+ """
+
+ REQUIREMENTS_PEX = 'python_requirements_pex'

This comment has been minimized.

@jsirois

jsirois Nov 17, 2016

Member

Checking my understanding: This time you're using a string product type deliberately because there will be (at least) 2 PEX products, requirements chroots and actual python_binary pexes; thus, using the PEX type now would become ambiguous in a follow-up RB.

@jsirois

jsirois Nov 17, 2016

Member

Checking my understanding: This time you're using a string product type deliberately because there will be (at least) 2 PEX products, requirements chroots and actual python_binary pexes; thus, using the PEX type now would become ambiguous in a follow-up RB.

This comment has been minimized.

@benjyw

benjyw Nov 18, 2016

Contributor

Exactly right. There might in fact be multiple source pexes (one for real sources, one for generated thrift, one for generated proto) plus this one for requirements, so using PEX as a type seemed not worth it.

@benjyw

benjyw Nov 18, 2016

Contributor

Exactly right. There might in fact be multiple source pexes (one for real sources, one for generated thrift, one for generated proto) plus this one for requirements, so using PEX as a type seemed not worth it.

+
+ return context.products.get_data(ResolveRequirements.REQUIREMENTS_PEX)
+
+ def test_resolve_requirements(self):

This comment has been minimized.

@jsirois

jsirois Nov 17, 2016

Member

It'd be nice to test multi-platform resolves as well - that's key functionality.

@jsirois

jsirois Nov 17, 2016

Member

It'd be nice to test multi-platform resolves as well - that's key functionality.

This comment has been minimized.

@benjyw

benjyw Nov 18, 2016

Contributor

Good idea. Done!

@benjyw

benjyw Nov 18, 2016

Contributor

Good idea. Done!

@benjyw

Thanks for the comments. PTAL.

- register('--platforms', advanced=True, type=list, default=['current'],
- help='The wheel version for this python environment.')
+ register('--platforms', advanced=True, type=list, metavar='<platform>', default=['current'],
+ help='A list of platforms to be supported by this python environment. Each playform'

This comment has been minimized.

@benjyw

benjyw Nov 18, 2016

Contributor

Fixed.

@benjyw

benjyw Nov 18, 2016

Contributor

Fixed.

+ def compute_fingerprint(self, req_lib):
+ hash_elements_for_target = []
+ hash_elements_for_target.extend([req.cache_key() for req in req_lib.requirements])
+ if not hash_elements_for_target:

This comment has been minimized.

@benjyw

benjyw Nov 18, 2016

Contributor

Yep, but here we are...

@benjyw

benjyw Nov 18, 2016

Contributor

Yep, but here we are...

+ for running the relevant python code.
+ """
+
+ REQUIREMENTS_PEX = 'python_requirements_pex'

This comment has been minimized.

@benjyw

benjyw Nov 18, 2016

Contributor

Exactly right. There might in fact be multiple source pexes (one for real sources, one for generated thrift, one for generated proto) plus this one for requirements, so using PEX as a type seemed not worth it.

@benjyw

benjyw Nov 18, 2016

Contributor

Exactly right. There might in fact be multiple source pexes (one for real sources, one for generated thrift, one for generated proto) plus this one for requirements, so using PEX as a type seemed not worth it.

- 'src/python/pants/base:exceptions',
- 'tests/python/pants_test/tasks:task_test_base',
- ]
+ name='resolve_requirements',

This comment has been minimized.

@benjyw

benjyw Nov 18, 2016

Contributor

No reason other than convention - we seem to like having a separate target per test module everywhere else. But I am 100% in favor of what you suggest and am happy to start the pushback here... :)

@benjyw

benjyw Nov 18, 2016

Contributor

No reason other than convention - we seem to like having a separate target per test module everywhere else. But I am 100% in favor of what you suggest and am happy to start the pushback here... :)

- 'src/python/pants/base:exceptions',
- 'tests/python/pants_test/tasks:task_test_base',
- ]
+ name='resolve_requirements',

This comment has been minimized.

@benjyw

benjyw Nov 18, 2016

Contributor

Done.

@benjyw

benjyw Nov 18, 2016

Contributor

Done.

+
+ return context.products.get_data(ResolveRequirements.REQUIREMENTS_PEX)
+
+ def test_resolve_requirements(self):

This comment has been minimized.

@benjyw

benjyw Nov 18, 2016

Contributor

Good idea. Done!

@benjyw

benjyw Nov 18, 2016

Contributor

Good idea. Done!

@jsirois

LGTM

@benjyw benjyw merged commit 2bd2782 into pantsbuild:master Nov 18, 2016

1 of 2 checks passed

coverage/coveralls Coverage pending from Coveralls.io
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@benjyw benjyw deleted the benjyw:resolve_requirements branch Nov 18, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment