Making Segmented Ref Profiles upload as ZIP #1469
Conversation
- This PR intends to use songbird's latest Header to generate the URL on logAsync, which will be used to write potentially all kinds of profiles in the future - It also extracts some methods out of WhyLabsWriter.write to make it easier to understand
murilommen
force-pushed
the
dev/murilommen/zip-segmented-reference-with-songbird
branch
from
f5001a9
to
1fa4953
Compare
| @@ -475,6 +478,27 @@ def get_writables(self) -> Optional[List[Writable]]: | |||
| ) | |||
| return results | |||
|
|
|||
| def in_memory_zip(self) -> Optional[bytes]: | |||
There was a problem hiding this comment.
Might be too big for in memory?
There was a problem hiding this comment.
True, in case we're dealing with a gigantic SegmentedResultSet, which can contain multiple SegmentedDatasetProfiles with a number of columns.
I'll turn it into a static method on WhyLabsWriter then, so other types of Writables can leverage it
There was a problem hiding this comment.
it will be an in-memory zipped file, that will be persisted to tmp_dir. same logic as before, only extracted out to its own method
murilommen
force-pushed
the
dev/murilommen/zip-segmented-reference-with-songbird
branch
from
7979c21
to
4acbefc
Compare
murilommen
force-pushed
the
dev/murilommen/zip-segmented-reference-with-songbird
branch
from
239f922
to
d54d30e
Compare
| @patch("whylogs.api.writer.whylabs.WhyLabsWriter._do_upload", return_value=(True, "Success")) | ||
| @patch("whylogs.api.writer.whylabs.WhyLabsWriter._get_dataset_timestamp", return_value=1234567890) | ||
| @patch("whylogs.api.writer.whylabs.WhyLabsWriter._upload_zipped_files", return_value=(True, "Success")) | ||
| def test_write_segmented_reference_result_set( |
There was a problem hiding this comment.
I would probably leave this test out... it won't survive the refactoring.
There was a problem hiding this comment.
this "all mock" test has saved me some trouble, especially to understand exactly what are the method's calls. I'd vote for leaving it and just changing the patches -- or deleting it -- after the refactor
| @@ -448,6 +481,15 @@ def tag_custom_performance_column( | |||
| ) | |||
| return False, str(e) | |||
|
|
|||
| def _tag_custom_perf_metrics(self, view: Union[DatasetProfileView, SegmentedDatasetProfileView]) -> None: | |||
There was a problem hiding this comment.
Perhaps this should handle SegmentedDatasetProfileView or not be Union?
There was a problem hiding this comment.
it is called within _get_uncompounded_view, which is a part of the flow for DatasetProfileView and SegmentedDatasetProfileView. but inside of it, it makes an isinstance check for DatasetProfileView only. very confusing, so I didn't want to modify it. out of the scope of this PR
murilommen
force-pushed
the
dev/murilommen/zip-segmented-reference-with-songbird
branch
from
5a4ec3e
to
1a80dfd
Compare
|
Closing because an alternative PR was merged |
This PR intends to use songbird's latest Header to generate the URL on logAsync, which will be used to write potentially all kinds of profiles in the future
It also extracts some methods out of WhyLabsWriter.write to make it easier to understand
I have reviewed the Guidelines for Contributing and the Code of Conduct.