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

Refactor python pipeline utilities #5474

Merged
merged 3 commits into from Feb 17, 2018

Conversation

Projects
None yet
4 participants
@benjyw
Copy link
Contributor

benjyw commented Feb 15, 2018

Refactor python pipeline utilities.

Primarily - when you have some standalone requirements
specified as pip-style strings, previously we injected a
PythonRequirementsLibrary into the build graph for them,
and then passed them down the pipeline.
This refactoring allows them to be resolved without that
unnecessary hackery.

This will help get rid of some legacy cruft from
the old pipeline, such as PythonTask.

Also applies changes to two contrib tasks: eval and mypy.

benjyw added some commits Feb 15, 2018

Refactor python pipeline utilities.
This will help get rid of some legacy cruft from
the old pipeline, such as PythonTask.
@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Feb 15, 2018

There are two commits, best reviewed separately: One for the refactoring changes, the other for their application in the mypy contrib. @tdyas - your review of the latter would be great.

@benjyw benjyw requested review from tdyas and CMLivingston Feb 15, 2018

@benjyw benjyw changed the title Mypy fixes2 Refactor python pipeline utilities Feb 15, 2018

@tdyas

This comment has been minimized.

Copy link
Contributor

tdyas commented Feb 15, 2018

Looks good to me from the mypy side of things.

@benjyw benjyw requested a review from kwlzn Feb 15, 2018

@CMLivingston
Copy link
Contributor

CMLivingston left a comment

Thanks for cleaning this up! I have two small suggestions (take or leave) and a question about the new target_set_id for the extra requirements pex. Beyond that, this looks good to me.

else:
req_strings_id = hash_all(requirement_strings)

path = os.path.realpath(os.path.join(self.workdir, str(interpreter.identity), req_strings_id))

This comment has been minimized.

@CMLivingston

CMLivingston Feb 16, 2018

Contributor

It appears that this "extra_requirements" pex was previously governed by the CacheManager + an invalidation check. I am not sure if hashing in this way would cause any issues since the reqs are coming from self.extra_requirements (which don't/rarely change afaict?). Is the invalidation strategy a concern here?

This comment has been minimized.

@benjyw

benjyw Feb 16, 2018

Contributor

The id will change if any of the requirements strings changes, and the requirements will be re-resolved. So this is as robust as generating a synthetic target and running invalidation on it, without the superfluous machinery. We have to create a unique output path anyway, so we couldn't remove any of this code even if we did do the other thing.

builder = PEXBuilder(path=path, interpreter=interpreter, copy=True)
dump_requirements(builder, interpreter, req_libs, self.context.log)
builder.freeze()
def resolve_requirement_strings(self, interpreter, requirement_strings):

This comment has been minimized.

@CMLivingston

CMLivingston Feb 16, 2018

Contributor

I think the name of this method could be more descriptive, like build_pex_from_requirement_strings, but this is just my opinion and the current name is in line with the previous name, so it also makes sense to leave as is. Just a suggestion.

builder.add_interpreter_constraint(constraint)
builder.freeze()
pexes = [extra_requirements_pex] + pexes
constraints = {constraint for rt in relevant_targets if has_python_sources(rt)

This comment has been minimized.

@CMLivingston

CMLivingston Feb 16, 2018

Contributor

Might be helpful to name this more explicitly to interpreter_constraints since this was previously explained by the builder.add_interpreter_constraint(constraint) on the following line, and now this info is not immediately inferable from the file context alone. However, one could argue that experienced devs will get this and this is more of a concern for those who are not super-familiar with Python targets, so just a suggestion.

for req_lib in req_libs:
for req in req_lib.requirements:
reqs.add(req)
deduped_reqs = OrderedSet()

This comment has been minimized.

@jsirois

jsirois Feb 16, 2018

Member

deduped_reqs = OrderedSet(reqs) should work.

This comment has been minimized.

@benjyw

benjyw Feb 16, 2018

Contributor

Ah yes, good call. Done!

return PEX(path, interpreter=interpreter)

@classmethod
def merge_pexes(cls, path, pex_info, interpreter, pexes, interpeter_constraints=None):

This comment has been minimized.

@jsirois

jsirois Feb 16, 2018

Member

Seems worth calling out pex_info is mutated or else doing a pex_info.copy().

This comment has been minimized.

@benjyw

benjyw Feb 16, 2018

Contributor

Good idea, done (copied the pex_info).

@benjyw benjyw force-pushed the benjyw:mypy_fixes2 branch from 04a544e to 38ea5c5 Feb 16, 2018

@benjyw benjyw merged commit bc58220 into pantsbuild:master Feb 17, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@benjyw benjyw deleted the benjyw:mypy_fixes2 branch Feb 17, 2018

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