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]: Handle Error response from AI API #548

Merged
merged 8 commits into from
Sep 17, 2024

Conversation

VipinDevelops
Copy link
Contributor

@VipinDevelops VipinDevelops commented Sep 15, 2024

Linked Issue(s)

#546

Acceptance Criteria fulfillment

  • Handle the AI API response in the backend, ensuring proper error handling.
  • Handle the BFF response for DORA metrics ensuring proper error feedback to the frontend.

Proposed changes (including videos or screenshots)

Screencast.from.2024-09-15.11-54-08.webm

let status = 'success';
let message = '';

for (const value of Object.values(responses)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

const errorResponse = Object.values(responses))
                      .find(res => res.status==='error')

const {status = 'success', message = ''} = errorResponse ?? {} 
// if `errorResponse` is not found, default it to an empty object

Copy link
Contributor

@e-for-eshaan e-for-eshaan Sep 16, 2024

Choose a reason for hiding this comment

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

we rarely use for and while

Copy link
Contributor

@e-for-eshaan e-for-eshaan Sep 16, 2024

Choose a reason for hiding this comment

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

in fact use an if statement

if(errorResponse)
  return {status: errorResponse.status, message: errorResponse.message}

return {status:"success", message:""}

});

function checkForErrors(responses: any): { status: string; message: string } {
Copy link
Contributor

@e-for-eshaan e-for-eshaan Sep 16, 2024

Choose a reason for hiding this comment

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

usually we use

const func = () => {}

@e-for-eshaan
Copy link
Contributor

@samad-yar-khan will submit the final review


def _fetch_completion(
self, messages: List[Dict[str, str]]
) -> Union[Dict[str, str], Dict[str, Union[str, int]]]:
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems over complicated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'll try to simplify it


def get_dora_metrics_score(
self, four_keys_data: Dict[str, float]
) -> Dict[str, str]:
) -> Union[Dict[str, str], Dict[str, Union[str, int]]]:
Copy link
Contributor

Choose a reason for hiding this comment

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

although this serves our usecase, this return type feels like an overkill

Copy link
Contributor

Choose a reason for hiding this comment

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

and super hard to decipher

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will make it simpler TBH I don't like this either

Copy link
Contributor

@samad-yar-khan samad-yar-khan left a comment

Choose a reason for hiding this comment

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

LGTM, but I think a lot of this can be made better by better exception handling, lets take those up in future PRs: https://flask.palletsprojects.com/en/2.3.x/errorhandling/

@e-for-eshaan
Copy link
Contributor

@VipinDevelops merge this then!

@VipinDevelops VipinDevelops merged commit c5657d0 into middlewarehq:main Sep 17, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants