Remove the umbrella import. #1

michaelreneer · 2018-12-18T23:58:32Z

No description provided.

ZacharyGarrett

Approving to test the workflow.

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: 502890245

…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: 502890245

…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

1. Rollback cl/348558130 - Refactors the computation wrapper to allow for non-tracing strategies. 2. Rollback cl/334208326 - Remove the remaining TFF stack frames from tracing errors. 4. Rollback cl/333598731 - Invert control of some tracing functions to shorten stacktraces. PiperOrigin-RevId: 568323122

These three CLs are stacked on top of each other and they make the tracing logic more complex. Rolling these CLs back will simplify tracing logic and improve static analysis and lint. The stacktraces will have more depth, but hopefully they will also be more clear as well. 1. Revert b82dd1a - Refactors the computation wrapper to allow for non-tracing strategies. 2. Revert 827f21e - Remove the remaining TFF stack frames from tracing errors. 4. Revert 1406a91 - Invert control of some tracing functions to shorten stacktraces. PiperOrigin-RevId: 568323122

``` # expansion is pretty cool # https://www.gnu.org/software/bash/manual/html_node/Shell-Expansions.html echo "hello `world`" echo "hello ${world}" # single quotes disable expansion # https://www.gnu.org/software/bash/manual/html_node/Single-Quotes.html echo 'hello `world`' echo 'hello ${world}' # Problem - single quotes in single quotes echo 'hello ' world' # Fix #1: quote the single quote, which is exciting echo 'hello '"'"' world' # Fix #2: heredoc, maybe more readable but awkward # https://www.gnu.org/savannah-checkouts/gnu/bash/manual/bash.html#Here-Documents echo <<'EOF' hello ' world EOF ``` PiperOrigin-RevId: 675226634

``` # expansion is pretty cool # https://www.gnu.org/software/bash/manual/html_node/Shell-Expansions.html echo "hello `world`" echo "hello ${world}" # single quotes disable expansion # https://www.gnu.org/software/bash/manual/html_node/Single-Quotes.html echo 'hello `world`' echo 'hello ${world}' # Problem - single quotes in single quotes echo 'hello ' world' # Fix #1: quote the single quote, which is exciting echo 'hello '"'"' world' # Fix #2: heredoc, maybe more readable but awkward # https://www.gnu.org/savannah-checkouts/gnu/bash/manual/bash.html#Here-Documents echo <<'EOF' hello ' world EOF ``` PiperOrigin-RevId: 675586161

Remove the umbrella import.

292bfb0

michaelreneer requested a review from ZacharyGarrett December 19, 2018 00:01

ZacharyGarrett approved these changes Dec 19, 2018

View reviewed changes

michaelreneer closed this Dec 19, 2018

michaelreneer deleted the test-pr branch December 19, 2018 04:13

copybara-service bot mentioned this pull request Jan 18, 2023

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

Merged

copybara-service bot mentioned this pull request Sep 25, 2023

Simplify tracing computations, part #1. #4138

Closed

copybara-service bot mentioned this pull request Sep 27, 2023

Simplify tracing computations. #4139

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove the umbrella import. #1

Remove the umbrella import. #1

michaelreneer commented Dec 18, 2018

ZacharyGarrett left a comment

Remove the umbrella import. #1

Remove the umbrella import. #1

Conversation

michaelreneer commented Dec 18, 2018

ZacharyGarrett left a comment

Choose a reason for hiding this comment