ai-cfia / nachet-backend

A flask-based backend for Nachet to handle Azure endpoint and Azure storage API requests from the frontend.
MIT License
1 stars 3 forks source link

Sylvanie85/issue61 #82

Closed sylvanie85 closed 2 months ago

sylvanie85 commented 2 months ago

I did some refactoring as my first issue on nachet-backend. The issue #61 was deprecated because the code has changed a lot since it was created. But I did make some fixes to the exception handling.

Francois-Werbrouck commented 2 months ago

Can we move the Error class definitions into a separate module so that we are not redefining them in multiple files?

There is a custom_exceptions.py file

The issue this PR is based upon #61 which basically has the goal to remove the custom_exceptions file and redistribute the error to be module based instead

ChromaticPanic commented 2 months ago

Can we move the Error class definitions into a separate module so that we are not redefining them in multiple files? There is a custom_exceptions.py file

The issue this PR is based upon #61 which basically has the goal to remove the custom_exceptions file and redistribute the error to be module based instead

That's okay for exceptions unique to a module, but some exceptions are shared. For example we do not redefine KeyError because that is something that is common.

For example the proposed approach has ApiErrors defined in multiple files. That seems wrong on so many levels.

ChromaticPanic commented 2 months ago

Can we move the Error class definitions into a separate module so that we are not redefining them in multiple files? There is a custom_exceptions.py file

The issue this PR is based upon #61 which basically has the goal to remove the custom_exceptions file and redistribute the error to be module based instead

I can elaborate on the issue as follows

Module 1 defines ApiError Module 2 defines ApiError Module 3 imports Module 1 and 2 and wants to handle the exceptions = name collision There are also now 2 potential definitions of what ApiError means, when the reality is that they probably both refer to an issue with connecting an azure end point

Now if ApiError in Module 1 and 2 are in fact different but kinda related then they should inherit from a common parent ApiError but then they should have their own name

Francois-Werbrouck commented 2 months ago

Can we move the Error class definitions into a separate module so that we are not redefining them in multiple files? There is a custom_exceptions.py file

The issue this PR is based upon #61 which basically has the goal to remove the custom_exceptions file and redistribute the error to be module based instead

That's okay for exceptions unique to a module, but some exceptions are shared. For example we do not redefine KeyError because that is something that is common.

For example the proposed approach has ApiErrors defined in multiple files. That seems wrong on so many levels.

Reviewing the changes I agree. The custom_exception file had too many exceptions in it, which are now module based. However defining APIErrors in 6+ files seems wrong.

sylvanie85 commented 2 months ago

Based on the discussion with @ChromaticPanic we should have a module to inherit the reccuring error Ex: class ProcessInferenceResultsError(APIErrors) :

So i need to create a new file to inherit the exception as was custom_exeptions ?

ChromaticPanic commented 2 months ago

Based on the discussion with @ChromaticPanic we should have a module to inherit the reccuring error Ex: class ProcessInferenceResultsError(APIErrors) :

So i need to create a new file to inherit the exception as was custom_exeptions ?

Yeah I would make that common ApiError

But also in each file I would use unique names for example in swin.py instead of ProcessInferenceResultsError(ApiError) I would name it SwinError(ApiError) or SwinApiError (I don't know at this point how many error types will be thrown by each module) Then it would work with the idea of having unique module errors defined in #61