-
Notifications
You must be signed in to change notification settings - Fork 32
Functions api β¨ ποΈ #7539
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7539 +/- ##
==========================================
- Coverage 87.57% 86.03% -1.54%
==========================================
Files 1794 1731 -63
Lines 69368 68182 -1186
Branches 1136 1037 -99
==========================================
- Hits 60747 58661 -2086
- Misses 8311 9247 +936
+ Partials 310 274 -36
*This pull request uses carry forward flags. Click here to find out more.
Continue to review full report in Codecov by Sentry.
π New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the great effort Werner! It looks very cool.
I suggest several minor changes, but I have one big request and that concerns running the function. I think it is crucial that that happens asyncronously and in one of our dedicated celery workers. This call (in the case of studies) was exactly what was blocking us last time we did the metamodeling. We are in a better position now because we have a job scheduling framework for handling "internal long running tasks". We should use it! π
packages/postgres-database/src/simcore_postgres_database/models/functions_models_db.py
Outdated
Show resolved
Hide resolved
packages/postgres-database/src/simcore_postgres_database/models/functions_models_db.py
Outdated
Show resolved
Hide resolved
packages/postgres-database/src/simcore_postgres_database/models/functions_models_db.py
Outdated
Show resolved
Hide resolved
...ages/service-library/src/servicelib/rabbitmq/rpc_interfaces/webserver/functions/functions.py
Show resolved
Hide resolved
...ibrary/src/servicelib/rabbitmq/rpc_interfaces/webserver/functions/functions_rpc_interface.py
Outdated
Show resolved
Hide resolved
services/web/server/src/simcore_service_webserver/functions/_functions_controller_rpc.py
Outdated
Show resolved
Hide resolved
services/web/server/src/simcore_service_webserver/functions/_functions_controller_rpc.py
Outdated
Show resolved
Hide resolved
services/web/server/src/simcore_service_webserver/functions/_functions_controller_rpc.py
Outdated
Show resolved
Hide resolved
services/web/server/src/simcore_service_webserver/functions/_functions_controller_rpc.py
Outdated
Show resolved
Hide resolved
services/web/server/src/simcore_service_webserver/functions/_functions_controller_rpc.py
Outdated
Show resolved
Hide resolved
Yeah, sure makes sense. I'm basically calling the study_job api. It's probably best to implement the celery changes there? (it they're not there yet) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the changes. I had another look and provided some more feedback. I think it is quite critical that we sort out the run function so that creating it becomes a long running task in a celery worker somewhere.
packages/models-library/src/models_library/api_schemas_api_server/functions.py
Outdated
Show resolved
Hide resolved
packages/models-library/src/models_library/api_schemas_api_server/functions.py
Outdated
Show resolved
Hide resolved
packages/models-library/src/models_library/api_schemas_webserver/functions_wb_schema.py
Show resolved
Hide resolved
services/api-server/src/simcore_service_api_server/models/schemas/functions_api_schema.py
Show resolved
Hide resolved
services/web/server/src/simcore_service_webserver/functions/_functions_repository.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few more comments and suggestions.
Also a way to simplify the api-server's tests which will remove quite some code and make them more intuitive when reading.
packages/models-library/src/models_library/api_schemas_webserver/functions_wb_schema.py
Outdated
Show resolved
Hide resolved
...core_postgres_database/migration/versions/32a9ab5fa107_add_creation_modification_time_to_.py
Outdated
Show resolved
Hide resolved
...ibrary/src/servicelib/rabbitmq/rpc_interfaces/webserver/functions/functions_rpc_interface.py
Outdated
Show resolved
Hide resolved
...ibrary/src/servicelib/rabbitmq/rpc_interfaces/webserver/functions/functions_rpc_interface.py
Outdated
Show resolved
Hide resolved
...ibrary/src/servicelib/rabbitmq/rpc_interfaces/webserver/functions/functions_rpc_interface.py
Outdated
Show resolved
Hide resolved
services/web/server/tests/unit/with_dbs/04/functions_rpc/test_functions_controller_rpc.py
Outdated
Show resolved
Hide resolved
services/web/server/tests/unit/with_dbs/04/functions_rpc/test_functions_controller_rpc.py
Outdated
Show resolved
Hide resolved
services/web/server/tests/unit/with_dbs/04/functions_rpc/test_functions_controller_rpc.py
Outdated
Show resolved
Hide resolved
services/api-server/tests/unit/api_functions/test_api_routers_functions.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Enables the new Functions API end-to-end by removing legacy stubs, adding RPC client wrappers, Pydantic schemas, HTTP routes, database tables, and migrations.
- Removes old webserver RPC controller for functions.
- Adds RPC client methods in
WbApiRpcClientand Pydantic schemas for functions, jobs, and collections. - Registers new
/functions,/function_jobs, and/function_job_collectionsroutes and creates SQL tables with migrations.
Reviewed Changes
Copilot reviewed 28 out of 30 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| services/web/server/src/simcore_service_webserver/functions/_controller_rpc.py | Removed old RPC router and ping endpoint |
| services/api-server/tests/unit/api_functions/conftest.py | Added fixtures and dummy RPC client for functions API tests |
| services/api-server/src/simcore_service_api_server/services_rpc/wb_api_server.py | Added wrapper methods for functions RPC in WbApiRpcClient |
| services/api-server/src/simcore_service_api_server/models/schemas/functions_api_schema.py | New Pydantic schemas for functions, jobs, and collections |
| services/api-server/src/simcore_service_api_server/api/root.py | Registered new routers for functions, jobs, and collections |
| packages/service-library/src/servicelib/rabbitmq/rpc_interfaces/webserver/functions/ | RPC interface implementations for all functions endpoints |
| packages/postgres-database/src/simcore_postgres_database/models/funcapi_*.py | Added tables for functions, jobs, collections, and join |
| packages/postgres-database/src/simcore_postgres_database/migration/versions/*.py | Alembic scripts to create and evolve function API tables |
Files not reviewed (2)
- services/api-server/requirements/_test.in: Language not supported
- services/api-server/requirements/_test.txt: Language not supported
Comments suppressed due to low confidence (2)
services/api-server/src/simcore_service_api_server/models/schemas/functions_api_schema.py:74
- [nitpick] The
FunctionJobCollectionmodel usesidfor its primary key field while other models useuid; consider renamingidtouidfor consistency across your schemas.
class FunctionJobCollection(BaseModel):
services/api-server/src/simcore_service_api_server/services_rpc/wb_api_server.py:3
- The imports
dataclassandpartialare unused in this file; please remove them to clean up dependencies.
from dataclasses import dataclass
.../simcore_postgres_database/models/funcapi_function_job_collections_to_function_jobs_table.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry didn't have time to go over it, but I do not want to block you so approve, I will go over it as soon as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, I added a few comments.
For next time please create smaller PRs that are easier to review (you will also get faster reviews):
- 1 PR with database changes
- 1 PR with API changes,
- ...
Thank you and good luck!
packages/models-library/src/models_library/api_schemas_webserver/functions_wb_schema.py
Outdated
Show resolved
Hide resolved
packages/models-library/src/models_library/api_schemas_webserver/functions_wb_schema.py
Show resolved
Hide resolved
packages/models-library/src/models_library/api_schemas_webserver/functions_wb_schema.py
Outdated
Show resolved
Hide resolved
packages/postgres-database/src/simcore_postgres_database/models/funcapi_function_jobs_table.py
Outdated
Show resolved
Hide resolved
packages/postgres-database/src/simcore_postgres_database/models/funcapi_functions_table.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already approved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the changes!
I'm OK with it, just some minor things still remain.
...ibrary/src/servicelib/rabbitmq/rpc_interfaces/webserver/functions/functions_rpc_interface.py
Outdated
Show resolved
Hide resolved
services/api-server/src/simcore_service_api_server/models/schemas/functions_api_schema.py
Outdated
Show resolved
Hide resolved
services/web/server/tests/unit/with_dbs/04/functions_rpc/test_functions_controller_rpc.py
Outdated
Show resolved
Hide resolved
|
|
@mergify queue |
β The pull request has been merged automaticallyThe pull request has been merged automatically at 67c3d67 |



What do these changes do?
This introduces the Functions API.
Functions are objects that can be in the backend: projects, solvers, python code, etc ...
Users can create their own functions from the relevant objects.
They accept parameters and return results.
Each function run creates a function job. These function job can also act as a cache to prevent rerunning a function with the same parameters.
Related issue/s
#7506
How to test
There are tests in the api-server (Rest) and web/server (RPC) service directories.
A high-level functional test script is here:
https://github.com/wvangeit/osparc-functions-api-test/blob/master/test.py
Dev-ops checklist