Properly resolve transitive dependencies in V2 Pytest runner #7720
Conversation
|
@illicitonion put this up now so that you can review while still at work. Here's how I recommend approaching it: |
Eric-Arellano
changed the title
| @@ -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): | |||
There was a problem hiding this comment.
I think this should be equivalent to all_targets = transitive_hydrated_target.closure...
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Opened up #7726 to make this possible.
There was a problem hiding this comment.
Re: #7726: looking at the rule graph rendered by ./pants --native-engine-visualize-to=blah help, I see:
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]))
There was a problem hiding this comment.
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
force-pushed
the
v2-pytest-transitive-deps
branch
from
2cc6555 to
93d304a
Compare
|
Re:
This one is python! pants/src/python/pants/engine/legacy/graph.py Lines 454 to 477 in 31f4c73 |
Problem
./pants --no-v1 --v2 test tests/python/pants_test/util:dirutilwould fail to pass because the snapshot would not include the filestrutil.pyso the import would fail.This is the result of transitive dependencies not being properly resolved.
test_dirutil.pydoes not directly depend onstrutil.py; instead,test_dirutil.pydepends ondirutil.py, which itself depends onstrutil.py. We were not recursively grabbing the 3rd party requirements and source files for transitive dependencies.Solution
TransitiveHydratedTargetalready resolves all transitive dependencies for us. We simply must use those results differently to fix this issue.Specifically, we can convert the
TransitiveHydratedTargetintoTransitiveHydratedTargets, which then allows us to access theclosureproperty to get all theHydratedTargetsin a de-duplicated and flattened data structure.Result
./pants --no-v1 --v2 test tests/python/pants_test/util:dirutilnow works, along with the majority of theutilunit tests.