Add Google Drive and AWS S3 as a Remote Storage option #503

cs7-shrey · 2025-03-27T20:23:13Z

Description

This pull requests is a first version of Google Drive and AWS S3 integration in the datashuttle API and TUI.

What is this PR

Bug fix
Addition of a new feature
Other

Why is this PR needed?

What does this PR do?
This PR is to discuss the approach for implementing Google Drive and AWS S3 as a remote storage option.

References

Issue #407

How has this PR been tested?

This PR was manually tested in the Datashuttle's Python API and TUI. Comprehensive tests are yet to be written.

Is this a breaking change?

No

Does this PR require an update to the documentation?

Yes

Checklist:

The code has been tested locally
Tests have been added to cover all new functionality
The documentation has been updated to reflect any changes
The code has been formatted with pre-commit

cs7-shrey · 2025-04-02T12:48:52Z

Hey @adamltyson @JoeZiminski, I know reviewing this PR wouldn't be feasible at the moment. But it would be great if you can try it out locally whenever you have time and let me know if it works correctly / any suggestions you'd have. No hurry at all, I understand if it takes a bit longer.

JoeZiminski

Hey @cs7-shrey thanks a lot for this, this is excellent. It works really well and the code is structured neatly and clearly. Please see comments for some suggestions / questions / general comments.

The only thought I had on the user-end was whether we can let them provide a specific google-drive folder (rather than giving access to all of their drive folders). This might be a little more secure and avoid us accidentally interfering with other folders on their drive. The user could pass the google drive folder ID and this could be used for connection. We will have to make it clear that central_path now means relative to the root of the folder. It would also be nice to do this for AWS, but I think it is less simple and not in keeping with the way that AWS buckets works (but if it is possible, that would be great).

Please let me know if you have any questions!

JoeZiminski · 2025-05-22T18:52:10Z

datashuttle/configs/canonical_configs.py

