Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ Enhance compatibility-check for catalog service's ports including units #2913

Merged

Conversation

pcrespov
Copy link
Member

@pcrespov pcrespov commented Mar 22, 2022

What do these changes do?

This PR enhances port's compatibility-check logic between catalog services which is used by the front-end to determine how "two services can connect" (see catalog_utils.py and screenshot below).

Port units are now parsed, validated and formatted using pint. Notice that this PR only adds this library on the catalog plugin.

Highlights

  • Some new can_connect rules :
    • unitless can connect unit (and the contrary)
    • wider units compatibility (e.g. miles can connect with mm). Notice that in later PR we will introduce the conversion of these units when the data flows from one service to the next.
  • service input/outputs models returned by API contains html-formated units in the fields unitLong and unitShort

Extras

  • 🐛 gunicorn log uses same log level as app

Related issue/s

How to test

$ cd services/web/server
$ make install-dev
$ pytest --pdb -vv tests/**/test_catalog*.py
  • test auto-connect using demo units function service
    chrome-capture

Checklist

  • Adds units plugin? since will be used in catalog and projects?
    • creates instance in app
  • Openapi changes? make openapi-specs, git commit ... and then make version-*)
  • Unit tests for the changes exist
  • Runs in the swarm
  • Documentation reflects the changes

@pcrespov pcrespov self-assigned this Mar 22, 2022
@pcrespov pcrespov added the a:webserver issue related to the webserver service label Mar 22, 2022
@pcrespov pcrespov added this to the E.Shackleton milestone Mar 22, 2022
@codecov
Copy link

codecov bot commented Mar 22, 2022

Codecov Report

Merging #2913 (4f279c3) into master (a920852) will increase coverage by 1.3%.
The diff coverage is 78.6%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #2913     +/-   ##
========================================
+ Coverage    78.2%   79.6%   +1.3%     
========================================
  Files         671     671             
  Lines       27787   27828     +41     
  Branches     3224    3229      +5     
========================================
+ Hits        21743   22162    +419     
+ Misses       5259    4914    -345     
+ Partials      785     752     -33     
Flag Coverage Δ
integrationtests 65.7% <34.5%> (+<0.1%) ⬆️
unittests 75.3% <78.6%> (+0.5%) ⬆️

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

Impacted Files Coverage Δ
.../src/models_library/function_service/demo_units.py 100.0% <ø> (ø)
.../src/simcore_service_webserver/catalog_handlers.py 37.7% <25.0%> (-0.9%) ⬇️
...er/src/simcore_service_webserver/catalog_models.py 88.6% <73.6%> (-0.3%) ⬇️
...ver/src/simcore_service_webserver/catalog_utils.py 86.0% <86.0%> (ø)
...ls-library/src/models_library/projects_nodes_io.py 96.3% <100.0%> (+0.2%) ⬆️
...eb/server/src/simcore_service_webserver/catalog.py 100.0% <100.0%> (ø)
...rvice_datcore_adapter/utils/requests_decorators.py 71.0% <0.0%> (-7.9%) ⬇️
.../simcore_service_catalog/db/repositories/groups.py 72.9% <0.0%> (-5.5%) ⬇️
.../director/src/simcore_service_director/producer.py 62.7% <0.0%> (+1.0%) ⬆️
...tor_v2/modules/dynamic_sidecar/scheduler/events.py 93.7% <0.0%> (+1.1%) ⬆️
... and 41 more

@pcrespov pcrespov marked this pull request as ready for review March 23, 2022 23:45
@pcrespov pcrespov changed the title WIP: ✨ Is515/port units n constraints ✨ Enhance compatibility-check for catalog service's ports including units Mar 23, 2022
@pcrespov pcrespov requested review from odeimaiz and mguidon March 23, 2022 23:46
Copy link
Contributor

@GitHK GitHK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Works for me, please check some questions and especially the yarl pinning. Not sure why we would like to do that.

Comment on lines +26 to +27
# SEE services/web/server/tests/unit/isolated/test_utils.py::test_yarl_url_compose_changed_with_latest_release
yarl<1.6
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add when this could be unpinned? I cannot find the reference from the comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you mean? I think the line 26 is pretty clear, ritgh? This is the test

def test_yarl_url_compose_changed_with_latest_release():

Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool, but please not ureg...

@KZzizzle
Copy link
Contributor

very cool - excited to see this in action!

Copy link
Member

@odeimaiz odeimaiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌

@pcrespov pcrespov force-pushed the is515/port-units-n-constraints branch from 6e3920c to 61e0102 Compare March 24, 2022 12:34
@pcrespov pcrespov force-pushed the is515/port-units-n-constraints branch from 61e0102 to 4f279c3 Compare March 24, 2022 12:36
@pcrespov pcrespov merged commit f401ad2 into ITISFoundation:master Mar 24, 2022
@pcrespov pcrespov deleted the is515/port-units-n-constraints branch March 24, 2022 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:webserver issue related to the webserver service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants