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: add error code when searching for a record missing specific vector #4856

Merged
merged 6 commits into from
May 23, 2024

Conversation

jfcalvo
Copy link
Member

@jfcalvo jfcalvo commented May 20, 2024

Description

An error is raised from backend when trying to find similar records when the vector is not defined for the used record. The error is fine but an additional attribute is needed on the front to support mapping that specific error to a translated message to the user.

This PR adds a new custom error using message and code attributes to be rendered as a JSON response.

Closes #4655

Type of change

(Please delete options that are not relevant. Remember to title the PR according to the type of change)

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (change restructuring the codebase without changing functionality)
  • Improvement (change adding some improvement to an existing functionality)
  • Documentation update

How Has This Been Tested

(Please describe the tests that you ran to verify your changes. And ideally, reference tests)

  • Modifying existents tests.

Checklist

  • I added relevant documentation
  • follows the style guidelines of this project
  • I did a self-review of my code
  • I made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I filled out the contributor form (see text above)
  • I have added relevant notes to the CHANGELOG.md file (See https://keepachangelog.com/)

Frontend handling

Argilla

@jfcalvo jfcalvo requested a review from frascuchon May 20, 2024 14:09
Copy link

codecov bot commented May 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.95%. Comparing base (3404dbe) to head (1d520c9).
Report is 3 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4856      +/-   ##
===========================================
+ Coverage    90.94%   90.95%   +0.01%     
===========================================
  Files          205      206       +1     
  Lines        10082    10098      +16     
===========================================
+ Hits          9169     9185      +16     
  Misses         913      913              
Flag Coverage Δ
argilla-server 90.95% <100.00%> (+0.01%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

async def unprocessable_entity_error_exception_handler(request, exc):
return JSONResponse(
status_code=status.HTTP_422_UNPROCESSABLE_ENTITY,
content={"code": exc.code, "message": exc.message},
Copy link
Member Author

Choose a reason for hiding this comment

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

One suggestion from @frascuchon is the possibility of returning a content compatible with the API v0 errors:

Suggested change
content={"code": exc.code, "message": exc.message},
content={
"detail": {
"code": exc.code,
"params": {
"detail": exc.message
},
},
},

Add a comment as well noticing how we should return to the simple version once we remove API v0.

Copy link
Member Author

Choose a reason for hiding this comment

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

After discussing it with @damianpumar we agreed into left this new more simple way of sending the error. Damián will manage this without any problem and in the future we can improve and remove the old API v0 errors structure.

Copy link

The URL of the deployed environment for this PR is https://argilla-quickstart-pr-4856-ki24f765kq-no.a.run.app

@jfcalvo jfcalvo merged commit ffd25e5 into develop May 23, 2024
11 of 12 checks passed
@jfcalvo jfcalvo deleted the fix/filtering-by-missing-record-vector branch May 23, 2024 11:40
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.

[BUG-UI/UX] Filtering by vectors
3 participants