Skip to content

Conversation

dakinggg
Copy link
Contributor

@dakinggg dakinggg commented Oct 8, 2022

What does this PR do?

Removes object_store_cls and instead parses out which remote backend to use given the passed in bucket uri. Also some renaming of related variables.

What issue(s) does this change relate to?

Closes CO-1187
Related to CO-1167

Manual testing

Tested s3 with kwargs

    object_store_logger = RemoteUploaderDownloader(
        remote_bucket_uri="s3://mosaicml-internal-checkpoints-test",
        remote_path_format_string="{remote_file_name}",
        remote_backend_kwargs={"bucket": "mosaicml-internal-checkpoints-test", "prefix": "daniel-test"}
    )

Tested s3 without kwargs

    object_store_logger = RemoteUploaderDownloader(
        remote_bucket_uri="s3://mosaicml-internal-checkpoints-test",
        # remote_path_format_string="daniel-test/{remote_file_name}"
    )

Tested libcloud with s3

    object_store_logger = RemoteUploaderDownloader(
        remote_bucket_uri="libcloud://mosaicml-internal-checkpoints-test",
        remote_backend_kwargs={"provider": "s3", "provider_kwargs": {
            "secret": "----",
            "key": "----",
            "region": "us-west-2"
            }}
    )

Might want to see if I can test GCS too.

@dakinggg dakinggg force-pushed the remove_object_store_cls branch from 1db6d97 to 9070fbc Compare October 10, 2022 20:11
@dakinggg dakinggg marked this pull request as ready for review October 10, 2022 21:08
@dakinggg dakinggg requested a review from eracah as a code owner October 10, 2022 21:08
@dakinggg dakinggg requested a review from mvpatel2000 October 10, 2022 21:09
@mvpatel2000
Copy link
Contributor

Mostly LGTM. Deferring approval to Evan though

Copy link
Contributor

@eracah eracah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for doing this! This will greatly simplify the checkpointing PR

@dakinggg dakinggg merged commit caf2285 into mosaicml:dev Oct 11, 2022
@dakinggg dakinggg deleted the remove_object_store_cls branch October 20, 2022 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants