🎨 publish port events to frontend #6396

GitHK · 2024-09-19T09:17:49Z

What do these changes do?

To provide better feedback for users in the UI we are adding notifications for ports during the runtime of the platform. Notifications are send from the backend for each individual port. Even when all ports are required to download/upload notifications are sent one port at a time.

The only notifications that make sense are:

input port when it downloads data (we never upload data form an input port)
output port when it uploads data (outputs are downloaded and they have a progress bar when the service starts)

Input ports

Notification message for each input port download:
42["stateInputPorts",{"project_id":"b4a03e00-7691-11ef-a0a5-0242ac14001a","node_id":"81e4a4d9-e8f8-4816-ad22-58ce586f46e5","port_key":"input_20","status":"DOWNLOAD_FINISHED_SUCCESSFULLY"}].

It can have one of the following status states:

DOWNLOAD_STARTED
DOWNLOAD_WAS_ABORTED
DOWNLOAD_FINISHED_SUCCESSFULLY
DOWNLOAD_FINISHED_WITH_ERRROR

Output ports

Notification message for each output port upload:
42["stateOutputPorts",{"project_id":"b4a03e00-7691-11ef-a0a5-0242ac14001a","node_id":"81e4a4d9-e8f8-4816-ad22-58ce586f46e5","port_key":"output_2","status":"UPLOAD_STARTED"}].

It can have one of the following status states:

UPLOAD_STARTED
UPLOAD_WAS_ABORTED
UPLOAD_FINISHED_SUCCESSFULLY
UPLOAD_FINISHED_WITH_ERRROR

Related issue/s

How to test

Dev-ops checklist

No ENV changes or I properly updated ENV (read the instruction)

codecov · 2024-09-19T09:27:40Z

Codecov Report

Attention: Patch coverage is 76.31579% with 36 lines in your changes missing coverage. Please review.

Project coverage is 88.8%. Comparing base (cafbf96) to head (9f2854f).
Report is 562 commits behind head on master.

Files with missing lines	Patch %	Lines
...mcore_service_dynamic_sidecar/modules/nodeports.py	21.6%	29 Missing ⚠️
...-sdk/src/simcore_sdk/node_ports_v2/nodeports_v2.py	58.8%	6 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##           master   #6396      +/-   ##
=========================================
+ Coverage    84.5%   88.8%    +4.2%     
=========================================
  Files          10    1227    +1217     
  Lines         214   54047   +53833     
  Branches       25     867     +842     
=========================================
+ Hits          181   48024   +47843     
- Misses         23    5891    +5868     
- Partials       10     132     +122

Flag	Coverage Δ
integrationtests	`64.7% <58.7%> (?)`
unittests	`86.5% <76.3%> (+1.9%)`	⬆️

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

Files with missing lines	Coverage Δ
...odels_library/api_schemas_dynamic_sidecar/ports.py	`100.0% <100.0%> (ø)`
...ls_library/api_schemas_dynamic_sidecar/socketio.py	`100.0% <100.0%> (ø)`
...namic_sidecar/api/containers_long_running_tasks.py	`100.0% <ø> (ø)`
...imcore_service_dynamic_sidecar/core/application.py	`98.2% <100.0%> (ø)`
...vice_dynamic_sidecar/modules/long_running_tasks.py	`94.4% <100.0%> (ø)`
..._dynamic_sidecar/modules/notifications/__init__.py	`100.0% <100.0%> (ø)`
...ecar/modules/notifications/_notifications_ports.py	`100.0% <100.0%> (ø)`
...les/notifications/_notifications_system_monitor.py	`100.0% <100.0%> (ø)`
...dynamic_sidecar/modules/notifications/_notifier.py	`100.0% <100.0%> (ø)`
...ce_dynamic_sidecar/modules/notifications/_setup.py	`100.0% <100.0%> (ø)`
... and 6 more

... and 1198 files with indirect coverage changes

…ange-notifications

pcrespov

thx, left some suggests to consider

