datasetRw: add dataset upload file capabilities to domino-python (multi-part, multi-thread) #185

ddl-giuliocapolino · 2024-01-03T03:48:04Z

Link to JIRA

What issue does this pull request solve?

Needed a new endpoint to handle dataset upload

What is the solution?

Add endpoint as well as full client implementation + tests for single file, multi-part, multi-thread uploads

Testing

Briefly describe how the change was tested. The purpose of this section is that a reviewer can identify a test gap, if any.

e.g. "I ran an upgrade from 4.2 to 4.6".

Unit test(s)

Pull Request Reminders

Has relevant documentation been updated?
Does the code follow [Python Style Guide] (https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html)
Update the changelog
Are the existing unit tests still passing?
Have new unit tests been added to cover any changes to the code?
Has the JIRA ticket(s) been linked above?

References (optional)

ddl-gabrielhaim · 2024-01-04T22:05:22Z

domino/datasets.py

+UPLOAD_READ_TIMEOUT_IN_SEC = 30
+
+
+class UploadChunk:


seems to be a good use case for a dataclass https://docs.python.org/3/library/dataclasses.html

ddl-gabrielhaim · 2024-01-04T22:29:10Z

domino/datasets.py

+        url = self.routes.datasets_end_upload(self.dataset_id, self.upload_key, self.target_relative_path)
+        return self.request_manager.get(url)
+
+    def _create_chunk_queue(self) -> [UploadChunk]:


I think you have to use list[UploadChunk] for proper type hinting here

ddl-gabrielhaim · 2024-01-04T22:30:55Z

domino/domino.py

+        )
+
+        try:
+            uploader.start_upload_session()


I feel like you could easily make this a context manager https://www.pythonmorsels.com/creating-a-context-manager/ to avoid having the user of this class know which method to call first (since there are no arguments once created)

ddl-gabrielhaim · 2024-01-04T22:32:42Z

domino/datasets.py

+            return self._create_chunk(self.local_path_file_or_directory)
+        else:


python like early return, no need for else here

ddl-gabrielhaim · 2024-01-04T22:34:15Z

domino/datasets.py

+                    # append chunk to queue
+                    nested_q.append(self._create_chunk(relative_path_to_file))
+            # flattening list of chunks
+            return [chunk for chunks in nested_q for chunk in chunks]


WAT is happening here 😆 . Can you name things differently? is there a way to flatten this in the previous for loop?

ddl-gabrielhaim · 2024-01-04T22:36:09Z

domino/datasets.py

+        if should_upload:
+            upload_try = 1
+            uploaded = False
+            while upload_try <= MAX_UPLOAD_ATTEMPTS and not uploaded:


big fan of making good use a decorator for this kind of logic https://pypi.org/project/retry/. This allow clearer code by making the function do only what it is supposed to do

ddl-gabrielhaim · 2024-01-04T22:37:37Z

domino/datasets.py

+        self.upload_key = upload_key
+
+
+class Uploader:


see my comment about making this a context manager class

ddl-eric-jin · 2024-01-04T23:01:37Z

tests/test_datasets.py

+
+    assert response.status_code == 200
+    assert "filecache" in response.json()
+q


Is this q supposed to be here?

haha no!! thanks :)

ddl-eric-jin · 2024-01-04T23:02:29Z

domino/datasets.py

+                except Exception:
+                    if upload_try > MAX_UPLOAD_ATTEMPTS:
+                        raise Exception(f"Uploading chunk {chunk.chunk_number} of {chunk.total_chunks} "
+                                           f"for {chunk.file_name} failed. Please try again")


Nit: Can add period after "Please try again"

ddl-eric-jin · 2024-01-04T23:04:00Z

domino/routes.py

+    def datasets_end_upload(self, dataset_id, upload_key, target_relative_path=None):
+        url = self.host + f"/v4/datasetrw/datasets/{dataset_id}/snapshot/file/end/{upload_key}"
+        if target_relative_path:
+            url += f"?targetRelativePath={target_relative_path}"


ddl-eric-jin · 2024-01-04T23:08:38Z

domino/datasets.py

+        self.relative_path = relative_path
+        self.target_chunk_size = target_chunk_size
+        self.total_chunks = total_chunks
+        self.upload_key = upload_key


Small nit: Might be clearer to match the order or the arguments of __init__ with the order the variables are assigned.

ddl-eric-jin · 2024-01-04T23:11:12Z

domino/datasets.py

+                except Exception:
+                    if upload_try > MAX_UPLOAD_ATTEMPTS:
+                        raise Exception(f"Uploading chunk {chunk.chunk_number} of {chunk.total_chunks} "
+                                           f"for {chunk.file_name} failed. Please try again")


