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

fix: Separates sync/async error handlers and makes 3 simple calls sync #819

Conversation

gustavoloureirodosreis
Copy link

Description

This PR improves the error handling system in the HTTP API, as a first step to solve issue #569. The motivation is to provide better support for both synchronous and asynchronous endpoints while maintaining backwards compatibility. Key changes include:

Error Handling Improvements:

  • Introduced handle_common_exceptions() for shared error-handling logic.
  • Added with_sync_route_exceptions() for synchronous endpoints.
  • Added with_async_route_exceptions() for asynchronous endpoints.
  • Maintained with_route_exceptions() for backward compatibility.

Endpoint Adjustments:

Converted /info, /model/registry, and /device/stats to synchronous endpoints.

No new dependencies were introduced for this change.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

How has this change been tested, please provide a testcase or example of how you tested the change?

Testing was done using Docker to verify endpoint behavior:

Pre-change testing: Ensured the baseline functionality and error responses of all endpoints.
Post-change testing: Verified that the endpoints were still performing as they should.

Endpoints changed and tested:

/info (static version info)
/model/registry (in-memory model listing)
/device/stats (Docker stats via synchronous SDK)

Any specific deployment considerations

No changes in usability, usage, costs, or secrets were required. The inline documentation now reflects the synchronous or asynchronous nature of the handlers.

Docs

  • Docs updated? What were the changes:

Updated to specify the new sync/async nature of endpoints.
Maintained backward compatibility documentation.

@CLAassistant
Copy link

CLAassistant commented Nov 16, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@PawelPeczek-Roboflow PawelPeczek-Roboflow left a comment

Choose a reason for hiding this comment

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

Hello there, thanks for contribution - it is really appreciated.

This is a good refactor and the only comment I have is to push the improved error handlers into separate module, let's say inference/core/interfaces/http/handlers/errors.py

@PawelPeczek-Roboflow
Copy link
Collaborator

And could you also accept CLA - without that we cannot move forward

@PawelPeczek-Roboflow
Copy link
Collaborator

Hi there, since this comment is not applied, pushing to the next release

@PawelPeczek-Roboflow
Copy link
Collaborator

hello there - will you be willing to carry on with work around this PR?

@gustavoloureirodosreis
Copy link
Author

gustavoloureirodosreis commented Nov 25, 2024 via email

@PawelPeczek-Roboflow
Copy link
Collaborator

ok, thanks for confirming

@PawelPeczek-Roboflow PawelPeczek-Roboflow added enhancement New feature or request and removed release 0.29.0 labels Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants