Skip to content

🎨 👽️ Improve start job endpoint in webserver and improve error handling in api-server #5927

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

Merged

Conversation

bisgaard-itis
Copy link
Contributor

@bisgaard-itis bisgaard-itis commented Jun 7, 2024

What do these changes do?

  • Introduce better exception handling in the api-server. This allows for more control over how exceptions raised while communicating with the backend are handled.
  • When starting a computation which is already running the director-v2 and webserver now return 409 instead of 403. This is done because it seems like a more correct status code to return and because it is convenient to have a single return status which indicates this conflict so we can handle it separately in the api-server.
  • Convert the 409 to a 200 in the api-server. The reason for this is the following: The aim was originally to follow docker's api but a 304 must not contain a body and in this case it is useful to return the job status so that the user of the osparc python client doesn't notice that (s)he hit the start endpoint twice.
  • A return status 202 is now returned when succesfully starting a study/solver job via the api-server. This could alternatively have been a 201 but I decided for a 202 because no resource is really created in this operation.

Related issue/s

How to test

Dev-ops checklist

Copy link

codecov bot commented Jun 7, 2024

Codecov Report

Attention: Patch coverage is 96.69421% with 4 lines in your changes missing coverage. Please review.

Project coverage is 85.5%. Comparing base (cafbf96) to head (28c0048).
Report is 288 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #5927      +/-   ##
=========================================
+ Coverage    84.5%   85.5%    +0.9%     
=========================================
  Files          10     639     +629     
  Lines         214   31161   +30947     
  Branches       25     205     +180     
=========================================
+ Hits          181   26666   +26485     
- Misses         23    4444    +4421     
- Partials       10      51      +41     
Flag Coverage Δ
integrationtests 63.5% <0.0%> (?)
unittests 86.7% <96.6%> (+2.2%) ⬆️

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

Files Coverage Δ
...core_service_api_server/api/routes/solvers_jobs.py 100.0% <100.0%> (ø)
...re_service_api_server/exceptions/backend_errors.py 100.0% <100.0%> (ø)
...service_api_server/exceptions/handlers/__init__.py 100.0% <100.0%> (ø)
...er/exceptions/handlers/_handlers_backend_errors.py 100.0% <100.0%> (ø)
...vice_api_server/exceptions/service_errors_utils.py 87.2% <100.0%> (ø)
...src/simcore_service_api_server/services/catalog.py 67.4% <100.0%> (ø)
...simcore_service_api_server/services/director_v2.py 77.2% <100.0%> (ø)
...c/simcore_service_api_server/services/webserver.py 99.6% <100.0%> (ø)
...ore_service_director_v2/api/routes/computations.py 83.7% <100.0%> (ø)
..._service_api_server/services/solver_job_outputs.py 54.1% <75.0%> (ø)
... and 1 more

... and 638 files with indirect coverage changes

@bisgaard-itis bisgaard-itis self-assigned this Jun 10, 2024
@bisgaard-itis bisgaard-itis added the a:apiserver api-server service label Jun 10, 2024
@bisgaard-itis bisgaard-itis added this to the South Island Iced Tea milestone Jun 10, 2024
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@sanderegg
Copy link
Member

@mguidon you might have to modify your c++ client according to these changes. Once thee changes go in the api-server will return a 202 when you succesfully start a job. It used to be a 200.

Thanks for letting me know. I just double checked. I usually check if the http response is in [200, 300). However, I explicitly check for a 201 if I create the job and I have a special treatment for 402 (no credits left).

So for this round I am safe. However, if you change the 201 or the 402 sim4life will not work anymore. That is bad.

@pcrespov @sanderegg what is a good best practice here?

@mguidon well I guess we are lucky on that one.
Now I guess in case we make an incompatible change, then we would have to make sure that your "old" clients cannot access that API? that would probably mean versioning the API up or something like that, or using headers.

@bisgaard-itis
Copy link
Contributor Author