@@ -39,9 +40,16 @@ def get_canonical_configs() -> dict:
    canonical_configs = {
        "local_path": Union[str, Path],
        "central_path": Optional[Union[str, Path]],
-        "connection_method": Optional[Literal["ssh", "local_filesystem"]],
+        "connection_method": Optional[
+            Literal["ssh", "local_filesystem", "gdrive", "aws_s3"]


I think "aws" as a name will be sufficient (?) as S3 will cover nearly all cases we will encounter (if we add other aws types, we can extend the name for those types only)

JoeZiminski · 2025-05-22T18:54:08Z

datashuttle/configs/canonical_configs.py

        "central_host_id": Optional[str],
        "central_host_username": Optional[str],
+        "gdrive_client_id": Optional[str],


This is good and extend the existing approach, similar for the checks. Down the line we may want to refactor this, we can create an issue to think about it (maybe there is a way to simplfy it as a user-facing API). For now however, let's proceed with this structure as it is simple, and refactoring this might mean changing a lot of internal things related to configs.

JoeZiminski · 2025-05-22T18:59:49Z

datashuttle/configs/canonical_configs.py

@@ -128,6 +136,32 @@ def check_dict_values_raise_on_fail(config_dict: Configs) -> None:
            ConfigError,
        )

+    # Check gdrive settings
+    elif config_dict["connection_method"] == "gdrive" and (


In this case, is it possible be set to gdrive but supply neither gdrive_client_id nor gdrive_client_secret?

JoeZiminski · 2025-05-22T19:19:03Z

datashuttle/configs/regions.py

+    US_GOV_WEST_1 = "us-gov-west-1"
+
+    @classmethod
+    def get_all_regions(cls):


This is cool and nice use of class and class method, it is cleaner than a dictionary. I think however there is a subtly here that means it would be preferable to define this as a dictionary and return it from a function (there can be two functions, get_canonical_regions() or get_all_region_names() : return list(get_canonical_regions().keys())). One of the reasons is that the getter function is slightly cleaner as it does not have to workaround __-prefixed instances of the class, but this is negligible.

There is a more subtle issue around scope, for this class the scope becomes global across the entire application, meaning that if one of the regions is accidentally changed somewhere (not that it ever should be) it will propagate across the application. For example, if you create a file called mod1.py1:

class myMod: attr=1

and in a second file mod2.py:

from mod1 import myMod import mod1 myMod.attr = 2 print(myMod.attr) print(mod1.myMod.attr) # prints 2, 2

you will see that the attribute value is changed to 2 in both modules. If we return a dict from a function, the returned value is instantiated at call time and so will not be global i.e. in mod1.py:

def get_my_mod(): myMod = {"attr": 1} return myMod

and in mod2.py:

import mod1 from mod1 import get_my_mod myMod1 = get_my_mod() myMod1["attr"] = 2 print(myMod1["attr"]) print(mod1.get_my_mod()["attr"]) # prints 2, 1

JoeZiminski · 2025-05-22T19:19:36Z

datashuttle/configs/regions.py

@@ -0,0 +1,35 @@
+class AWS_REGION:


I think this module can be called aws_regions.py just for added clarity

JoeZiminski · 2025-05-23T18:45:25Z

datashuttle/utils/gdrive.py

+from datashuttle.utils import rclone, utils
+
+
+def preliminary_for_setup_without_browser(


This is really great, giving the option to connect without browser

JoeZiminski · 2025-05-23T18:51:21Z

datashuttle/utils/gdrive.py

+def preliminary_for_setup_without_browser(
+    cfg: Configs, rclone_config_name: str, log: bool = True
+):
+    client_id_key_value = (


Same note here as for in rclone, let's handle this at a high level and assume that these are at least None in the config.

JoeZiminski · 2025-05-23T19:01:50Z

datashuttle/utils/gdrive.py

+        if cfg.get("gdrive_client_secret", None)
+        else ""
+    )
+    output = rclone.call_rclone(


Some of these arguments to the rclone call are a little mysterious, a short comment above would be useful just to explain to readers what is going on. Of course, its always possible to check the rclone docs, but in general it is more efficient to leave a short comment to give the pertinent details.

JoeZiminski · 2025-05-23T19:02:01Z

datashuttle/utils/gdrive.py

+        pipe_std=True,
+    )
+
+    # TODO: make this more robust


JoeZiminski · 2025-05-23T19:02:59Z

datashuttle/utils/gdrive.py

+        answer = False
+
+    if log:
+        utils.log(message)


Can the answer also be logged here?

shrey added 10 commits March 25, 2025 19:10

google drive setup via python api first draft

6f08b47

enable google drive config setup via tui

6131331

minor compatibility and ui changes

c47f2d1

protectedclient secret input box

3e845e1

google drive connection setup via TUI

d7f8f48

add aws as remote storage via python api first draft

0fd532d

add: logging and connection check for aws s3

184d8dc

update: type checking for aws regions

d5b1e3c

add: save aws configs via TUI

2690f1a

add: setup aws connection via TUI

63f6d71

cs7-shrey changed the title ~~Add Google Drive as a Remote Storage option~~ Add Google Drive and AWS S3 as a Remote Storage option Mar 30, 2025

shrey added 4 commits April 1, 2025 03:59

feat: setup google drive on machines with no browser

5c572d3

fix: minor bug

8931dce

fix: logical error

717fd21

add: logging for google drive connections

5990e6b

JoeZiminski requested changes May 23, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add Google Drive and AWS S3 as a Remote Storage option #503

Add Google Drive and AWS S3 as a Remote Storage option #503

Uh oh!

cs7-shrey commented Mar 27, 2025 •

edited

Loading

Uh oh!

cs7-shrey commented Apr 2, 2025

Uh oh!

JoeZiminski left a comment

Uh oh!

JoeZiminski May 22, 2025 •

edited

Loading

Uh oh!

JoeZiminski May 22, 2025

Uh oh!

JoeZiminski May 22, 2025

Uh oh!

JoeZiminski May 22, 2025

Uh oh!

JoeZiminski May 22, 2025

Uh oh!

JoeZiminski May 23, 2025

Uh oh!

JoeZiminski May 23, 2025

Uh oh!

JoeZiminski May 23, 2025

Uh oh!

JoeZiminski May 23, 2025

Uh oh!

JoeZiminski May 23, 2025

Uh oh!

Uh oh!

		from datashuttle.utils import rclone, utils


		def preliminary_for_setup_without_browser(

Add Google Drive and AWS S3 as a Remote Storage option #503

Are you sure you want to change the base?

Add Google Drive and AWS S3 as a Remote Storage option #503

Uh oh!

Conversation

cs7-shrey commented Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

References

How has this PR been tested?

Is this a breaking change?

Does this PR require an update to the documentation?

Checklist:

Uh oh!

cs7-shrey commented Apr 2, 2025

Uh oh!

JoeZiminski left a comment

Choose a reason for hiding this comment

Uh oh!

JoeZiminski May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cs7-shrey commented Mar 27, 2025 •

edited

Loading

JoeZiminski May 22, 2025 •

edited

Loading