✨ Expose multipart upload #63

bisgaard-itis · 2023-08-18T09:44:29Z

What do these changes do?

Update openapi.json to the latest version from https://github.com/ITISFoundation/osparc-simcore master
Expose multipart upload and introduce e2e test which uploads a 10 GB file.
Introduce async generator for the pagination iterator and add this to e2e test

Related issue/s

closes fix bug in API that limits to 2GB osparc-simcore#3263
closes 5Gb limit osparc-issues#617
closes OverflowError in public API osparc-simcore#2520

How to test

e2e tests: https://git.speag.com/bisgaard/e2e-portal-testing/-/pipelines/543186

bisgaard-itis · 2023-08-18T10:26:34Z

clients/python/test/e2e/test_files_api.py

+    Version(osparc.__version__) < Version("0.6.0"),
+    reason=f"osparc.__version__={osparc.__version__} is older than 0.6.0",
+)
+def test_upload_file(tmp_path: Path, cfg: osparc.Configuration) -> None:


@pcrespov should I skip this file until we have the delete operation? As it is it uploads a 10GB file.

yes please.

pcrespov

Great job!
Some style/minor comments

clients/python/test/e2e/test_files_api.py

pcrespov · 2023-08-21T07:46:52Z

clients/python/test/e2e/test_files_api.py

+    Version(osparc.__version__) < Version("0.6.0"),
+    reason=f"osparc.__version__={osparc.__version__} is older than 0.6.0",
+)
+def test_upload_file(tmp_path: Path, cfg: osparc.Configuration) -> None:


yes please.

clients/python/test/e2e/test_files_api.py

pcrespov · 2023-08-21T07:53:52Z

clients/python/test/e2e/test_files_api.py

+    with osparc.ApiClient(cfg) as api_client:
+        files_api: osparc.FilesApi = osparc.FilesApi(api_client=api_client)
+        uploaded_file: osparc.File = files_api.upload_file(tmp_file)
+        downloaded_file = files_api.download_file(uploaded_file.id)


THOUGHT: it would be nice to have an optional parameter in files_api.download_file to set where I want to download the file.

def download_file(self, file_id, *, destination_folder: Path | None = None) -> Path: ...

Would it be easy to do that in the client? This way the user can avoid copy/moving things around.

and the test would be

downloaded_file = files_api.download_file(uploaded_file.id, destination_folder = tmp_path) assert downloaded_file.parent == temp_path

Sure, I will add this functionality to the client and add the assert to the test

clients/python/client/osparc/_utils.py

clients/python/client/osparc/_http_client.py

pcrespov · 2023-08-21T08:14:25Z

clients/python/test/e2e/test_solvers_api.py

