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

[BUG-python/deployment] backwards compatibility src/argilla/client/sdk/v1/datasets/api.py get_metadata_properties #4073

Closed
davidberenstein1957 opened this issue Oct 27, 2023 · 3 comments · Fixed by #4325
Assignees
Labels
area: api Indicates that an issue or pull request is related to the Fast API server or REST endpoints team: backend Indicates that the issue or pull request is owned by the backend team type: bug Indicates an unexpected problem or unintended behavior
Milestone

Comments

@davidberenstein1957
Copy link
Member

davidberenstein1957 commented Oct 27, 2023

Describe the bug
Whenever retrieving a dataset from server 1.17 with client 1.19 there seems to be an incompatibility issue when calling response.json. I think this might be a more recurrent issue, where for some reason the response does return with response.status_code==200 even though the endpoint is not defined in FastAPI. Hence this might also occur with the semantic search update.

Stacktrace and Code to create the bug

def get_metadata_properties(

Try loading a dataset from the 1.17 server with a 1.19 client.

Expected behavior
A clear and concise description of what you expected to happen.

Environment:

  • Argilla Version [e.g. 1.0.0]: 1.19
  • ElasticSearch Version [e.g. 7.10.2]: N.A.
  • Docker Image (optional) [e.g. argilla:v1.0.0]: 1.17

Additional context
N.A.

@davidberenstein1957 davidberenstein1957 added type: bug Indicates an unexpected problem or unintended behavior area: api Indicates that an issue or pull request is related to the Fast API server or REST endpoints labels Oct 27, 2023
@davidberenstein1957
Copy link
Member Author

ahh nop, is the 404 page
is the frontend redirection
Ideally, the frontend should be mounted in a different path for example argilla.io/ui. for the ui and argilla.io/api for the api. This will simplify to handle this kind of problems

@davidberenstein1957 davidberenstein1957 added the team: backend Indicates that the issue or pull request is owned by the backend team label Nov 8, 2023
@frascuchon
Copy link
Member

The 404 page returned is returning a 200 status code, but it should return a 404. Not sure if this can be handled by the frontend application in an easy way @damianpumar

curl -I -XGET http://localhost:6900/bad-url
HTTP/1.1 200 OK
date: Sat, 11 Nov 2023 14:41:43 GMT
server: uvicorn
content-type: text/html; charset=utf-8
content-length: 3343
last-modified: Sat, 11 Nov 2023 14:38:57 GMT
etag: 203c3b25094c8251a8ea2f427745a917

@frascuchon frascuchon added this to the v1.20.0 milestone Nov 11, 2023
@davidberenstein1957
Copy link
Member Author

This seems related #4302

@frascuchon frascuchon linked a pull request Nov 27, 2023 that will close this issue
8 tasks
frascuchon added a commit that referenced this issue Nov 27, 2023
<!-- Thanks for your contribution! As part of our Community Growers
initiative 🌱, we're donating Justdiggit bunds in your name to reforest
sub-Saharan Africa. To claim your Community Growers certificate, please
contact David Berenstein in our Slack community or fill in this form
https://tally.so/r/n9XrxK once your PR has been merged. -->

# Description

This PR adds a callback controller to return a proper status code and
error message when a client requests a non-existing endpoint.

This change will help identify incompatibility problems when using new
clients over old server instances as suggested in #4073

Closes #4073

**Type of change**

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

- [X] Bug fix (non-breaking change which fixes an issue)
- [ ] Breaking change (fix or feature that would cause existing
functionality to not work as expected)

**How Has This Been Tested**

Calling unexisting endpoints locally 

**Checklist**

- [X] I followed the style guidelines of this project
- [X] I did a self-review of my code
- [X] My changes generate no new warnings
- [X] I have added tests that prove my fix is effective or that my
feature works
- [ ] I filled out [the contributor form](https://tally.so/r/n9XrxK)
(see text above)
- [ ] I have added relevant notes to the `CHANGELOG.md` file (See
https://keepachangelog.com/)

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
davidberenstein1957 pushed a commit that referenced this issue Nov 29, 2023
<!-- Thanks for your contribution! As part of our Community Growers
initiative 🌱, we're donating Justdiggit bunds in your name to reforest
sub-Saharan Africa. To claim your Community Growers certificate, please
contact David Berenstein in our Slack community or fill in this form
https://tally.so/r/n9XrxK once your PR has been merged. -->

# Description

This PR adds a callback controller to return a proper status code and
error message when a client requests a non-existing endpoint.

This change will help identify incompatibility problems when using new
clients over old server instances as suggested in #4073

Closes #4073

**Type of change**

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

- [X] Bug fix (non-breaking change which fixes an issue)
- [ ] Breaking change (fix or feature that would cause existing
functionality to not work as expected)

**How Has This Been Tested**

Calling unexisting endpoints locally 

**Checklist**

- [X] I followed the style guidelines of this project
- [X] I did a self-review of my code
- [X] My changes generate no new warnings
- [X] I have added tests that prove my fix is effective or that my
feature works
- [ ] I filled out [the contributor form](https://tally.so/r/n9XrxK)
(see text above)
- [ ] I have added relevant notes to the `CHANGELOG.md` file (See
https://keepachangelog.com/)

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: api Indicates that an issue or pull request is related to the Fast API server or REST endpoints team: backend Indicates that the issue or pull request is owned by the backend team type: bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants