Round robin during spread scheduling

[jjyao]

Why are these changes needed?

This PR does round robin over all the nodes instead of always starting from the beginning of the node list for spread scheduling.

Related issue number

Checks

• I've run scripts/format.sh to lint the changes in this PR.
• I've included any doc changes needed for https://docs.ray.io/en/master/.
• I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
• Testing Strategy
  • Unit tests
  • Release tests
  • This PR is not tested :(

[rkooo567]

Nice change!

[jjyao]

pipelined_ingestion_1500_gb_15_windows succeeded without prefix hack: https://buildkite.com/ray-project/periodic-ci/builds/1902#_

[ericl]

How's the before/after time? And time without spread hint?

…

On Thu, Dec 9, 2021, 7:18 PM Jiajun Yao ***@***.***> wrote: pipelined_ingestion_1500_gb_15_windows succeeded without prefix hack: https://buildkite.com/ray-project/periodic-ci/builds/1902#_ — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#19968 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAADUSXJ54ZSURT5E2ATOX3UQFWQZANCNFSM5HFOSUIQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[jjyao]

Without prefix, without spread scheduling strategy: Forever (hang)
With prefix: ~33m
With spread scheduling strategy: ~47m

@scv119 can confirm this.

[ericl]

Any idea why it's still 50% slower? Btw, .stats() can show you the node distribution though it might not be complete for shuffle.

…

On Thu, Dec 9, 2021, 7:46 PM Jiajun Yao ***@***.***> wrote: Without prefix, without spread scheduling strategy: Forever (hang) With prefix: ~33m With spread scheduling strategy: ~47m @scv119 <https://github.com/scv119> can confirm this. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#19968 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAADUSUBSTQ3FNQQHOCPNZ3UQFZYTANCNFSM5HFOSUIQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[jjyao]

I'm trying to figure out the difference now. This is a good validation use case for dataset stats and scheduler observability projects.

[rkooo567]

LGTM for cpp part. I will wait for Eric's approval since I didn't review the dataset part.

Only one nit comment

[clarkzinzow]

Datasets side looks good, a few comments about the Ray side.

[jjyao]

Not sure if there is anything changed or last run was just noise: I ran pipelined_ingestion_1500_gb_15_windows three times with this PR and the time is almost the same now:
https://buildkite.com/ray-project/periodic-ci/builds/2059#_
https://buildkite.com/ray-project/periodic-ci/builds/2057
https://buildkite.com/ray-project/periodic-ci/builds/2054

[jjyao]

It's ready to be merged. Perf number looks good: https://buildkite.com/ray-project/periodic-ci/builds/2086#_

[jjyao]

Oh, I think for latests runs, I ran the tests against master which still had the prefix code in pipelined_training.py. Let me revert the PR to fix pipelined_ingestion_1500_gb_15_windows.