@mguidon you might have to modify your c++ client according to these changes. Once thee changes go in the api-server will return a 202 when you succesfully start a job. It used to be a 200.

Thanks for letting me know. I just double checked. I usually check if the http response is in [200, 300). However, I explicitly check for a 201 if I create the job and I have a special treatment for 402 (no credits left).

So for this round I am safe. However, if you change the 201 or the 402 sim4life will not work anymore. That is bad.

@pcrespov @sanderegg what is a good best practice here?

Actually I change the 201 to a 202 in this PR. So this would probably break sim4life as I understand it. But maybe I should stick with a 201 in case a job is successfully started. I am sure @pcrespov has an opinion about that (?😉)

@mguidon
Copy link
Member

mguidon commented Jun 19, 2024

@mguidon you might have to modify your c++ client according to these changes. Once thee changes go in the api-server will return a 202 when you succesfully start a job. It used to be a 200.

Thanks for letting me know. I just double checked. I usually check if the http response is in [200, 300). However, I explicitly check for a 201 if I create the job and I have a special treatment for 402 (no credits left).
So for this round I am safe. However, if you change the 201 or the 402 sim4life will not work anymore. That is bad.
@pcrespov @sanderegg what is a good best practice here?

Actually I change the 201 to a 202 in this PR. So this would probably break sim4life as I understand it. But maybe I should stick with a 201 in case a job is successfully started. I am sure @pcrespov has an opinion about that (?😉)

You mention above that you have a 202 if the jobs starts. Thats fine. I just check the 201 when I create the job.

@bisgaard-itis
Copy link
Contributor Author

@mguidon you might have to modify your c++ client according to these changes. Once thee changes go in the api-server will return a 202 when you succesfully start a job. It used to be a 200.

Thanks for letting me know. I just double checked. I usually check if the http response is in [200, 300). However, I explicitly check for a 201 if I create the job and I have a special treatment for 402 (no credits left).
So for this round I am safe. However, if you change the 201 or the 402 sim4life will not work anymore. That is bad.
@pcrespov @sanderegg what is a good best practice here?

Actually I change the 201 to a 202 in this PR. So this would probably break sim4life as I understand it. But maybe I should stick with a 201 in case a job is successfully started. I am sure @pcrespov has an opinion about that (?😉)

You mention above that you have a 202 if the jobs starts. Thats fine. I just check the 201 when I create the job.

Ah ok, perfect! Then I misunderstood. Very good. So then this should be compatible with your client 🤞🏻

Copy link
Member

@pcrespov pcrespov left a comment

Choose a reason for hiding this comment

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

thx. I left some suggestsion and thoughts :-)

@pcrespov
Copy link
Member

@mguidon you might have to modify your c++ client according to these changes. Once thee changes go in the api-server will return a 202 when you succesfully start a job. It used to be a 200.

Thanks for letting me know. I just double checked. I usually check if the http response is in [200, 300). However, I explicitly check for a 201 if I create the job and I have a special treatment for 402 (no credits left).
So for this round I am safe. However, if you change the 201 or the 402 sim4life will not work anymore. That is bad.
@pcrespov @sanderegg what is a good best practice here?

Actually I change the 201 to a 202 in this PR. So this would probably break sim4life as I understand it. But maybe I should stick with a 201 in case a job is successfully started. I am sure @pcrespov has an opinion about that (?😉)

@mguidon as I client I would just check for a successful response , i.e. 2XX. That would include both

@bisgaard-itis bisgaard-itis requested a review from pcrespov June 19, 2024 09:49
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.

👍

@bisgaard-itis bisgaard-itis enabled auto-merge (squash) June 20, 2024 08:03
@bisgaard-itis bisgaard-itis merged commit e08c73e into ITISFoundation:master Jun 20, 2024
56 checks passed
@bisgaard-itis bisgaard-itis deleted the 5922-fix-start-job-endpoint branch June 20, 2024 14:26
@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Jul 5, 2024
26 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:apiserver api-server service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue wrt to start_job returning that job already has started, although it's the first time being called
5 participants