packages/models-library/src/models_library/api_schemas_dynamic_sidecar/ports.py

pcrespov · 2024-09-20T12:04:02Z

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

+                await outputs_callbacks.aborted(port_key)
+                raise
+            except Exception:
+                await outputs_callbacks.finished_with_error(port_key)


Q: So if there is an **unexpected ** error, you want to notify simply that it failed? Since this reaches the user , I would use the technique of logging the error on the server side and sending an OEC to the front-end.

wait, you are reraising it ... so perhaps report it when you catch it?
BTW why do not you need to reraise?

pcrespov · 2024-09-20T12:07:38Z

services/dynamic-sidecar/src/simcore_service_dynamic_sidecar/modules/notifications/__init__.py

THOUGHT: this approach is growing faster than we anticipated. I believe it is time we redirect everything via notifiers service as we discussed sometime ago #1828

sonarcloud · 2024-09-20T13:11:14Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

sanderegg

a few comments. Good luck with it ! see you later!

sanderegg · 2024-09-20T15:33:16Z

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

@@ -138,6 +139,7 @@ async def mock_node_port_creator_cb(*args, **kwargs):
                + list(original_outputs.values())
            },
            progress_bar=progress_bar,
+            outputs_callbacks=AsyncMock(),


not sure I understand here. you do not test anything of what you added right?

sanderegg · 2024-09-20T17:11:38Z

packages/models-library/src/models_library/api_schemas_dynamic_sidecar/ports.py

+from pydantic import BaseModel
+
+
+class OutputStatus(StrAutoEnum):


I am a bit concerned that we have many of these states and status all over the place... duplication is evil, and makes everything more complex. be careful please.

sanderegg · 2024-09-20T17:13:45Z

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

@@ -27,6 +29,20 @@
 log = logging.getLogger(__name__)


+class OutputsCallbacks(ABC):


Here I have a few comments:

this OutputsCallbacks, and actually set_multiple are only used by the dynamic sidecar, why are they coming with this >complex stuff inside the node ports package? Would it not make sense that you build on top instead of creating these weird constructs?

The tests you added do not check anything regarding how this callbacks work,

Sorry, but this looks like C++ man... ;)

sanderegg · 2024-09-20T17:14:57Z

services/dynamic-sidecar/src/simcore_service_dynamic_sidecar/core/application.py

@@ -172,6 +173,7 @@ def create_app():
    setup_rabbitmq(app)
    setup_background_log_fetcher(app)
    setup_resource_tracking(app)
+    setup_notifications(app)


is this not already a copy paste of something else? I have the feeling this notifications thing comes up several time in the repo

sanderegg · 2024-09-20T17:16:08Z

...ic-sidecar/src/simcore_service_dynamic_sidecar/modules/notifications/_notifications_ports.py

+    async def _send_output_port_status(
+        self, port_key: ServicePortKey, status: OutputStatus
+    ) -> None:
+        notifier: Notifier = Notifier.get_from_app_state(self.app)


thought: Would this not be more generic if it would not depend on the app? you would need to pass the notifier.

sanderegg · 2024-09-20T17:18:06Z

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

@@ -19,6 +19,9 @@
 from servicelib.progress_bar import ProgressBarData
 from servicelib.utils import logged_gather
 from simcore_sdk.node_data import data_manager
+from simcore_service_dynamic_sidecar.modules.notifications._notifications_ports import (


relative imports

sanderegg · 2024-09-20T17:18:53Z

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

@@ -70,13 +72,28 @@ def _get_size_of_value(value: tuple[ItemConcreteValue | None, SetKWargs | None])
 )


+class OutputCallbacksWrapper(OutputsCallbacks):


as I said... this thing should live entirely in the dynamic sidecar. nobody else needs it.

sanderegg · 2024-09-20T17:19:05Z

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

        port_value
        for port_value in (await PORTS.outputs).values()
        if (not port_keys) or (port_value.key in port_keys)
    ]

