🐛 Fix file-picker downstream service notification issues #3058

GitHK · 2022-05-20T07:08:06Z

What do these changes do?

file-picker propagated updates to downstream services due to a side effect (FIXED)
file-picker properly sends notifications when the file source changes
dynamic-sidecar properly purges directory when it downloads new data
dynamic-sidecar inputs downloads are now queued and can no longer be happening in parallel

Related issue/s

Pipeline Update Issues osparc-issues#622

How to test

create a jupyter-math:2.0.5
connect a file-picker
add a file to the file-picker
see that the contes is displayed in the inputs_1 directory
remove the file form the file-picker (reset button)
see that the inputs_1 is directory is empty
repeat again from step 3.

Checklist

Unit tests for the changes exist

codecov · 2022-05-20T07:09:40Z

Codecov Report

Merging #3058 (a0e1311) into master (4bd0ac7) will increase coverage by 5.7%.
The diff coverage is 72.2%.

@@           Coverage Diff            @@
##           master   #3058     +/-   ##
========================================
+ Coverage    75.0%   80.8%   +5.7%     
========================================
  Files         715     716      +1     
  Lines       30849   30903     +54     
  Branches     4024    4032      +8     
========================================
+ Hits        23166   24974   +1808     
+ Misses       6820    5061   -1759     
- Partials      863     868      +5

Flag	Coverage Δ
integrationtests	`66.3% <81.5%> (+0.1%)`	⬆️
unittests	`76.7% <68.6%> (+5.8%)`	⬆️

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

Impacted Files	Coverage Δ
...mcore_service_dynamic_sidecar/modules/nodeports.py	`23.5% <13.3%> (+0.9%)`	⬆️
...service_webserver/projects/projects_nodes_utils.py	`90.4% <90.4%> (ø)`
...mcore_service_webserver/projects/projects_utils.py	`84.1% <93.7%> (+33.0%)`	⬆️
...ervice_dynamic_sidecar/api/containers_extension.py	`89.1% <100.0%> (ø)`
...er/src/simcore_service_webserver/catalog_client.py	`59.3% <100.0%> (+20.8%)`	⬆️
...webserver/computation_comp_tasks_listening_task.py	`76.5% <100.0%> (-2.9%)`	⬇️
.../simcore_service_webserver/projects/projects_db.py	`92.5% <100.0%> (+1.6%)`	⬆️
...rvice_webserver/projects/projects_handlers_crud.py	`87.7% <100.0%> (+0.1%)`	⬆️
...mcore_service_webserver/garbage_collector_utils.py	`79.4% <0.0%> (-3.0%)`	⬇️
.../simcore_service_catalog/services/access_rights.py	`78.7% <0.0%> (-2.5%)`	⬇️
... and 101 more

…ed issue with parallel inputs downloads

…inks

sanderegg

great, but I have some doubts on a few things, please have a look, we can discuss these.

sanderegg · 2022-05-22T19:22:37Z

services/dynamic-sidecar/src/simcore_service_dynamic_sidecar/modules/nodeports.py

@@ -170,6 +170,76 @@ async def _get_data_from_port(port: Port) -> Tuple[Port, ItemConcreteValue]:
    return (port, ret)


+async def _download_files(
+    target_path: Path, download_tasks: Deque[Coroutine[Any, int, Any]]
+) -> Tuple[dict[str, Any], ByteSize]:


Could you create a TypeDict at least, to know what is returned?

check if you can use deque instead of Deque

you can use tuple instead of Tuple

if you bother on butting a deque here, why then not just pass a Sequence type? I mean the best and most secure/fast way is to pass a tuple because then you also pass the fact that the fct will not modify the argument, plus tuple is faster than deque... also I doubt a bit that going from list to deque brings so much of a difference... especially when downloading GBs...

I've found out we already have something similar defined OutputsDict. Will be using that.

Sorry I don't understand the suggestion.

👍

There is no life changing improvement. The deque is just a better suited for appending data than a list. The change from list to deuque brings no real life benefit. I do not agree on the use of tuples, since they are immutable ad when constructing the sequence you need to append data to it.

deque is usable for typing

services/dynamic-sidecar/src/simcore_service_dynamic_sidecar/modules/nodeports.py

sanderegg · 2022-05-22T19:25:35Z

services/dynamic-sidecar/src/simcore_service_dynamic_sidecar/modules/nodeports.py

