Skip to content

Compute Transfer examples for transfers that require a flow#13

Merged
MaxTueckeGlobus merged 4 commits intomainfrom
gcs-transfer-requires-flow
Apr 28, 2025
Merged

Compute Transfer examples for transfers that require a flow#13
MaxTueckeGlobus merged 4 commits intomainfrom
gcs-transfer-requires-flow

Conversation

@MaxTueckeGlobus
Copy link
Contributor

@MaxTueckeGlobus MaxTueckeGlobus commented Apr 16, 2025

sc-40468

For GCS collection transfer actions that require a flow, added a variation for each compute transfer example flow.

Example 1

Example 1 is for transferring files from the shared GCS compute endpoint. The shared GCS compute collection should have in its associated_flow_policy that this flow is required when it is the source of a transfer. It can take an optional destination path to save the tar file to. If it is not provided, the flow will use the random UUID from the compute function as the output destination path. The flow will then behave the same as its counterpart; creating a tarball from the src files, and transferring said tarball to the destination collection.

Example 2

Example 2 is meant for transferring files between two collections. The destination collection should have in its associated_flow_policy that this flow is required when it is the destination of a transfer. It can take an optional destination path to save the tar file to. If it is not provided, the flow will use the random UUID from the compute function as the output destination path. The flow behaves the same as its counterpart; transferring the source files into the shared GCS compute collection, creating a tarball from them, and then transferring said tarball to the specified destination.

Both flows were tested on sandbox and prod with the new webapp behavior for automatically starting a flow when trying to start a transfer from the file manager.

Copy link
Member

@sirosen sirosen left a comment

Choose a reason for hiding this comment

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

I only have feedback on the first example so far, but as addressing it may also impact the second example I decided to post early.

These need READMEs, since the only place we currently have descriptive text about what they are and what they do is in the PR description. I'd prefer that we add them as asciidoc files, so that we have fewer conversions to do if we want to integrate these examples into the docs.globus.org publishing pipeline in the future.
Even stubby readmes -- equivalent to the current PR text -- are a very significant step up from no readme.

@MaxTueckeGlobus MaxTueckeGlobus force-pushed the gcs-transfer-requires-flow branch from e361287 to 0af64c5 Compare April 18, 2025 17:28
@MaxTueckeGlobus
Copy link
Contributor Author

These need READMEs, since the only place we currently have descriptive text about what they are and what they do is in the PR description. I'd prefer that we add them as asciidoc files, so that we have fewer conversions to do if we want to integrate these examples into the docs.globus.org publishing pipeline in the future. Even stubby readmes -- equivalent to the current PR text -- are a very significant step up from no readme.

I have some drafted READMEs. Once we are happy with the flow definitions, I'll update them as needed and add them.

Copy link
Member

@sirosen sirosen left a comment

Choose a reason for hiding this comment

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

I think the first example is in pretty good shape -- certainly good enough for a first draft -- although I have some inline notes on more doc which is needed.

For the second example, I don't think I can formulate good feedback without understanding the motivation behind some of what I see in there.

"GetDestinationPath": {
"Type": "ExpressionEval",
"Parameters": {
"path.=": "getattr('destination_path', '/~/' + pathsplit(compute_result.details.result[0])[1])"
Copy link
Member

Choose a reason for hiding this comment

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

I'm flying blind a little since I don't know what the proposed function for this example does. I find it matters most when trying to double check that the output is being used correctly here. Could we include a .py file with just the relevant function?
(I'd prefer to omit any Compute registration code and just document the function itself.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the same compute function as the original tar and transfer examples. I wasn't sure if we wanted to include the file again

Copy link
Member

Choose a reason for hiding this comment

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

I would vote for including it again, or at least providing a readme which references it in the other existing example.

I'd be better able to comment on what the right approach is if we had a readme for these examples -- I know you said you have drafts, but I think not having them as part of the proposal is impeding review.

For example, suppose I'm looking at a limitation of the example flow. If the readme says "NOTE: this example has this limitation...", then I know it's intentional. And if it's not, I can suggest that we add such a note and make it intentional. Without that documentation, it's very hard to read intent from the code itself, or to grasp these as full examples -- they're incomplete without the accompanying docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The draft docs have been added. I have yet to verify they render correctly.

@MaxTueckeGlobus MaxTueckeGlobus force-pushed the gcs-transfer-requires-flow branch from 919bd56 to 0baf5df Compare April 23, 2025 20:34
@ada-globus
Copy link
Collaborator

@MaxTueckeGlobus Looks like this hit a failure in pre-commit (codespell flagged a typo in "initialization")

Copy link
Member

@sirosen sirosen left a comment

Choose a reason for hiding this comment

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

Looks great! I had a very minor tweak to asciidoc rendering I noted.

I'm not sure we will want to keep all of the examples long-term, but this is good and we can refine in the future.

Co-authored-by: Stephen Rosen <sirosen@globus.org>
@MaxTueckeGlobus MaxTueckeGlobus merged commit 5954236 into main Apr 28, 2025
2 checks passed
@MaxTueckeGlobus MaxTueckeGlobus deleted the gcs-transfer-requires-flow branch April 28, 2025 19:25
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