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

Enabled and fixed python type annotations in some modules in the executors package. #3525

Merged
merged 1 commit into from
Jan 24, 2023

Conversation

copybara-service[bot]
Copy link

@copybara-service copybara-service bot commented Jan 18, 2023

Enabled and fixed python type annotations in some modules in the executors package.

This revealed two Python typing-related issues:

  1. Every subclass of executor_base.Executor was using a property on the corresponding subclass of executor_value_base.ExecutorValue without having access to that property. This was fixed by:
  2. The methods on the subclasses of federating_executor.FederatingStrategy (FederatedComposingStrategy and FederatedResolvingStrategy) were annotated to return types specific to that subclass. This return type conflicted with the return types of the shared implementations defined in executor_utils.
  3. The type annotation of target_executors parameter of FederatedResolvingStrategy was incorrect.
  • Enabled pytype in the following modules:
    • federated_composing_strategy
    • federated_resolving_strategy
    • federating_executor
    • reference_resolving_executor
    • remote_executor
    • thread_delegating_executor
    • value_serialization

To fix issue #1:

  • Promoted the reference property from the subclasses of executor_value_base.ExecutorValue to executor_value_base.ExecutorValue.
  • Renamed usage of this API to be consistent.

To fix issue #2:

  • Updated the methods on the subclasses of federating_executor.FederatingStrategy (FederatedComposingStrategy and FederatedResolvingStrategy) to accept and return executor_value_base.ExecutorValue types.

Note: That this does make the return types more generic, but I believe this is the intended interface. An alternative fix could be to update the executor_utils function to take and return TypeVar's (i.e. generic functions). However, I don't think this is the intended interface.

To fix issue #3:

  • Updated the type annotation of the target_executors parameter to dict[placements.PlacementLiteral, Union[list[executor_base.Executor], executor_base.Executor]].

@copybara-service copybara-service bot changed the title Enable and fix some Python typing errors in the executors package. Enabled and fixed python type annotations in some modules in the executors package. Jan 23, 2023
@copybara-service copybara-service bot force-pushed the cl/502890245 branch 3 times, most recently from e68335a to 48fa2d6 Compare January 24, 2023 00:24
…cutors` package.

This revealed two Python typing-related issues:
1. Every subclass of `executor_base.Executor` was using a property on the corresponding subclass of `executor_value_base.ExecutorValue` without having access to that property. This was fixed by:
2. The methods on the subclasses of `federating_executor.FederatingStrategy` (`FederatedComposingStrategy` and `FederatedResolvingStrategy`) were annotated to return types specific to that subclass. This return type conflicted with the return types of the shared implementations defined in `executor_utils`.
3. The type annotation of `target_executors` parameter of `FederatedResolvingStrategy` was incorrect.

* Enabled pytype in the following modules:
    * `federated_composing_strategy`
    * `federated_resolving_strategy`
    * `federating_executor`
    * `reference_resolving_executor`
    * `remote_executor`
    * `thread_delegating_executor`
    * `value_serialization`

To fix issue #1:

* Promoted the `reference` property from the subclasses of `executor_value_base.ExecutorValue` to `executor_value_base.ExecutorValue`.
* Renamed usage of this API to be consistent.

To fix issue #2:

* Updated the methods on the subclasses of `federating_executor.FederatingStrategy` (`FederatedComposingStrategy` and `FederatedResolvingStrategy`) to accept and return `executor_value_base.ExecutorValue` types.

Note: That this does make the return types more generic, but I believe this is the intended interface. An alternative fix could be to update the `executor_utils` function to take and return `TypeVar`'s (i.e. generic functions). However, I don't think this is the intended interface.

To fix issue #3:

* Updated the type annotation of the `target_executors` parameter to `dict[placements.PlacementLiteral, Union[list[executor_base.Executor], executor_base.Executor]]`.

PiperOrigin-RevId: 504114859
@copybara-service copybara-service bot merged commit 34a0395 into main Jan 24, 2023
@copybara-service copybara-service bot deleted the cl/502890245 branch January 24, 2023 00:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant