♻️ refactor dv-2 dy-sidecar's API client

[GitHK]

What do these changes do?

Brings a "drop-in" replacement of the dynamic-sidecar client_api module inside director-v2.
This new module adds:

• an easily extensible central error handling system.
• retry on fail when trying to initiate an API call

The new module api_clientis composed of 3 layers:

• _base.py contains all the low level error handling and utils to create an interface for an API. The ideas is that this module can be moved elsewhere and shared with other clients. Functionality is exposed via decorators, like: expect_status and retry_on_errors
• _thin.py contains a thin client wrapping around the HTTP interface of the dynamic-sidecar. In the future this could be autogenerated from the OAS file of a service.
• _public.py is the port of the previous client_api based on the thin_ client.

Related issue/s

• improving dynamic-sidecar design osparc-issues#638

How to test

Checklist

• Unit tests for the changes exist

[codecov]

Codecov Report

Merging #3121 (6ad0191) into master (8e1c4f2) will increase coverage by 1.3%.
The diff coverage is 94.7%.

@@           Coverage Diff            @@
##           master   #3121     +/-   ##
========================================
+ Coverage    79.7%   81.1%   +1.3%     
========================================
  Files         706     710      +4     
  Lines       30496   30615    +119     
  Branches     3950    3948      -2     
========================================
+ Hits        24321   24834    +513     
+ Misses       5352    4941    -411     
- Partials      823     840     +17     

Flag	Coverage Δ
integrationtests	66.2% <89.2%> (+16.1%)	⬆️
unittests	77.5% <90.4%> (+0.3%)	⬆️

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

Impacted Files	Coverage Δ
...vice_director_v2/modules/dynamic_sidecar/errors.py	100.0% <ø> (ø)
...tor_v2/modules/dynamic_sidecar/scheduler/events.py	92.3% <80.0%> (+1.0%)	⬆️
...r_v2/modules/dynamic_sidecar/api_client/_public.py	90.9% <90.9%> (ø)
...tor_v2/modules/dynamic_sidecar/api_client/_base.py	93.2% <93.2%> (ø)
...2/src/simcore_service_director_v2/core/settings.py	96.2% <100.0%> (+<0.1%)	⬆️
...or_v2/models/schemas/dynamic_services/scheduler.py	98.4% <100.0%> (ø)
..._v2/modules/dynamic_sidecar/api_client/__init__.py	100.0% <100.0%> (ø)
...r_v2/modules/dynamic_sidecar/api_client/_errors.py	100.0% <100.0%> (ø)
...tor_v2/modules/dynamic_sidecar/api_client/_thin.py	100.0% <100.0%> (ø)
...irector_v2/modules/dynamic_sidecar/module_setup.py	100.0% <100.0%> (ø)
... and 36 more

[sanderegg]

it's a nice addition. I would like to see a test where you catch the httpx error. I do not get why this one still goes through when all the others are encapsulated.

Also if you could maybe asses the fact that the dynamic sidecar already retries every 5 seconds, what is the point of bringing 4 more retries in every request call?

[sanderegg]

maybe simplify the name of the functions, we know they all are linked to the api_client.. suggestion:
setup, close, get

And what means: update_dynamic_sidecar_health? what is it supposed to do? It sounds a bit strange that as a client of the dynamic sidecar I would update its health.

[GitHK]

I agree for setup and close, not with the get_api_client.

[GitHK]

I went with a rename from update_dynamic_sidecar_health to get_dynamic_sidecar_service_health which should be more precise. Only issue is that it does not return anything but it stores it memory. That's why it was called update previously.

[pcrespov]

This check is in a constructor or a class, I do not think this check will run when imported.

What I mean is that a tool like mypy will detect that in your decorators (e.g. expected_status) you annotated that it wraps Callable[..., Awaitable[Response]]. Therefore if it does not return a Reponse it fails the static check!

[pcrespov]

Please review my latest comments

[sanderegg]

Very nice. Please check my 2-3 comments.

[sanderegg]

I do find a bit weird that most of the calls actually raise further but this one not. I am not sure that is consistent. maybe consider raising? or is there a rationale for not doing so?

[GitHK]

Tis one can still raise BaseClientHTTPError. Which is actually trapped and handled at the point where it is used.
This happens in the two below calls you singled and in task.py

[sanderegg]

Suggested change

	def env_mocks(monkeypatch: MonkeyPatch, mock_env: None) -> None:
	def mock_end(monkeypatch: MonkeyPatch, mock_env: None) -> None:

Let's define this as good practice now. overwrite it, let's not have thousand different names

[GitHK]

OK, not bad, I like this one!

[sanderegg]

actually mock_env not mock_end...

[GitHK]

So I can overwrite the same exiting mock with one of the same name even when inheriting from it!

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
0.4% Duplication