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

Properly resolve transitive dependencies in V2 Pytest runner #7720

Merged

Conversation

Projects
None yet
3 participants
@Eric-Arellano
Copy link
Contributor

commented May 14, 2019

Problem

./pants --no-v1 --v2 test tests/python/pants_test/util:dirutil would fail to pass because the snapshot would not include the file strutil.py so the import would fail.

This is the result of transitive dependencies not being properly resolved. test_dirutil.py does not directly depend on strutil.py; instead, test_dirutil.py depends on dirutil.py, which itself depends on strutil.py. We were not recursively grabbing the 3rd party requirements and source files for transitive dependencies.

Solution

TransitiveHydratedTarget already resolves all transitive dependencies for us. We simply must use those results differently to fix this issue.

Specifically, we can convert the TransitiveHydratedTarget into TransitiveHydratedTargets, which then allows us to access the closure property to get all the HydratedTargets in a de-duplicated and flattened data structure.

Result

./pants --no-v1 --v2 test tests/python/pants_test/util:dirutil now works, along with the majority of the util unit tests.

@Eric-Arellano Eric-Arellano requested a review from illicitonion May 14, 2019

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

@illicitonion put this up now so that you can review while still at work. Here's how I recommend approaching it:

  1. Start with the integration tests: 407c6b7
  2. How I originally fixed it: de95e8e
  3. How I refactored it: 95453cb

@Eric-Arellano Eric-Arellano changed the title Properly resolve in V2 Pytest runner transitive dependencies Properly resolve transitive dependencies in V2 Pytest runner May 14, 2019

@@ -42,18 +45,31 @@ def parse_interpreter_constraints(python_setup, python_target_adaptors):
return constraints_args


def resolve_all_transitive_hydrated_targets(initial_transitive_hydrated_target):

This comment has been minimized.

Copy link
@illicitonion

illicitonion May 14, 2019

Contributor

I think this should be equivalent to all_targets = transitive_hydrated_target.closure...

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano May 14, 2019

Author Contributor

That sounds very nifty, and I see now that that is available in V1. It doesn't look to be implemented in V2:

(Pdb) dir(transitive_hydrated_target)
['__abstractmethods__', '__add__', '__class__', '__contains__', '__delattr__', '__dict__', '__dir__', '__doc__', '__eq__', '__format__', '__ge__', '__getattribute__', '__getitem__', '__getnewargs__', '__gt__', '__hash__', '__init__', '__init_subclass__', '__iter__', '__le__', '__len__', '__lt__', '__module__', '__mul__', '__ne__', '__new__', '__reduce__', '__reduce_ex__', '__repr__', '__rmul__', '__setattr__', '__sizeof__', '__slots__', '__str__', '__subclasshook__', '__weakref__', '_abc_cache', '_abc_negative_cache', '_abc_negative_cache_version', '_abc_registry', '_asdict', '_fields', '_make', '_replace', '_source', '_super_iter', 'copy', 'count', 'dependencies', 'index', 'make_type_error', 'root', 'type_check_error_type']

(Pdb) dir(transitive_hydrated_target.root)
['__abstractmethods__', '__add__', '__class__', '__contains__', '__delattr__', '__dict__', '__dir__', '__doc__', '__eq__', '__format__', '__ge__', '__getattribute__', '__getitem__', '__getnewargs__', '__gt__', '__hash__', '__init__', '__init_subclass__', '__iter__', '__le__', '__len__', '__lt__', '__module__', '__mul__', '__ne__', '__new__', '__reduce__', '__reduce_ex__', '__repr__', '__rmul__', '__setattr__', '__sizeof__', '__slots__', '__str__', '__subclasshook__', '__weakref__', '_abc_cache', '_abc_negative_cache', '_abc_negative_cache_version', '_abc_registry', '_asdict', '_fields', '_make', '_replace', '_source', '_super_iter', 'adaptor', 'address', 'addresses', 'copy', 'count', 'dependencies', 'index', 'make_type_error', 'type_check_error_type']