+    return data, ByteSize(transferred_bytes)
+
+
+@run_sequentially_in_context()
 async def download_target_ports(
    port_type_name: PortTypeName, target_path: Path, port_keys: List[str]


use list[str]

Will decide that if I remove Dict or List from a function in a module from now on, I will also remove it from the entire module.
@pcrespov could we agree on this?

services/web/server/src/simcore_service_webserver/projects/projects_db.py

sanderegg · 2022-05-22T19:30:41Z

services/web/server/src/simcore_service_webserver/projects/projects_db.py

+    def _cast_outputs_store(dict_data: dict[str, Any]) -> None:
+        for data in dict_data.get("outputs", {}).values():
+            if "store" in data:
+                data["store"] = f"{data['store']}"


Minor: actually I think I would prefer going to int than to string... especially since the day it is fixed it will be an int for sure.

That can work as well. String was selected since is the safer type cast between the two.

sanderegg · 2022-05-22T19:32:41Z

services/web/server/src/simcore_service_webserver/projects/projects_db.py

@@ -724,6 +811,10 @@ def _update_workbench(
                    .returning(literal_column("*"))
                )
                project: RowProxy = await result.fetchone()
+
+                for node_update_task in nodes_update_tasks:
+                    fire_and_forget_task(node_update_task)


did you check what happens if you have computational services running and stuff like that? you do not end up in a loop?

Yes, I've checked, and sorry for the bad naming. This is only for frontend services. And for now of all the frontend services only the file picker will be used. I've changed the names to reflect this.

Computational services are not influences by this.

services/web/server/src/simcore_service_webserver/projects/projects_db.py

sanderegg · 2022-05-22T19:39:37Z

services/web/server/tests/unit/with_dbs/02/test_project_db.py

+            id="different keys but missing key and outputs do not trigger",
+        ),
+        pytest.param(
+            {"key": "simcore/services/frontend/file-picker"},


if I get that correctly now this function will be called from comp_tasks_listening_task and also from PATCH /projects right?
is there any test for the case of computational services, and/or other dynamic services? or is this purely and only for file-picker? in which case please change the name of your functions to reflect that (if not this would be confusing).

Yes, this affects comp_tasks_listening_task and PATCH /projects (aka, when the fronted saves the project)

The only affects services are frontend services. And also, for now only the file-picker is involved. I've renamed the functions to highlight this.

…inks

pcrespov

If it is urgent, i might let it go through but I am concerned about the circular dependency though. Will recheck again later and try to help with a solution.

services/web/server/tests/unit/with_dbs/01/test_comp_tasks_listening_task.py

services/web/server/tests/unit/with_dbs/02/test_project_utils.py

pcrespov · 2022-05-23T09:02:36Z

services/web/server/src/simcore_service_webserver/projects/projects_nodes_utils.py

+    # this function is called from:
+    # - `projects/projects_db.py`
+    # - `computation_comp_tasks_listening_task.py` (was originally here)
+    from . import projects_api


This is really not good. We should have a look at it together. The more we delay solving this the worse it becomes
Please add an issue to follow up on this and make sure there is a FIXME here

Added FIXME and created an issue
#3069

I checked and reduced a bit the dependency but there is still a loop that IMO reveals a design problem in the changes of your PR

The problem i see is that you are including plugin-level API (i.e. projects_api) in the implementation of db-API repository (i.e. projects_db) while it should be the other way around i.e. the plugin-level API uses the db-API.

More details on the code follow ...

Thanks for this. How did you mange to find the loop, just by visually checking?

RTFM? :-D ... jokes aside, I did it both visually and following the code.

But there is also the option to find them automatically --show-cycles

NOTE: Perhaps add an example of --show-cycles in the doc of the script?

services/web/server/src/simcore_service_webserver/projects/projects_utils.py

pcrespov · 2022-05-23T09:32:49Z

services/web/server/src/simcore_service_webserver/projects/projects_utils.py

+            if "store" in data:
+                data["store"] = int(data["store"])
+
+    if current_dict.get("key") == "simcore/services/frontend/file-picker":


sorry, this implementation feel very hacky provided the generic name (find_changed_dict_keys) you gave to the function! Make sure it is only used in your specific case.

MINOR please consider comment above

So I think the comment in the function is explaining what the issue is. I'd guess that we need a better name for the function since it was moved from its original module. Changing to find_changed_node_keys

…inks

pcrespov

Please check the feedback i provided you together with the PR and consider re-designing your changes in projects/projects_db.py. Try re-designing this logic on top (i.e. using) of the project's db-API.
The circular import is a warning that we misusing the hierarchies of our design.

pcrespov · 2022-05-24T11:53:34Z

services/web/server/src/simcore_service_webserver/projects/projects_db.py

@@ -724,6 +739,10 @@ def _update_workbench(
                    .returning(literal_column("*"))
                )
                project: RowProxy = await result.fetchone()
+
+                for frontend_node_update_task in frontend_nodes_update_tasks:


This is the main cause of the circular import discussed in https://github.com/ITISFoundation/osparc-simcore/pull/3058/files#r880401810

IMO the db-API layer for projects should know nothing about front-end nodes etc . The idea here is create a repository pattern (SEE e.g. https://www.cosmicpython.com/book/chapter_02_repository.html) whose responsibility is accessing the database and hiding all sql-alchemy specialized logic.

The operations you are doing here IMO belong outside projects_db, where the concept of e.g. front-end-node is well defined but not here, where the only things that are defined are the columns of the table etc

I managed to move it outside in the end. You are correct, those functions should have not been inside the projects_db since it has a different objective.

pcrespov

👍

pcrespov · 2022-05-24T15:31:02Z

services/web/server/src/simcore_service_webserver/projects/projects_utils.py

+            if "store" in data:
+                data["store"] = int(data["store"])
+
+    if current_dict.get("key") == "simcore/services/frontend/file-picker":


MINOR please consider comment above

services/web/server/tests/unit/with_dbs/02/test_project_utils.py

sanderegg

Very neat, this will close some gaps. Thanks a lot for doing it!

services/web/server/src/simcore_service_webserver/projects/projects_db.py

services/web/server/src/simcore_service_webserver/projects/projects_nodes_utils.py

sanderegg · 2022-05-24T19:22:58Z

services/web/server/src/simcore_service_webserver/projects/projects_nodes_utils.py

+from models_library.projects import ProjectID
+from .projects_utils import get_frontend_node_outputs_changes
+from servicelib.utils import fire_and_forget_task
+from . import projects_api


actually a little bit weird that a utils calls the project_api... maybe that is one part of your issue with circular dependencies

No, I've removed the circular dependency, this ok now. This is no longer called from the projects_db but from the projects_handlers_crud.py. PC actually pointed out the issue.

…inks

sonarqubecloud · 2022-05-25T05:59:57Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
0.4% Duplication

fix example

8f005dc

GitHK self-assigned this May 20, 2022

GitHK added a:webserver issue related to the webserver service a:dynamic-sidecar dynamic-sidecar service changelog:🐛bugfix labels May 20, 2022

GitHK added this to the Croissant milestone May 20, 2022

Andrei Neagu added 6 commits May 20, 2022 09:10

dynamic-sidecar now purges files when downloadin incming data and fix…

4c307fa

…ed issue with parallel inputs downloads

fixed side effect due to multiple type of storage supporte

d6145bc

Merge remote-tracking branch 'upstream/master' into fix-file-picker-l…

8d629ad

…inks

file-picker events are now sent when files change

925b868

Merge remote-tracking branch 'upstream/master' into fix-file-picker-l…

4d69aca

…inks

codestyle

e91cd8c

GitHK changed the title ~~🐛 Fix file-picker issues~~ 🐛 Fix file-picker downstream service notification issues May 20, 2022

Andrei Neagu added 2 commits May 20, 2022 10:41

reduce deep nesting

ce92d38

refactor reducing complexity

2199ffa

GitHK marked this pull request as ready for review May 20, 2022 09:09

GitHK requested review from pcrespov and sanderegg as code owners May 20, 2022 09:09

Andrei Neagu added 3 commits May 20, 2022 11:34

fixed broken test

7452360

fixed typing

409498f

Merge remote-tracking branch 'upstream/master' into fix-file-picker-l…

fe4a47e

…inks

sanderegg reviewed May 22, 2022

View reviewed changes

Andrei Neagu added 7 commits May 23, 2022 07:46

Merge remote-tracking branch 'upstream/master' into fix-file-picker-l…

6489e74

…inks

refactor signatures

b233621

fixed typing

89bb6b2

remove old typing style

58b00c7

refactor typing and naming

1d43f53

moved get_node_outputs_changes and typing changes

6a72749

refactor moved find_changed_dict_keys

a257419

Andrei Neagu added 3 commits May 23, 2022 08:56

moved tests

b9c86db

renaming fucntions to make it more obvious

5e9c03d

replacing old style typing

ee30e3d

GitHK requested a review from sanderegg May 23, 2022 07:28

pcrespov reviewed May 23, 2022

View reviewed changes

Andrei Neagu added 5 commits May 23, 2022 15:28

fixed broken

7e35f35

Merge remote-tracking branch 'upstream/master' into fix-file-picker-l…

8fd5553

…inks

using simpler patching strategy

f7f88e9

repalced base types

fbeded1

fixed test styling

10ca0e7

GitHK mentioned this pull request May 24, 2022

Remove cyclic dependency #3069

Closed

Andrei Neagu added 3 commits May 24, 2022 09:28

adds fixme and issue notes

d05fe3c

fixed function, added more test cases

f4fd642

removed frotnend keys argument

e73c572

pcrespov requested changes May 24, 2022

View reviewed changes

pcrespov and others added 2 commits May 24, 2022 14:40

reduces dependency with projects_utils (#32)

8668697

refactor removing cyclic dependency

1b7721a

GitHK requested a review from pcrespov May 24, 2022 13:49

pcrespov approved these changes May 24, 2022

View reviewed changes

sanderegg approved these changes May 24, 2022

View reviewed changes

Andrei Neagu added 4 commits May 25, 2022 07:36

Merge remote-tracking branch 'upstream/master' into fix-file-picker-l…

899d3f0

…inks

moved test module

72ac601

refactor typing

b85becb

refactor function name

a0e1311

GitHK merged commit 46c78d3 into ITISFoundation:master May 25, 2022

GitHK deleted the fix-file-picker-links branch May 25, 2022 06:40

sanderegg mentioned this pull request Jun 2, 2022

maintenance/scaling of the platform ITISFoundation/osparc-issues#428

Closed

2 tasks

sanderegg mentioned this pull request Jun 24, 2022

computational services are always restarting #3137

Closed

sanderegg mentioned this pull request Jul 8, 2022

🐛 Regression fix: computational services always restarting #3176

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Fix file-picker downstream service notification issues #3058

🐛 Fix file-picker downstream service notification issues #3058

GitHK commented May 20, 2022 •

edited

Loading

codecov bot commented May 20, 2022 •

edited

Loading

sanderegg left a comment

sanderegg May 22, 2022 •

edited by pcrespov

Loading

GitHK May 23, 2022

GitHK May 23, 2022

sanderegg May 22, 2022

GitHK May 23, 2022

sanderegg May 22, 2022

GitHK May 23, 2022

sanderegg May 22, 2022

GitHK May 23, 2022 •

edited

Loading

sanderegg May 22, 2022

GitHK May 23, 2022

pcrespov left a comment

pcrespov May 23, 2022

GitHK May 24, 2022

pcrespov May 24, 2022

GitHK May 24, 2022

pcrespov May 24, 2022

pcrespov May 24, 2022

pcrespov May 23, 2022

pcrespov May 24, 2022

GitHK May 25, 2022

pcrespov left a comment

pcrespov May 24, 2022

GitHK May 24, 2022

pcrespov left a comment

pcrespov May 24, 2022

sanderegg left a comment

sanderegg May 24, 2022

GitHK May 25, 2022

sonarqubecloud bot commented May 25, 2022

🐛 Fix file-picker downstream service notification issues #3058

🐛 Fix file-picker downstream service notification issues #3058

Conversation

GitHK commented May 20, 2022 • edited Loading

What do these changes do?

Related issue/s

How to test

Checklist

codecov bot commented May 20, 2022 • edited Loading

Codecov Report

sanderegg left a comment

Choose a reason for hiding this comment

sanderegg May 22, 2022 • edited by pcrespov 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

GitHK May 23, 2022 • edited Loading

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

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

pcrespov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanderegg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented May 25, 2022

GitHK commented May 20, 2022 •

edited

Loading

codecov bot commented May 20, 2022 •

edited

Loading

sanderegg May 22, 2022 •

edited by pcrespov

Loading

GitHK May 23, 2022 •

edited

Loading