-
Notifications
You must be signed in to change notification settings - Fork 305
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
[FlyteClient][FlyteDeck] Get Downloaded Artifact Signed URL via Data Proxy #2777
Merged
Future-Outlier
merged 10 commits into
master
from
implement-create-download-link-request-response
Oct 4, 2024
Merged
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
4549fc8
[FlyteDeck] add get_download_deck_signed_url for FlyteClient
Future-Outlier 3bcc8cd
update comment
Future-Outlier 19e7678
nit
Future-Outlier 473a8f8
lint
Future-Outlier 09fb72a
Add Integration Tests
Future-Outlier 6ff792b
lint
Future-Outlier 0ab198e
better prefix deck url
Future-Outlier 3b3bb1d
lint
Future-Outlier ef1b961
Remove org argument
Future-Outlier 6e8e0b2
Update Eduardo's advice
Future-Outlier File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -13,7 +13,9 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
from flyteidl.admin import task_pb2 as _task_pb2 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
from flyteidl.admin import workflow_attributes_pb2 as _workflow_attributes_pb2 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
from flyteidl.admin import workflow_pb2 as _workflow_pb2 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
from flyteidl.core import identifier_pb2 as _identifier_pb2 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
from flyteidl.service import dataproxy_pb2 as _data_proxy_pb2 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
from flyteidl.service.dataproxy_pb2 import ARTIFACT_TYPE_DECK | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
from google.protobuf.duration_pb2 import Duration | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
from flytekit.clients.raw import RawSynchronousFlyteClient as _RawSynchronousFlyteClient | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -1046,3 +1048,33 @@ def get_data(self, flyte_uri: str) -> _data_proxy_pb2.GetDataResponse: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
resp = self._dataproxy_stub.GetData(req, metadata=self._metadata) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return resp | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
def get_download_deck_signed_url( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
self, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
node_id: str, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
project: str, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
domain: str, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
name: str, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
expires_in: datetime.timedelta = None, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) -> _data_proxy_pb2.CreateDownloadLinkResponse: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
This is a new API for flyte and union cluster to get the signed url for the deck artifact. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
expires_in_pb = None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if expires_in: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
expires_in_pb = Duration() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
expires_in_pb.FromTimedelta(expires_in) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return super(SynchronousFlyteClient, self).create_download_link( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
_data_proxy_pb2.CreateDownloadLinkRequest( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
artifact_type=ARTIFACT_TYPE_DECK, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
node_execution_id=_identifier_pb2.NodeExecutionIdentifier( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
node_id=node_id, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
execution_id=_identifier_pb2.WorkflowExecutionIdentifier( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
project=project, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
domain=domain, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
name=name, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
expires_in=expires_in_pb, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we need to change the function name to
get_download_signed_url
and addArtifactType
to the input args?cc @wild-endeavor @eapolinario
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah. What if we set the default
artifact_type
toARTIFACT_TYPE_DECK
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I can do that