(Pdb) dir(transitive_hydrated_target.root.adaptor)
['_ABSTRACT_FIELD', '_INHERITANCE_FIELDS', '_INTERNAL_FIELDS', '_TYPE_ALIAS_FIELD', '_UNINHERITABLE_FIELDS', '__abstractmethods__', '__class__', '__delattr__', '__dict__', '__dir__', '__doc__', '__eq__', '__format__', '__ge__', '__getattr__', '__getattribute__', '__gt__', '__hash__', '__init__', '__init_subclass__', '__le__', '__lt__', '__module__', '__ne__', '__new__', '__reduce__', '__reduce_ex__', '__repr__', '__setattr__', '__sizeof__', '__str__', '__subclasshook__', '__weakref__', '_abc_cache', '_abc_negative_cache', '_abc_negative_cache_version', '_abc_registry', '_asdict', '_extract_inheritable_attributes', '_key', '_kwargs', 'abstract', 'address', 'create', 'default_sources_exclude_globs', 'default_sources_globs', 'dependencies', 'extends', 'field_adaptors', 'get_sources', 'is_serializable', 'is_serializable_type', 'kwargs', 'merges', 'name', 'report_validation_error', 'type_alias', 'validate', 'validate_concrete', 'validate_sources']

Should this PR be focused on adding a closure attribute to all transitive hydrated targets? Would this be defined on the target_adaptor, the hydrated_target, or the transitive_hydrated_target?

This comment has been minimized.

Copy link
@illicitonion

illicitonion May 14, 2019

Contributor

Oh sorry, I mis-read slightly. If you request a TransitiveHydratedTargets instead of a TransitiveHydratedTarget, that has a root field which is the TransitiveHydratedTarget, and a closure field which is a flattened tuple of the transitive deps.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano May 14, 2019

Author Contributor

Opened up #7726 to make this possible.

This comment has been minimized.

Copy link
@stuhood

stuhood May 14, 2019

Member

Re: #7726: looking at the rule graph rendered by ./pants --native-engine-visualize-to=blah help, I see:
Screen Shot 2019-05-14 at 11 48 14 AM

Which indicates that you should be able to get TransitiveHydratedTargets using BuildFileAddresses. So:

transitive_hydrated_targets = yield Get(TransitiveHydratedTargets, BuildFileAddresses([address_of_the_root_target_i_care_about]))

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano May 14, 2019

Author Contributor

Great find! It's indeed a bit awkward, but better than defining our own recursive traversal code. I updated the issue to reflect that we now use this.

@Eric-Arellano Eric-Arellano force-pushed the Eric-Arellano:v2-pytest-transitive-deps branch from 2cc6555 to 93d304a May 14, 2019

@stuhood
Copy link
Member

left a comment

I believe that this can be unblocked via the above: but if you give it a try and it is too awkward, then feel free to land as is.

@stuhood

This comment has been minimized.

Copy link
Member

commented May 14, 2019

Re:

Use builtin Rust rule

This one is python!

@rule(TransitiveHydratedTargets, [BuildFileAddresses])
def transitive_hydrated_targets(build_file_addresses):
"""Given BuildFileAddresses, kicks off recursion on expansion of TransitiveHydratedTargets.
The TransitiveHydratedTarget struct represents a structure-shared graph, which we walk
and flatten here. The engine memoizes the computation of TransitiveHydratedTarget, so
when multiple TransitiveHydratedTargets objects are being constructed for multiple
roots, their structure will be shared.
"""
transitive_hydrated_targets = yield [Get(TransitiveHydratedTarget, Address, a)
for a in build_file_addresses.addresses]
closure = OrderedSet()
to_visit = deque(transitive_hydrated_targets)
while to_visit:
tht = to_visit.popleft()
if tht.root in closure:
continue
closure.add(tht.root)
to_visit.extend(tht.dependencies)
yield TransitiveHydratedTargets(tuple(tht.root for tht in transitive_hydrated_targets), closure)

@Eric-Arellano Eric-Arellano merged commit c6be885 into pantsbuild:master May 14, 2019

1 check passed

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

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:v2-pytest-transitive-deps branch May 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.