Repository navigation
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request isolates temporary variables to individual executor invocations by introducing an execution-local temporary_variable_container_ in ExecutorState. This prevents aborted or failed executions (such as those caused by errors, cancellations, or dataset exhaustion) from poisoning subsequent invocations, while ensuring other resources like TensorArray and Stack retain their existing lifetimes. Comprehensive C++ and Python tests have been added to verify these lifetime guarantees. I have no feedback to provide as the implementation is robust and well-tested.
Fixes #130303.
Problem
When Grappler rewrites
AddNintoTemporaryVariable/AssignAdd/DestroyTemporaryVariable, dataset exhaustion can abort execution after the temporary resource is created but before it is destroyed.Eager function invocations may reuse the same step container, leaving the resource behind and causing
AlreadyExistsErroron subsequent epochs.Fix
ScopedStepContainerfor temporary variables.TemporaryVariableandDestroyTemporaryVariablethrough this container, ensuring cleanup when execution finishes, including after errors or cancellation.Tests
Added C++ and Python regression tests covering successful execution, EOF, errors, concurrent invocations, cancellation, and resource lifetime compatibility.
Locally verified on WSL2/Docker (CPU-only):
executor_testsuite: passed.temporary_variable_testsuite: passed.AlreadyExistsError.