@@ -53,6 +38,9 @@ def test_get_jobs(configuration: osparc.Configuration):
        tmp_iter = solvers_api.get_jobs(


MIONR: is solvers_api.get_jobs called like that? Listing jobs should be called solvers_api.list_jobs. All listings are by convention called that way.

MINOR: I find interesting the naming tmp. A generation is indeed something of one use but the idea is that the use does not even feel it, i.e. the user would write something like

for job in solvers_api.list_jobs(...): print(job) # do something with job

OK, I can call it list_jobs if you want. But for me "list" is a loaded term in Python. I would expect such a method to return me a list (i.e. it seems to me to violate the principle of lease surprise 😉).

for job in solvers_api.list_jobs(...): print(job) # do something with job

works now. However more than that one can also do len(solvers_api.list_jobs(...)) to get the number of elements the iterator can produce. I find that quite useful. That's why I used te tmp_...-thing in the test. To get an object I could call len on.

pcrespov · 2023-08-21T08:15:49Z

clients/python/test/e2e/test_solvers_api.py

@@ -63,6 +51,17 @@ def test_get_jobs(configuration: osparc.Configuration):
        for elm in tmp_iter:
            assert isinstance(elm, osparc.Job)

+        async def return_iter_content(


I get this but perhaps you can write a separate async test for clarity?
both tests should like the same except that in one case is asyng test_ and the other is not

I have deleted the async part of the iterator for now. Reason: since I use the links I get from the first call to the api directly, the iterator now needs to hold its own http client. Hence, to have a sync and an async version it makes sense to have two different types of iterators: One holding a sync client and one holding an async client. Until someone tells me we need the async one I will stick with the sync one.

pcrespov

some comments

pcrespov · 2023-08-24T12:49:02Z

clients/python/client/osparc/_files_api.py

+        client_file: ClientFile = ClientFile(
+            filename=file.name, filesize=file.stat().st_size
+        )
+        client_upload_schema: ClientFileUploadSchema = self._super.get_upload_links(


MINOR: In reality client_upload_schema is NOT a schema but data. Note that pydantic class builds the schema but the instance is a data object. For that reason I try avoiding the word Schema in the classes. This should be modified in the server as well.

BTW, we try to follow a naming convention for request payloads and response bodies. We use the anem of the resource and then a verb at the end. For instance, ProjectGet would be the response body for a GET /project/{id} resource

OK, concerning the schema thing I will address this among other small things I need to do on the API server: ITISFoundation/osparc-simcore#4641

pcrespov · 2023-08-24T12:50:46Z

clients/python/client/osparc/_files_api.py

+                f"destination_folder: {destination_folder} must be a directory"
+            )
+        downloaded_file: Path = Path(super().download_file(file_id))
+        if destination_folder is not None:


why not using https://docs.python.org/3/library/tempfile.html#tempfile.mkdtemp if `destination_folder is not given?

The downloaded file is already located in the temporary directory by the automatically generated client. So that shouldn't be necessary.

pcrespov · 2023-08-24T12:51:40Z

clients/python/client/osparc/_files_api.py

+
+        tasks: list = []
+        async with AsyncHttpClient(
+            exc_req_typ="post", exc_url=links.abort_upload, exc_auth=self._auth


you really like writing acronyms :-) ... do not be shy. You can write long names. what is exc here?

Oops, sorry. This should already have been fixed

pcrespov · 2023-08-24T12:53:11Z

clients/python/test/e2e/test_solvers_api.py

 @pytest.mark.skipif(
    Version(osparc.__version__) < Version("0.6.0"),
    reason=f"osparc.__version__={osparc.__version__} is older than 0.6.0",
 )
-def test_get_jobs(configuration: osparc.Configuration):
-    """Test the get_jobs method
+def test_list_jobs(cfg: osparc.Configuration):


why cfg is better than configuration :-) ? these mathematicians ;-)

This fixture has to be used every where. So I am simply trying to save some screen-ink here.

pcrespov · 2023-08-24T12:57:01Z

clients/python/test/e2e/test_solvers_api.py

        solvers_api: osparc.SolversApi = osparc.SolversApi(api_client)
        sleeper: osparc.Solver = solvers_api.get_solver_release(solver, version)

        # initial iterator
-        init_iter = solvers_api.get_jobs(sleeper.id, sleeper.version, limit=3)
+        init_iter = solvers_api.list_jobs(sleeper.id, sleeper.version)


I thought you did not want to prefix it as list_!
I would like the operation ID to be called list since in the API these are denoted List method https://cloud.google.com/apis/design/standard_methods#list but feel free to map it to e.g. solvers_api.iter_jobs or simply solvers.jobs

OK, I like solvers_api.jobs. I will go for that one

pcrespov · 2023-08-24T12:59:53Z

clients/python/test/e2e/test_solvers_api.py


-        final_iter = solvers_api.get_jobs(sleeper.id, sleeper.version, limit=3)
+        final_iter = solvers_api.list_jobs(sleeper.id, sleeper.version)
        assert len(final_iter) > 0, "No jobs were available"


if final_iter is an iterator ... then len() should not work. Probably this return a list.?

final_iter is a PaginationGenerator. I decided to create that intermediary class instead of directly returning an iterator in order for it to have a __len__ method. However, PaginationGenerator has an __iter__ method, so

for elm in final_iter: ...

works

bisgaard-itis added 12 commits August 16, 2023 10:12

add new openapi.json

f95ac9e

start adding wrapper for files api

1fb6277

first implementation of async fileupload

8508c0c

use complete/upload link directly

f3c5791

add initial test for uploading 10gb file

c177c3f

minor correction

d2b0376

update openapi.json as well as upload fcn and test

98e2e4c

factor out completion request

08acb53

cleanup

a9aeb28

add final exception handling to http client

581297e

update tests

c2fb0ca

skip test if not right osparc version

b2fe544

bisgaard-itis added the enhancement New feature or request label Aug 18, 2023

bisgaard-itis added this to the Baklava milestone Aug 18, 2023

bisgaard-itis self-assigned this Aug 18, 2023

bisgaard-itis requested a review from pcrespov as a code owner August 18, 2023 09:44

tuple -> Tuple in hints to make py 3.6 happy

fd42d3a

bisgaard-itis commented Aug 18, 2023

View reviewed changes

bisgaard-itis added 3 commits August 18, 2023 12:31

list[] -> List[] in hints

b37a4ac

change initialization

393a662

minor correction

a80a3ae

pcrespov approved these changes Aug 21, 2023

View reviewed changes

bisgaard-itis added 7 commits August 23, 2023 13:18

start using httpx in order to also have sync client

fa6fbce

make pagination iterator sync for now

60bc124

several small changes to iterator and test

90cff10

remove aiohttp error handler

1f4e918

small bug fix

4ca5480

implement destination_folder input in download_file

f092cce

several small changes according to PR feedback

8aad099

bisgaard-itis requested a review from pcrespov August 24, 2023 12:19

pcrespov approved these changes Aug 24, 2023

View reviewed changes

bisgaard-itis added 6 commits August 25, 2023 08:02

list_jobs -> jobs

d9bcb74

merge master into expose-multipart-upload

19d02f0

add junit test suite name

7600bf4

use junit prefix instead

d7f19eb

fix addopts

dd17c44

fix prefix

237775a

bisgaard-itis merged commit ad0c87c into ITISFoundation:master Aug 25, 2023

bisgaard-itis deleted the expose-multipart-upload branch August 25, 2023 06:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Expose multipart upload #63

✨ Expose multipart upload #63

bisgaard-itis commented Aug 18, 2023 •

edited

Loading

bisgaard-itis Aug 18, 2023

pcrespov Aug 21, 2023

bisgaard-itis Aug 24, 2023

pcrespov left a comment

pcrespov Aug 21, 2023

pcrespov Aug 21, 2023

bisgaard-itis Aug 24, 2023

pcrespov Aug 21, 2023

bisgaard-itis Aug 24, 2023

pcrespov Aug 21, 2023

bisgaard-itis Aug 24, 2023

pcrespov left a comment

pcrespov Aug 24, 2023

bisgaard-itis Aug 24, 2023

pcrespov Aug 24, 2023

bisgaard-itis Aug 24, 2023

pcrespov Aug 24, 2023

bisgaard-itis Aug 24, 2023

bisgaard-itis Aug 24, 2023

pcrespov Aug 24, 2023

bisgaard-itis Aug 24, 2023

pcrespov Aug 24, 2023

bisgaard-itis Aug 25, 2023

pcrespov Aug 24, 2023

bisgaard-itis Aug 24, 2023

		@@ -53,6 +38,9 @@ def test_get_jobs(configuration: osparc.Configuration):
		tmp_iter = solvers_api.get_jobs(

✨ Expose multipart upload #63

✨ Expose multipart upload #63

Conversation

bisgaard-itis commented Aug 18, 2023 • edited Loading

What do these changes do?

Related issue/s

How to test

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcrespov left a 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

pcrespov left a 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

bisgaard-itis commented Aug 18, 2023 •

edited

Loading