-
Notifications
You must be signed in to change notification settings - Fork 68
[PLT-X] Make export_v2 methods use streamable backend #1515
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
Changes from all commits
791edb3
6096995
02aa1ed
525424c
fa638f5
4ed5ddd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -467,7 +467,8 @@ class ExportTask: | |
| class ExportTaskException(Exception): | ||
| """Raised when the task is not ready yet.""" | ||
|
|
||
| def __init__(self, task: Task) -> None: | ||
| def __init__(self, task: Task, is_export_v2: bool = False) -> None: | ||
| self._is_export_v2 = is_export_v2 | ||
|
Contributor
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. I assume we need to explicitly pass it in because of the api feature flag that might change streamable to export_v2?
Contributor
Author
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. This value comes down from API though. The flow is as follows:
|
||
| self._task = task | ||
|
|
||
| def __repr__(self): | ||
|
|
@@ -530,9 +531,79 @@ def metadata(self): | |
| """Returns the metadata of the task.""" | ||
| return self._task.metadata | ||
|
|
||
| @property | ||
| def result_url(self): | ||
| """Returns the result URL of the task.""" | ||
| if not self._is_export_v2: | ||
| raise ExportTask.ExportTaskException( | ||
| "This property is only available for export_v2 tasks due to compatibility reasons, please use streamable errors instead" | ||
| ) | ||
| base_url = self._task.client.rest_endpoint | ||
| return base_url + '/export-results/' + self._task.uid + '/' + self._task.client.get_organization( | ||
| ).uid | ||
|
|
||
| @property | ||
| def errors_url(self): | ||
| """Returns the errors URL of the task.""" | ||
| if not self._is_export_v2: | ||
| raise ExportTask.ExportTaskException( | ||
| "This property is only available for export_v2 tasks due to compatibility reasons, please use streamable errors instead" | ||
| ) | ||
| base_url = self._task.client.rest_endpoint | ||
| return base_url + '/export-errors/' + self._task.uid + '/' + self._task.client.get_organization( | ||
| ).uid | ||
|
|
||
| @property | ||
| def errors(self): | ||
| """Returns the errors of the task.""" | ||
| if not self._is_export_v2: | ||
| raise ExportTask.ExportTaskException( | ||
| "This property is only available for export_v2 tasks due to compatibility reasons, please use streamable errors instead" | ||
| ) | ||
| if self.status == "FAILED": | ||
| raise ExportTask.ExportTaskException("Task failed") | ||
| if self.status != "COMPLETE": | ||
| raise ExportTask.ExportTaskException("Task is not ready yet") | ||
|
|
||
| if not self.has_errors(): | ||
| return None | ||
|
|
||
| data = [] | ||
|
|
||
| metadata_header = ExportTask._get_metadata_header( | ||
| self._task.client, self._task.uid, StreamType.ERRORS) | ||
| if metadata_header is None: | ||
| return None | ||
| Stream( | ||
| _TaskContext(self._task.client, self._task.uid, StreamType.ERRORS, | ||
| metadata_header), | ||
| _MultiGCSFileReader(), | ||
| JsonConverter(), | ||
| ).start(stream_handler=lambda output: data.append(output.json_str)) | ||
| return data | ||
|
|
||
| @property | ||
| def result(self): | ||
| """Returns the result of the task.""" | ||
| if self._is_export_v2: | ||
| if self.status == "FAILED": | ||
| raise ExportTask.ExportTaskException("Task failed") | ||
| if self.status != "COMPLETE": | ||
| raise ExportTask.ExportTaskException("Task is not ready yet") | ||
| data = [] | ||
|
|
||
| metadata_header = ExportTask._get_metadata_header( | ||
| self._task.client, self._task.uid, StreamType.RESULT) | ||
| if metadata_header is None: | ||
| return [] | ||
| Stream( | ||
| _TaskContext(self._task.client, self._task.uid, | ||
| StreamType.RESULT, metadata_header), | ||
| _MultiGCSFileReader(), | ||
| JsonConverter(), | ||
| ).start(stream_handler=lambda output: data.append( | ||
| json.loads(output.json_str))) | ||
| return data | ||
| return self._task.result_url | ||
|
|
||
| @property | ||
|
|
||
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.
In this PR we return a union or a tuple in several places, is it possible to avoid that ? Here for instance I can't tell the difference between
TaskandExportTask. Also we end up not using one of the two return values in other places.Uh oh!
There was an error while loading. Please reload this page.
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.
This PR modifies
ExportTaskso that it is a superset ofTaskand behaves similarly. We cannot know what is being returned until we toggle the flag for all customers. We'll be able to narrow down the type in future version (once flag is enabled for everyone).