Deflake occasional deadlock in test_dataset.py::test_basic_actors[True]

[ericl]

Why are these changes needed?

This test is flaky. The underlying reason is that Datasets was not forcing eager evaluation of its blocks in the pipeline. This mean that the read stage could get overlapped with the actor pool stage, which can deadlock due to resource contention.

The fix is to force read tasks to finish prior to execution of the second stage, and to execute the stages from actors as a workaround to avoid ownership issues: #20554

To reproduce the hangs/failures:

for i in `seq 1 10`; do pytest -v -s test_dataset.py::test_basic_actors[True]; done

[ericl]

FYI @stephanie-wang @clarkzinzow

[clarkzinzow]

If this is required for pipelining correctness, could the pipeline executor always force reads on the first stage? That way we wouldn't need to require each pipeline creation point to do it.

[ericl]

@clarkzinzow fixed, this is now passing reliably and ready for review.

[clarkzinzow]

LGTM, few nits

[clarkzinzow]

Nice!

[clarkzinzow]

Note that @stephanie-wang's PR that ports stage task launching to a threadpool will also fix this by launching all tasks from the driver, and she'll have to resolve some merge conflicts here. #21845

[ericl]

This might be an alternative to the thread pools as well, though it's not tolerant of the actor failure.

[ericl]

Separately, @stephanie-wang seems to me that we could deadlock in reconstruction if the stages end up overlapping. We might need to re-think the actor pool implementation to avoid this (i.e., actor workers need to release resources if blocked on fetching data).

[clarkzinzow]

@ericl Great point!

[ericl]

On second thought, reconstruction isn't really supported with actor pools anyways, so perhaps we just need to make this clear in the documentation.

[clarkzinzow]

@ericl Don't actor tasks with the max_task_retries option specified support lineage-based reconstruction in current master, where the actor tasks are assumed to be idempotent? I know that actor tasks are not technically in scope for Q1 lineage reconstruction work, but I thought that the outstanding work was just an improved API and that the Datasets actor pool could possibly leverage the existing support.

[ericl]

@clarkzinzow I don't think actors support reconstruction at all in any case.

[clarkzinzow]

@ericl Lineage reconstruction doc suggests that what I indicated above is the case, unless I'm missing something.

Actor tasks with the decorator max_task_retries are automatically retried if the actor dies. Currently, if lineage reconstruction is enabled, actors with this decorator may also get resubmitted tasks, even if the actor has not died, due to a downstream object being reconstructed. The assumption is that these tasks are idempotent so it is safe to re-execute them.

Not sure if normal tasks submitted by actor methods would be covered at the end of Q1 work, however (multiple owners/borrowed refs in lineage).

https://docs.google.com/document/d/1LVk6JFGmdgxzKOoR8lz90TBAIDhJTcsY2dkoCRRLSR8/edit?usp=drivesdk

[ericl]

Makes sense, but it wouldn't apply here since the actors are destroyed after stage execution.

[clarkzinzow]

Hmm so the actors are destroyed once the executor goes out of scope, so this would be an issue for rewindowing, repeating, or splitting a pipeline. Makes sense.

[bveeramani]

‼️ ACTION REQUIRED ‼️

We've switched our code formatter from YAPF to Black (see #21311).

To prevent issues with merging your code, here's what you'll need to do:

1. Install Black

pip install -I black==21.12b0

2. Format changed files with Black

curl -o format-changed.sh https://gist.githubusercontent.com/bveeramani/42ef0e9e387b755a8a735b084af976f2/raw/7631276790765d555c423b8db2b679fd957b984a/format-changed.sh
chmod +x ./format-changed.sh
./format-changed.sh
rm format-changed.sh

3. Commit your changes.

git add --all
git commit -m "Format Python code with Black"

4. Merge master into your branch.

git pull upstream master

5. Resolve merge conflicts (if necessary).

After running these steps, you'll have the updated format.sh.