WIP: In target adaptors, populate attributes with a None value #7680
Conversation
|
Help much appreciated here. First off, is this solution correct to aim for? Second, any recommendations on how to go about implementing this? Here’s where I got in my investigation: pants/src/python/pants/engine/struct.py Line 42 in 3742edb • The Parser somehow gets kwargs passed to itself, then passes them to the init function of pants.init.engine_initializer._make_target_adaptor.<locals>.GlobsHandlingTargetAdaptor.
|
|
Adding @jsirois, who added the Struct API at the beginning of the v2 engine experiments. I'll take a look in an hour or two. |
|
@illicitonion and I have been discussing this extensively over Slack, but would love further feedback. We proposed four ways to populate
We rejected #4 outright for being a leaky abstraction. We should not have to rewrite our For #1 and #2, I think there is value in by default having all Instead, we are thinking #3 is the best solution for now. We can start by implementing #3, and then down the road we can decide let's just merge this into either #1 or #2 to automatically define -- So, if you all agree with those decisions, the question becomes how to implement #3. Daniel pointed to #7661 as being highly relevant. We'd be introducing a "Target Union". |
…ibility` value rather than a `PythonTarget`(#7691) ### Problem Our V2 Target API is not fully implemented, including that we do not populate in target adaptors attributes with a default value of `None` (#7680). This limitation resulted in https://github.com/pantsbuild/pants/pull/7679/files#diff-04a0048c70898e46a42c73225c47b906R58 having to monkey patch the `.compatibility` field for the target adaptors to get `PythonSetup.interpreter_or_constraints()` to not raise an `AttributeError`. While the underlying issue could be resolved with changing the V2 Target API, we can work around this by changing `PythonSetup.interpreter_or_constraints()` to no longer require a `Target` but rather just its `.compatibility` field. ### Solution Change `PythonSetup.interpreter_or_constraints()` to have a more direct API of expecting the `compatibility` field itself. #### Not using a deprecation Neither this function nor its class `PythonSetup` have a public API. Per our deprecation policy, and confirmed via Slack, it was decided that we do not need a deprecation cycle for this change. ### Result #7680 is unblocked. Regardless of V2, this is also arguably a better API to only request as a parameter the field we actually need. For example, this makes testing easier because we no longer would need to create a fake `PythonTarget`.
|
Closing because I do not plan to finish this PR anytime soon. The underlying motivation still exists, however. |
Problem
Certain attributes default to
None, such asPythonTarget'scompatibilitydefaulting toNone:pants/src/python/pants/backend/python/targets/python_target.py
Lines 27 to 32 in 3742edb
In the case of target adaptors, such as a
PythonTarget, if the BUILD entry does not provide an explicit value for a field likecompatibility, then the target adaptor will simply not have that attribute defined, as opposed to having it withNoneas a value.This became an issue when working on #7679. There, we are trying to call
PythonSetup.compatibility_or_constraints(hydrated_target.adaptor). This function expects the passed object to have.compatibilitydefined, or else it will raise anAttributeError. However, the value does not need to have a non-None value; in fact, a value ofNoneis meaningful and means that the global constraints should be used. So, if we were to filter out the hydrated targets to only call the function on targets with.compatibilityset to non-None, then we would never be considering the global interpreter constraints.Solution
First, we must modify the engine initializer to identify and populate attributes that are not defined in the BUILD entry but who have a
Nonevalue in the corresponding target class. TODO.Second, we must modify the
create()function to not filter out attributes withNone.