[checkpoint v2] add remote uploader class

[bigning]

What does this PR do?

Add a RemoteUploader class
Compared to RemoteUploaderDownloader, it

1. removes the download API, which is not used
2. uses modern ProcessPoolExecutor which provides high-level interface for asynchronously executing callables
3. greatly simplify the logic, it doesn't use any queues.
4. it's neither a logger nor a callback, so it's very light weight and standalone component. Instead, it's caller's responsibility to wait the upload to finish
5. much easier to maintain and add more features, since it's simpler

test

unit test

pytest tests/utils/test_remote_uploader.py

e2e test

see PR #3320

[mvpatel2000]

To avoid race conditions with symlink and checkpoints, we do num_workers=1. Should we just remove multi worker functionality (remove public arg)?

Otherwise, @eracah does the redesign have a way to fix this?

[eracah]

To avoid race conditions with symlink and checkpoints, we do num_workers=1. Should we just remove multi worker functionality (remove public arg)?

Otherwise, @eracah does the redesign have a way to fix this?

it will fix this because we will upload the symlink synchronously after the file(s) in the upload_worker

[bigning]

To avoid race conditions with symlink and checkpoints, we do num_workers=1. Should we just remove multi worker functionality (remove public arg)?
Otherwise, @eracah does the redesign have a way to fix this?

it will fix this because we will upload the symlink synchronously after the file(s) in the upload_worker

This version just does exact same thing as existing remote_uploader_downloader.

[eracah]

This is a good start, @bigning ! I think it could be a lot simpler if we move to a design where on each upload_file_async call a new executor.submit call is called. This would make it even simpler because we wouldn't need a file queue or an is_closed_event. See the design doc implementation section for more details

[eracah]

To avoid race conditions with symlink and checkpoints, we do num_workers=1. Should we just remove multi worker functionality (remove public arg)?
Otherwise, @eracah does the redesign have a way to fix this?

it will fix this because we will upload the symlink synchronously after the file(s) in the upload_worker

This version just does exact same thing as existing remote_uploader_downloader.

Right, but in the design doc, I designed a feature where the symlink gets uploaded synchronously after the file. Is that intended for a follow-up PR?

[bigning]

This is a good start, @bigning ! I think it could be a lot simpler if we move to a design where on each upload_file_async call a new executor.submit call is called. This would make it even simpler because we wouldn't need a file queue or an is_closed_event. See the design doc implementation section for more details

@eracah , that's actually my initial implementation (look at this), but it doesn't work unless we create a new object_store everytime we call upload_file. This is because, with mutli-process, it needs to pickle the input to the function, the object_store is not pickable. Does that make sense ?

[b-chu]

I'll leave the reviewing to others but adding myself to cc

[bigning]

To avoid race conditions with symlink and checkpoints, we do num_workers=1. Should we just remove multi worker functionality (remove public arg)?
Otherwise, @eracah does the redesign have a way to fix this?

it will fix this because we will upload the symlink synchronously after the file(s) in the upload_worker

This version just does exact same thing as existing remote_uploader_downloader.

Right, but in the design doc, I designed a feature where the symlink gets uploaded synchronously after the file. Is that intended for a follow-up PR?

discussed offline, this is a separate PR to add the new checkpoint saver

[eracah]

LGTM!