https://pantsbuild.org/ logo
Join Slack
Powered by
With the call-by-name migration, I think I'll be s...
# development
w
With the call-by-name migration, I think I'll be seeing a lot more of these (https://github.com/pantsbuild/pants/pull/21043) - works at runtime, fails typecheck:
Copy code
06:48:06.66 [ERROR] Completed: Typecheck using MyPy - mypy - mypy failed (exit code 1).
src/python/pants/backend/cc/dependency_inference/rules.py: note: In function "infer_cc_source_dependencies":
src/python/pants/backend/cc/dependency_inference/rules.py:129:13: error: Argument 1 to "determine_explicitly_provided_dependencies" has incompatible type "DependenciesRequest"; expected "ExplicitlyProvidedDependenciesRequest"  [arg-type]
                DependenciesRequest(request.field_set.dependencies), **implicitly()
Here was the original Getter:
Copy code
Get(ExplicitlyProvidedDependencies, DependenciesRequest(request.field_set.dependencies)),
And here's the updated call-by-name:
Copy code
determine_explicitly_provided_dependencies(
            DependenciesRequest(request.field_set.dependencies), **implicitly()
        ),
Which causes this Typecheck error in rules.py:
Copy code
Argument of type "DependenciesRequest" cannot be assigned to parameter "request" of type "ExplicitlyProvidedDependenciesRequest"
  "DependenciesRequest" is incompatible with "ExplicitlyProvidedDependenciesRequest"PylancereportArgumentType
Structure of the called rule:
Copy code
@rule
async def determine_explicitly_provided_dependencies(
    request: ExplicitlyProvidedDependenciesRequest,
    union_membership: UnionMembership,
    registered_target_types: RegisteredTargetTypes,
    subproject_roots: SubprojectRoots,
) -> ExplicitlyProvidedDependencies:
And the Deps vs ExplicitDeps:
Copy code
@dataclass(frozen=True)
class DependenciesRequest(EngineAwareParameter):
    field: Dependencies
    should_traverse_deps_predicate: ShouldTraverseDepsPredicate = TraverseIfDependenciesField()

    def debug_hint(self) -> str:
        return self.field.address.spec


# NB: ExplicitlyProvidedDependenciesRequest does not have a predicate unlike DependenciesRequest.
@dataclass(frozen=True)
class ExplicitlyProvidedDependenciesRequest(EngineAwareParameter):
    field: Dependencies

    def debug_hint(self) -> str:
        return self.field.address.spec
These might be a case-by-case handling problem, but my assumption is that 
Get(ExplicitlyProvidedDependencies
is the meat of what should be happening, and the rule engine did magic to make this happen 
DependenciesRequest(request.field_set.dependencies)),
Since converting Dep request to Explicit just abandons the 
should_traverse
param
h
OOooof
w
Yeah, I spent about 15 minutes right now running through that a few times. There is a TODO to maybe deprecate the API this was calling, in favour of one that doesn’t do that
Copy code
@rule
async def convert_dependencies_request_to_explicitly_provided_dependencies_request(
    request: DependenciesRequest,
) -> ExplicitlyProvidedDependenciesRequest:
    """This rule discards any deps predicate from DependenciesRequest.

    Calculating ExplicitlyProvidedDependencies does not use any deps traversal predicates as it is
    meant to list all explicit deps from the given field. By stripping the predicate from the
    request, we ensure that the cache key for ExplicitlyProvidedDependencies calculation does not
    include the predicate increasing the cache-hit rate.
    """
    # TODO: Maybe require Get(ExplicitlyProvidedDependencies, ExplicitlyProvidedDependenciesRequest)
    #       and deprecate Get(ExplicitlyProvidedDependencies, DependenciesRequest) via this rule.
    return ExplicitlyProvidedDependenciesRequest(request.field)
p
I suspect there are a lot of places where we provide the input for one rule expecting it to be transformed into the input of another rule; that other rule is what gives us the output we want. Can your tool get a rule graph from pantsd and use that to detect such intermediate rules?
w
Here we go, this is the one I remember so fondly from my testing
PexProcess
-> 
Process
-> magic -> 
FaillibleProcessResult
-> 
ProcessResult
@witty-crayon-22786 How should we handle these? In this case, I'm getting a typecheck error, but also a whole slew of rule graph errors - when PexProcessis in there
I feel like it's supposed to be 
process_request_to_process_result
w
if any sort of argument conversion is supposed to happen, then it should not be a positional argument: rather an 
**implicitly(PexProcess(..))
you call the function whose result you want, but you implicitly convert the arguments
w
Ahh, that's interesting - I'll try that tonight. That's definitely not in the migration though - really need to write some sort of mypy checker for this
And by tonight, I mean right now. Works!!
w
Hm: shouldn't need a mypy check: basically, unless the positional arg type exactly matches (no inheritance, etc), then it needs to be passed implicitly.
w
I mean on the migration, or after the fact for devs to lint/check against. This will be "a thing" Is this the more general approach to union-rule'd additions? (Trying to write some docs about the migration process)
Actually, handling this in the migration shouldn't be so bad - given all the information we already have (https://github.com/pantsbuild/pants/issues/21072). I still think a 
mypy
plugin or something for post-migration would be nice
7 Views
Open in Slack
PreviousNext
Powered by