Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compute and Transfer Example Flows #1

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

MaxTueckeGlobus
Copy link

@MaxTueckeGlobus MaxTueckeGlobus commented Feb 28, 2025

Created two example Compute and Transfer flows and a script to register the Compute function both examples invoke.

  • Example flow 1 takes a user-provided source file that already exists in the co-located GCS collection, creates a tarfile from it, and transfers the tarfile to a user provided destination collection.
  • Example flow 2 takes in a user provided list source files that exists on a user provided source collection, creates a tarfile from it, and transfers the tarfile to a user provided destination collection.

@MaxTueckeGlobus MaxTueckeGlobus marked this pull request as ready for review February 28, 2025 21:52
Copy link
Member

@kurtmckee kurtmckee left a comment

Choose a reason for hiding this comment

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

I'm leaving initial feedback about the names and descriptions for a number of items. I'll come back to review the Python/JSON code after you've had a chance to review/respond. 👍

Comment on lines 30 to 31
| `transform_from` | The path prefix to replace (default: "/") |
| `transform_to` | The prefix to use for absolute paths (default: "/") |
Copy link
Member

Choose a reason for hiding this comment

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

These names are not communicating their purpose (to me, at least). After reading the example scenario below, it seems like it might be worthwhile to rename these.

If I'm understanding the example, the GCS collection maps its root to /path/to/root/ on the filesystem. Would gcs_base_path (which uses the naming from the GCS globus-connect-server collection create command) be equivalent to transform_to here?

For transform_from, I'm not yet clear what its purpose is. I will likely understand after reading more, but since I'm not clear after reading the text here, I think this needs to be clarified but I don't yet have suggestions.

Copy link
Author

Choose a reason for hiding this comment

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

To be honest, I'm not entirely sure. The transform_from transform_to parameters were something I had initially lifted from a comment made by Lei on how he setup a do_tar function that could handle the path mappings. I was assuming in my head there was a potential collection config that could require the GCS collection's base path mapping to something other than '/', but re-looking at the GCS docs, I'm not seeing one. @LeiGlobus unless there is something I'm missing here, I'm inclined to go with your suggestion of gcs_base_path and remove transform_from as a parameter.

Choose a reason for hiding this comment

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

The context: when I first wrote that function 2 weeks ago (the tar one), I tried to make it generic but also wasn't clear on what the terminology in GCS was regarding the "prefix" paths. So thought that a 'from' and 'to' prefix mapping name in the function was fine.

Now that I think about it, as this is an example for people to use to interface with GCS, a name more in line with GCS is more clear in terms of its usage. (what Kurt suggested below base_path makes sense).

So it's something like gcs_base_path_from and gcs_base_path_to? For mapping to a path without any base paths, then only one side is necessary to be specified.

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.

4 participants