Nodeports/add file checksum #2011

sanderegg · 2020-11-30T17:10:14Z

What do these changes do?

This PR will add the S3 ETag in the payload of project/comp_tasks such that:

Nodeports package v2

based on pydantic
on upload sets the S3 ETag in comp_tasks table
this will trigger an automatic /retrieve call on the next service even if the filename does not change
sidecar uses the node_ports v2

NOTE: nodeports v1 is deprecated, stop using it!

if a file with the same name is uploaded, the ETag will change, triggering a new download
should allow the ControlCore use case with any dynamic service

NOTE: download is not yet prevented if the ETag is the same. this will come later.

Bonus:

timing the download/upload of files through node_ports_v2 using tqdm (available in the logs, but would be nice in the frontend log as well)
.5 fix of the frontend progressbar

Related issue number

refering to PR #1999

How to test

Checklist

Did you change any service's API? Then make sure to bundle document and upgrade version (make openapi-specs, git commit ... and then make version-*)
Unit tests for the changes exist
Runs in the swarm
Documentation reflects the changes
New module? Add your github username to .github/CODEOWNERS

codecov · 2020-11-30T17:11:35Z

Codecov Report

Merging #2011 (904ca55) into master (c7d97b5) will increase coverage by 0.7%.
The diff coverage is 94.6%.

@@           Coverage Diff            @@
##           master   #2011     +/-   ##
========================================
+ Coverage    72.6%   73.4%   +0.7%     
========================================
  Files         405     413      +8     
  Lines       14846   15097    +251     
  Branches     1503    1526     +23     
========================================
+ Hits        10785   11084    +299     
+ Misses       3664    3620     -44     
+ Partials      397     393      -4

