Clean up of unpack_remotedata()

[madsbk]

Separate the handling of futures with the argument of a SubgraphCallable and futures within the SubgraphCallable itself.

Closes #7289
Closes #7303

• Tests added / passed
• Passes pre-commit run --all-files

[github-actions]

Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

       15 files  ±  0         15 suites  ±0   6h 44m 21s ⏱️ - 15m 51s
  3 226 tests +  5    3 141 ✔️ +  4    84 💤 ±0  1 ❌ +1 
23 852 runs  +35  22 942 ✔️ +34  909 💤 ±0  1 ❌ +1 

For more details on these failures, see this check.

Results for commit dd7479a. ± Comparison against base commit 69b74f9.

♻️ This comment has been updated with latest results.

[mrocklin]

This does appear to fix the issue that I was seeing. I'm curious, what is your understanding of the problem and what did you do here to fix it?

[madsbk]

This does appear to fix the issue that I was seeing. I'm curious, what is your understanding of the problem and what did you do here to fix it?

We need to do two things when unpacking futures surrounding SubgraphCallable:

• We have to unpack all futures in the graph of the SubgraphCallable and add them to the input keys of the SubgraphCallable.
• Also, we have to unpack futures in the arguments given to the SubgraphCallable. Since they are not used within the SubgraphCallable itself, they do not need to be added to the input keys of the SubgraphCallable, which is what we use to do.

[mrocklin]

@fjetter is this something that you and your team can help to review?

[mrocklin]

Also I hope that eventually something like #6028 makes things like this irrelevant

[fjetter]

Changes to the code appear to be straightforward. Can we add a test for #7289 please? It sounds like we should be able to test this by asserting that the object is not in the output multiple times, shouldn't we?

[madsbk]

Changes to the code appear to be straightforward. Can we add a test for #7289 please? It sounds like we should be able to test this by asserting that the object is not in the output multiple times, shouldn't we?

@mrocklin do you have a small reproducer? Otherwise, I can try to make a synthetic one.

[mrocklin]

I don't have anything handy, no. This came up when using Datashader. I didn't do work to find something smaller unfortunately.

…

On Mon, Nov 21, 2022 at 11:56 AM Mads R. B. Kristensen < ***@***.***> wrote: Changes to the code appear to be straightforward. Can we add a test for #7289 <#7289> please? It sounds like we should be able to test this by asserting that the object is not in the output multiple times, shouldn't we? @mrocklin <https://github.com/mrocklin> do you have a small reproducer? Otherwise, I can try to make a synthetic one. — Reply to this email directly, view it on GitHub <#7322 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACKZTAUP3YBGUK7ZRTPTH3WJOZWNANCNFSM6AAAAAASDINDV4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[madsbk]

@fjetter added a test: dd7479a

[fjetter]

Thanks @madsbk !