Making Storage syncing explicit and moving it to SYNC_FILE_MOUNTS stage

[romilbhardwaj]

Context

In the current implementation of sky.Storage, uploading of local files ("syncing") to a Storage is an implicit operation when the Storage is initialized. That is, we force a sync whenever the Storage.add_store() method is called. This is undesirable because:

1. It forces a sync when the Storage object is created, not when it's actually used. E.g., sky launch examples/storage_demo.yaml would first sync the local directories with the s3 stores, even before the confirmation prompt is shown to the user. If the user cancels the launch, the file upload is wasted.

2. It forces us to execute a sync when the storage object is reconstructed from global user state, even when the sync may not be required. In turn, we have implemented complex flags as a knee-jerk reaction, which pollutes our API. For example, when the Storage object is reconstructed for deletion via sky storage delete, we have to add the sync_on_reconstruction=False argument to force it to not sync. @Michaelvll also rightly brought this up in his review of force_managed field for Storage #992.
3. It couples add_store with syncing, which I now think should be two distinct operations.

Proposal

I propose we make syncing an explicit operation that must be called by the Storage object creator. We already have the Storage.sync_all_stores() method - the creator can call this method to execute a file upload. For example:

Current implementation

s = sky.Storage(name='my-dataset-bucket', source='~/datasets`)
s.add_store(StoreType.S3) # This line also upload the source

Proposal

s = sky.Storage(name='my-dataset-bucket', source='~/datasets`)
s.add_store(StoreType.S3) # Does not execute upload.
s.sync_all_stores()

This allows us to do the file upload in the SYNC_FILE_MOUNTS stage, while add_store can still be invoked earlier by the .from_yaml_config or __init__ methods.

Thoughts? cc @Michaelvll @michaelzhiluo

[concretevitamin]

+1. Making that API lazy (or lazier) is more in line with other parts of our system and dataflow systems.

[Michaelvll]

I like the proposed method. I was confused by the add_store function several times before. A potential problem is that it can be a bit tricky for the following situation:

s = sky.Storage(name='my-dataset-bucket', source='~/datasets`)
s.add_store(StoreType.S3) # Does not execute upload.
s.sync_all_stores()
# Adding another store after sync
s.add_store(StoreType.GCS) # Does not execute upload.
# Will re-upload all the files again to S3
s.sync_all_stores()

There are two possible solutions:

1. Add a function sync_store(StoreType.S3) so that we can only sync to the S3. However, this can make different StoreType have different versions of the files if the local file changes.
2. Another solution is changing the sync behavior to incremental instead of replacing, so that every time it only uploads the modifications. However, this may cause problem, when the user changed the source path, and the sync will mix the files from different folders in the same bucket. Probably, for each storage object, we record the source path, and do the replacing sync when the source path changed.

I think the second solution may make more sense.