Flag	Coverage Δ
integrationtests	`64.4% <86.0%> (+1.0%)`	⬆️
unittests	`67.6% <85.6%> (+0.9%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
packages/simcore-sdk/src/simcore_sdk/config/db.py	`0.0% <0.0%> (ø)`
.../simcore-sdk/src/simcore_sdk/node_data/__init__.py	`100.0% <ø> (ø)`
...s/simcore-sdk/src/simcore_sdk/node_ports/config.py	`100.0% <ø> (ø)`
...re-sdk/src/simcore_sdk/node_ports/serialization.py	`81.5% <ø> (ø)`
...es/sidecar/src/simcore_service_sidecar/executor.py	`79.6% <69.5%> (ø)`
...core-sdk/src/simcore_sdk/node_ports/filemanager.py	`80.2% <77.7%> (+2.5%)`	⬆️
...re-sdk/src/simcore_sdk/node_ports_v2/port_utils.py	`94.7% <94.7%> (ø)`
...ls-library/src/models_library/projects_nodes_io.py	`100.0% <100.0%> (ø)`
...ages/models-library/src/models_library/services.py	`97.2% <100.0%> (ø)`
packages/simcore-sdk/src/simcore_sdk/__init__.py	`100.0% <100.0%> (ø)`
... and 49 more

pcrespov

Generally very nice work in v2!! Added some requests for your consideration.

Regarding your note in the description: Should add some

warnings.warn(
              
                category=DeprecationWarning,
            )

in simcore_skd to avoid

.pre-commit-config.yaml

pcrespov · 2020-12-08T22:24:00Z

packages/models-library/src/models_library/services.py

@@ -151,7 +161,7 @@ class ServiceProperty(BaseModel):
        description="Place the data associated with the named keys in files",
        examples=[{"dir/input1.txt": "key_1", "dir33/input2.txt": "key2"}],
    )
-    default_value: Optional[Union[str, float, bool, int]] = Field(
+    default_value: Optional[Union[StrictBool, StrictInt, StrictFloat, str]] = Field(
        None, alias="defaultValue", examples=["Dog", True]


So with the stricts you ensure that "3" or "True" -> str but 3 -> int and True -> bool??

so first:
very important is to use the ordering in the Union: go for most restricted values to less restricted (e.g. bool -> int -> float -> str).
Then the strict means that 0 is not False (which could be an issue in some cases but not here yet), also 3 is an int but 3.0 isn't.

packages/simcore-sdk/src/simcore_sdk/node_ports/_data_item.py

packages/pytest-simcore/src/pytest_simcore/services_api_mocks_for_aiohttp_clients.py

pcrespov · 2020-12-08T22:39:58Z

packages/simcore-sdk/src/simcore_sdk/node_ports/filemanager.py

-                await file_pointer.write(chunk)
+        file_size = int(response.headers.get("content-length", 0)) or None
+
+        with tqdm(


very nice! I find it very fancy for consoles but how good is to send log info between machines?

not sure I get what you mean?

packages/simcore-sdk/src/simcore_sdk/node_ports_v2/port.py

pcrespov · 2020-12-08T23:15:38Z

packages/simcore-sdk/src/simcore_sdk/node_ports_v2/port_utils.py

+
+        file_path = data_items_utils.create_file_path(key, file_name)
+        if value == file_path:
+            # this can happen in case


in case ... what? :-)

good catch. that was a copy paste from node_ports_v1.. which I will correct as well.

ok so I cannot change in the node_ports_v1 cause that is the very file that makes black hook fail with error 123... and I don't know how to make black skip it but by changing the pre-commit-config.yml file.

packages/simcore-sdk/src/simcore_sdk/node_ports_v2/ports_mapping.py

packages/simcore-sdk/src/simcore_sdk/node_ports_v2/serialization_v2.py

packages/simcore-sdk/tests/unit/test_nodeports_v2.py

pcrespov

Actually my comments before are not so critical as to request changes. They do not actually need re-review.

GitHK

Very nice job.

I have expressed some concerns regarding network errors (which can happen for different reasons like: timeouts, errors during file upload and download etc...)
Please try to figure out if they are valid and might have an impact and if it is possible to handle them gracefully to avoid future errors or bugs.

GitHK · 2020-12-09T07:41:20Z

packages/simcore-sdk/src/simcore_sdk/node_ports/filemanager.py

-            chunk = await response.content.read(CHUNK_SIZE)
-            while chunk:
-                await file_pointer.write(chunk)
+        file_size = int(response.headers.get("content-length", 0)) or None


basically only when content-length==0 because when undefined(missing) is always 0. I would just remove the or None part.

packages/simcore-sdk/src/simcore_sdk/node_ports/filemanager.py

packages/simcore-sdk/src/simcore_sdk/node_ports_v2/nodeports_v2.py

packages/simcore-sdk/src/simcore_sdk/node_ports_v2/port_utils.py

packages/simcore-sdk/src/simcore_sdk/node_ports_v2/port.py

packages/simcore-sdk/src/simcore_sdk/node_ports_v2/ports_mapping.py

packages/simcore-sdk/src/simcore_sdk/node_ports/__init__.py

packages/simcore-sdk/src/simcore_sdk/node_ports_v2/__init__.py

packages/simcore-sdk/tests/integration/test_nodeports2.py

services/sidecar/src/simcore_service_sidecar/executor.py

GitHK

👍 Thank you!

fixing small issues

sanderegg added a:webserver issue related to the webserver service a:sidecar issue related with the sidecar worker service a:pipeline-services issues on non-core services in user's pipeline (use osparc-services repo for specifics) a:api framework api, data schemas, labels Nov 30, 2020

sanderegg added this to the Alfred_Büchi milestone Nov 30, 2020

sanderegg self-assigned this Nov 30, 2020

sanderegg force-pushed the nodeports/add_file_checksum branch 7 times, most recently from ad26f5b to eeb26d0 Compare December 8, 2020 14:05

sanderegg requested review from pcrespov, GitHK, mguidon and odeimaiz December 8, 2020 14:08

sanderegg changed the title ~~WIP: Nodeports/add file checksum~~ Nodeports/add file checksum Dec 8, 2020

sanderegg marked this pull request as ready for review December 8, 2020 14:10

pcrespov requested changes Dec 8, 2020

View reviewed changes

pcrespov approved these changes Dec 8, 2020

View reviewed changes

sanderegg force-pushed the nodeports/add_file_checksum branch from af65f98 to 8743ae8 Compare December 9, 2020 08:30

GitHK reviewed Dec 9, 2020

View reviewed changes

sanderegg mentioned this pull request Dec 9, 2020

node_ports should have retry policies when upload/download fails #2035

Closed

sanderegg requested a review from GitHK December 9, 2020 10:17

GitHK approved these changes Dec 9, 2020

View reviewed changes

sanderegg force-pushed the nodeports/add_file_checksum branch from 288798b to cfd5817 Compare December 11, 2020 08:24

sanderegg added 2 commits December 14, 2020 15:00

ensure storage is in debug mode when in devel

61d983a

format

13f3f2c

sanderegg added 26 commits December 14, 2020 15:03

improve testing

8294c70

add ETag

a5f9434

added ETag when uploading file

c0500fe

fix test related to uploaded file and received eTag

6420568

cleaning up

160fd04

fixing small issues

add eTag into project json schema

f910562

bonus: fix progress already in percent

32a65f2

switch sidecar to node ports v2

04a82a1

add tqdm to time download

e1a3703

time download upload using tqdm

3825467

linter

c0124df

add bumpversion needed files

208d1a5

bumped version to 0.3.0

c1cce6a

fix bad merge

36473c9

@pcrespov review: add deprecation warnings

70814a2

@pcrespov review: remove unnecessary exclusion

a64afa4

@pcrespov review: rename mock fixtures from subsystem to service

c166688

@pcrespov review: missing comments

5136eda

@pcrespov review: use dump/load names

8043977

@pcrespov review: typo

031c322

@GitHK review: remove useless return

7aa4361

@GitHK review: check if while reading something bad happens

ec4601a

@GitHK review: re-enabling assertion

0dbe66f

@GitHK review: removed unnecessary nullhandler for library

7295f33

added a bit of debug logs

d8fe51c

fixed sidecar test by using both versions of node ports

898c9c5

sanderegg force-pushed the nodeports/add_file_checksum branch from cfd5817 to 898c9c5 Compare December 14, 2020 14:05

bad merge

904ca55

sanderegg merged commit 888cf9e into ITISFoundation:master Dec 14, 2020

sanderegg deleted the nodeports/add_file_checksum branch December 14, 2020 14:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nodeports/add file checksum #2011

Nodeports/add file checksum #2011

sanderegg commented Nov 30, 2020 •

edited

Loading

codecov bot commented Nov 30, 2020 •

edited

Loading

pcrespov left a comment

pcrespov Dec 8, 2020

sanderegg Dec 9, 2020

pcrespov Dec 8, 2020

sanderegg Dec 9, 2020

pcrespov Dec 8, 2020

sanderegg Dec 9, 2020

sanderegg Dec 9, 2020

pcrespov left a comment

GitHK left a comment •

edited

Loading

GitHK Dec 9, 2020

GitHK left a comment

Nodeports/add file checksum #2011

Nodeports/add file checksum #2011

Conversation

sanderegg commented Nov 30, 2020 • edited Loading

What do these changes do?

Related issue number

How to test

Checklist

codecov bot commented Nov 30, 2020 • edited Loading

Codecov Report

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

GitHK left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GitHK left a comment

Choose a reason for hiding this comment

sanderegg commented Nov 30, 2020 •

edited

Loading

codecov bot commented Nov 30, 2020 •

edited

Loading

GitHK left a comment •

edited

Loading