Nit: Could add period at the end of message.

ddl-gabrielhaim · 2024-01-05T23:23:24Z

domino/datasets.py

+    def __init__(
+            self,
+            absolute_path: str,
+            chunk_number: int,
+            dataset_id: str,
+            file_name: str,
+            file_size: int,
+            identifier: str,
+            relative_path: str,
+            target_chunk_size: int,
+            total_chunks: int,
+            upload_key: str,
+    ):


you don't need the __init__ in a dataclass (that was the goal of my comment)

ddl-gabrielhaim · 2024-01-05T23:23:59Z

domino/datasets.py

+            raise RuntimeError(f"upload key for {self.dataset_id} not found. Session could not start.")
+        return self
+
+    def __exit__(self, exc_type, exc_val, exc_tb):


you might want to check on the exc_type to cancel the upload session in the event an exception was raised? This way you handle the whole lifecycle of the upload here instead of leaving that to the user

ddl-gabrielhaim · 2024-01-05T23:27:02Z

domino/domino.py

+            except ValueError:
+                self.log.error(f"Erroneous input parameter. Please fix the parameters and try again")
+            except Exception as e:
+                self.log.error(f"Upload for dataset {dataset_id} and file {local_path_to_file_or_directory} failed, "
+                               f"canceling session. Please try again.")
+                uploader.cancel_upload_session()


see previous comment, want to avoid that here

ddl-olsonJD · 2024-01-10T16:47:39Z

tests/test_datasets.py

+@pytest.mark.skipif(
+    not domino_is_reachable(), reason="No access to a live Domino deployment"
+)
+def test_datasets_upload(default_domino_client):


Please add some more test cases.

ddl-gabrielhaim · 2024-01-10T17:16:00Z

domino/datasets.py

+        except Exception as e:
+            self.log.error("Upload failed. See error for details.")
+            raise e


I don't think you need to use try..catch here since it gets captured in the exit

ddl-gabrielhaim · 2024-01-10T17:17:08Z

domino/datasets.py

+        if not self.upload_key:
+            raise RuntimeError(f"upload key for {self.dataset_id} not found. Could not end session.")


you might want to check the exception before this since an error can happen before the upload_key is set

ddl-gabrielhaim · 2024-01-10T17:17:25Z

domino/datasets.py

+        # catching errors
+        if not self.upload_key:
+            raise RuntimeError(f"upload key for {self.dataset_id} not found. Could not end session.")
+        if exc_type != None:


if exc_type is not None is the python way of checking null values

ddl-gabrielhaim · 2024-01-10T17:20:14Z

domino/datasets.py

+                           f"attempting to cancel session. Please try again.")
+            self.log.error(f"Error type: {exc_val}. Error message: {exc_tb}.")
+            if not isinstance(exc_type, ValueError):
+                self._cancel_upload_session() # is it is a ValueError, canceling session would fail


what happen if this raise an exception? afaik __exit__ must return True or False https://docs.python.org/3/library/contextlib.html#contextlib.ContextDecorator

it raises it! modifying below.

ddl-gabrielhaim · 2024-01-10T17:21:18Z

domino/datasets.py

+            self.log.info("Upload session cancelled successfully.")
+        except Exception as e:
+            self.log.info("Cancelling upload session failed. See error for details.")
+            raise e


probably want to prevent the exception from being re-raise (see previous comment)

ddl-gabrielhaim · 2024-01-10T17:23:38Z

domino/domino.py

+            target_chunk_size=target_chunk_size,
+            target_relative_path=target_relative_path
+        ) as path:
+            if path:


in what scenario is path not evaluating to true?

ah previously I was logging “Upload complete” here and was trying to find a way around the possibility of the ending failing - this is a remnant of that. good catch!

ddl-gabrielhaim

Approving but couple of comments that we might want to iterate on a future PR.

ddl-gabrielhaim · 2024-01-16T16:03:53Z

domino/datasets.py

+        self.upload_key = self.request_manager.post(start_upload_url, json=start_upload_body).json()
+        if not self.upload_key:
+            raise RuntimeError(f"upload key for {self.dataset_id} not found. Session could not start.")
+        return self._upload()


not sure whether you should let the user (user of the context manager) initiate the upload or not, seems more correct to let them do it in case they want to do some logging before and after.

ddl-gabrielhaim · 2024-01-16T16:04:18Z

domino/datasets.py

+        except Exception as e:
+            self.log.error("Ending snapshot upload failed. See error for details. Attempting to cancel "
+                           "upload session.")
+            self._cancel_upload_session()


see previous comment, the exit method should return true/false

ZeroCool2u · 2024-03-20T15:17:04Z

Big hype, this seems like it closes #171! Thank you!