+    await logged_gather(


limited_gather?

sanderegg · 2024-09-20T17:20:57Z

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

+            # NOTE: if one archiving task fails/cancelled all the ports are affected
+            # setting all other ports as finished with error/cancelled
+            try:
+                await logged_gather(*archiving_tasks)


please use limited_gather.
Also there is an asyncGenerator available.

Why not create a specific function that does all this stuff instead of that weird thing? you know that if an exception is raised in one of them coroutine it will not stop the others? Please double check these things...

sanderegg · 2024-09-20T17:21:54Z

services/dynamic-sidecar/tests/unit/test_modules_notifier.py

+        [RabbitSettings], _AsyncGeneratorContextManager[AsyncServer]
+    ],
+) -> AsyncIterable[AsyncServer]:
+    # Same configuration as simcore_service_webserver/socketio/server.py


So in this file you basically test that notifier thing. Please move it out of the node ports. this is indirect testing.
If we do not need that thing anywhere else (which is the case) there is no need to have it in the node ports.

extracting notifications as seprate module

220b7b6

GitHK self-assigned this Sep 19, 2024

GitHK added this to the MartinKippenberger milestone Sep 19, 2024

Andrei Neagu added 12 commits September 19, 2024 11:28

refactor internals

b419e64

Merge remote-tracking branch 'upstream/master' into pr-osparc-port-ch…

71678d6

…ange-notifications

rename module

d1c838e

extend notifications interface

fc88f8d

added notifications tests

6bd6d15

refactor to work as expected

249dda7

added missing project_id to notification

0c2153c

Merge remote-tracking branch 'upstream/master' into pr-osparc-port-ch…

4e22b75

…ange-notifications

adding missing port notifier to constructor

4ed8e2a

refactor broken tests

d9950fa

Merge remote-tracking branch 'upstream/master' into pr-osparc-port-ch…

796e1b5

…ange-notifications

fixed tests

9973d68

GitHK marked this pull request as ready for review September 20, 2024 07:34

GitHK requested review from sanderegg, pcrespov and matusdrobuliak66 as code owners September 20, 2024 07:34

GitHK requested a review from giancarloromeo September 20, 2024 07:34

pcrespov approved these changes Sep 20, 2024

View reviewed changes

using str auto enum

9f2854f

sanderegg reviewed Sep 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🎨 publish port events to frontend #6396

🎨 publish port events to frontend #6396

GitHK commented Sep 19, 2024 •

edited

Loading

codecov bot commented Sep 19, 2024 •

edited

Loading

pcrespov left a comment

pcrespov Sep 20, 2024

pcrespov Sep 20, 2024

sonarcloud bot commented Sep 20, 2024

sanderegg left a comment

sanderegg Sep 20, 2024

sanderegg Sep 20, 2024

sanderegg Sep 20, 2024

sanderegg Sep 20, 2024

sanderegg Sep 20, 2024

sanderegg Sep 20, 2024

sanderegg Sep 20, 2024

sanderegg Sep 20, 2024

sanderegg Sep 20, 2024

sanderegg Sep 20, 2024

		from pydantic import BaseModel


		class OutputStatus(StrAutoEnum):

		@@ -27,6 +29,20 @@
		log = logging.getLogger(__name__)


		class OutputsCallbacks(ABC):

		@@ -70,13 +72,28 @@ def _get_size_of_value(value: tuple[ItemConcreteValue \| None, SetKWargs \| None])
		)


		class OutputCallbacksWrapper(OutputsCallbacks):

🎨 publish port events to frontend #6396

Are you sure you want to change the base?

🎨 publish port events to frontend #6396

Conversation

GitHK commented Sep 19, 2024 • edited Loading

What do these changes do?

Input ports

Output ports

Related issue/s

How to test

Dev-ops checklist

codecov bot commented Sep 19, 2024 • 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

sonarcloud bot commented Sep 20, 2024

Quality Gate passed

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

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 commented Sep 19, 2024 •

edited

Loading

codecov bot commented Sep 19, 2024 •

edited

Loading