[Object Spilling] Clean up FS storage upon sigint for ray.init().

[rkooo567]

Why are these changes needed?

This cleans up FS storage upon sigint. It also prints the progress when it is shutting down (so that users won't be confused why their driver is not terminated quickly).

The current output looks like...

Removing remaining files that are spilled to /private/var/folders/b8/khqvq5l13d76pjwz49bw76ph0000gn/T/pytest-of-sangbincho/pytest-98/test_file_deleted_when_driver_0/spill.

Total number of files spilled: 21
Path: /private/var/folders/b8/khqvq5l13d76pjwz49bw76ph0000gn/T/pytest-of-sangbincho/pytest-98/test_file_deleted_when_driver_0/spill.
Removed [5 / 21]
Removed [9 / 21]
Removed [13 / 21]
Removed [17 / 21]

Removed: 21
Path: /private/var/folders/b8/khqvq5l13d76pjwz49bw76ph0000gn/T/pytest-of-sangbincho/pytest-98/test_file_deleted_when_driver_0/spill.

Note

• It doesn't work with ray start. Making it work complicates the design, and it is unlikely that anyone who uses ray start will have the issue that their objects are not deleted from their external storage (because we already delete objects when refs are gone out of scope).
• I found Ray doesn't stop upon Sigterm. Not sure if it was intended.
• Didn't implement S3 yet.

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 :(

[ericl]

Can we just delete the entire directory with one call?

[rkooo567]

Agree this is a simpler logic; The only concern I have is the UX is probably bad (since we cannot guarantee users will give us a directory that is solely for object spilling). What about this? I will always create a directory appended to a given spilling directory and delete that directory directly. For example,

file_system_object_spilling_config = {
    "type": "filesystem",
    "params": {
        "directory_path": "/tmp/something"
    }
}

This will create a new directory /tmp/something/ray_spilled_objects/[files] and I will delete ray_spilled_objects so that we can avoid deleting files that are not related to object spilling.

[ericl]

Yeah it sounds good. When we turn it on by default, this can be the directory inside the session dir.

…

On Sat, Jan 23, 2021, 11:36 PM SangBin Cho ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In python/ray/external_storage.py <#13649 (comment)>: > @@ -243,6 +256,43 @@ def delete_spilled_objects(self, urls: List[str]): filename = parse_url_with_offset(url.decode()).base_url os.remove(os.path.join(self.directory_path, filename)) + def destroy_external_storage(self): Agree this is a simpler logic; The only concern I have is the UX is probably bad (since we cannot guarantee users will give us a directory that is solely for object spilling). What about this? I will always create a directory appended to a given spilling directory and delete that directory directly. For example, file_system_object_spilling_config = { "type": "filesystem", "params": { "directory_path": "/tmp/something" } } This will create a new directory /tmp/something/ray_spilled_objects/[files] and I will delete ray_spilled_objects so that we can avoid deleting files that are not related to object spilling. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#13649 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAADUSVOQHJQQPRAWRRYFZTS3PEWHANCNFSM4WPRBKBA> .

[rkooo567]

It is simplified.

[rkooo567]

@ericl Found another race condition when we go to the deleting directory solution. When the directory deletion is performed, if there are still IO workers that are deleting files, it throws an exception and fails. To avoid it I added an additional logic. If you prefer just the previous solution since it also has complexity lmk.

[ericl]

Suggested change

	print("There were unexpected errors while deleting "
	logger.exception("Error cleaning up spill files")

[rkooo567]

We still want to display the traceback right?

[rkooo567]

Lmk if you'd like me to delete a traceback msgs.

[ericl]

You don't need to include the traceback; logger.exception already does this.

[rkooo567]

Oh I see. Didn't know about that. I will fix this and merge the PR.