ddl-giuliocapolino added 6 commits January 2, 2024 22:47

add skeleton for upload file

fc60bdb

various improvements

a650cd2

chkpt1 - working solution

01b8d62

chkpt2 - decoupled solution with commented working solution + tests

ca197a7

chkpt3 - removing commented solution and nit fixes

d2b7976

chkpt4 - ensuring completion before end upload

e48bef8

ddl-giuliocapolino force-pushed the ddl-giuliocapolino.dataset-file-upload branch from c76f0fa to e48bef8 Compare January 4, 2024 19:22

ddl-giuliocapolino changed the title ~~datasetRw: add dataset upload file capabilities to domino-python~~ datasetRw: add dataset upload file capabilities to domino-python (multi-part, multi-thread) Jan 4, 2024

ddl-giuliocapolino marked this pull request as ready for review January 4, 2024 19:24

ddl-giuliocapolino requested a review from a team January 4, 2024 19:25

ddl-giuliocapolino added 3 commits January 4, 2024 16:24

fixing test

d469051

adding subfolder input capability and fixing cancel upload

ee645ac

adding support for directory uploads

ea10969

ddl-gabrielhaim reviewed Jan 4, 2024

View reviewed changes

ddl-eric-jin reviewed Jan 4, 2024

View reviewed changes

iteration on comments

741cfb1

ddl-gabrielhaim reviewed Jan 5, 2024

View reviewed changes

further iteration on comments

f3cf99e

ddl-olsonJD reviewed Jan 10, 2024

View reviewed changes

ddl-gabrielhaim reviewed Jan 10, 2024

View reviewed changes

ddl-giuliocapolino added 3 commits January 11, 2024 20:42

cleanup on error flows

3538ffa

more tests

953dc8c

add README

9312358

ddl-gabrielhaim approved these changes Jan 16, 2024

View reviewed changes

final iteration on comments

37eff04

ddl-giuliocapolino merged commit 6f501c0 into master Jan 16, 2024

ZeroCool2u mentioned this pull request Mar 20, 2024

Add upload_file_to_dataset() method #171

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

datasetRw: add dataset upload file capabilities to domino-python (multi-part, multi-thread) #185

datasetRw: add dataset upload file capabilities to domino-python (multi-part, multi-thread) #185

ddl-giuliocapolino commented Jan 3, 2024 •

edited

Loading

ddl-gabrielhaim Jan 4, 2024

ddl-gabrielhaim Jan 4, 2024

ddl-gabrielhaim Jan 4, 2024

ddl-gabrielhaim Jan 4, 2024

ddl-gabrielhaim Jan 4, 2024

ddl-gabrielhaim Jan 4, 2024

ddl-gabrielhaim Jan 4, 2024

ddl-eric-jin Jan 4, 2024

ddl-giuliocapolino Jan 5, 2024

ddl-eric-jin Jan 4, 2024

ddl-eric-jin Jan 4, 2024

ddl-eric-jin Jan 4, 2024 •

edited

Loading

ddl-eric-jin Jan 4, 2024

ddl-gabrielhaim Jan 5, 2024

ddl-gabrielhaim Jan 5, 2024

ddl-giuliocapolino Jan 7, 2024

ddl-gabrielhaim Jan 5, 2024

ddl-olsonJD Jan 10, 2024

ddl-gabrielhaim Jan 10, 2024

ddl-gabrielhaim Jan 10, 2024

ddl-gabrielhaim Jan 10, 2024

ddl-gabrielhaim Jan 10, 2024

ddl-giuliocapolino Jan 11, 2024

ddl-gabrielhaim Jan 10, 2024

ddl-gabrielhaim Jan 10, 2024

ddl-giuliocapolino Jan 11, 2024

ddl-gabrielhaim left a comment

ddl-gabrielhaim Jan 16, 2024

ddl-gabrielhaim Jan 16, 2024

ZeroCool2u commented Mar 20, 2024

		return self._create_chunk(self.local_path_file_or_directory)
		else:

		if not self.upload_key:
		raise RuntimeError(f"upload key for {self.dataset_id} not found. Could not end session.")

datasetRw: add dataset upload file capabilities to domino-python (multi-part, multi-thread) #185

datasetRw: add dataset upload file capabilities to domino-python (multi-part, multi-thread) #185

Conversation

ddl-giuliocapolino commented Jan 3, 2024 • edited Loading

Link to JIRA

What issue does this pull request solve?

What is the solution?

Testing

Pull Request Reminders

References (optional)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddl-eric-jin Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddl-gabrielhaim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZeroCool2u commented Mar 20, 2024

ddl-giuliocapolino commented Jan 3, 2024 •

edited

Loading

ddl-eric-jin Jan 4, 2024 •

edited

Loading