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: allow SDK to handle the Horde rc #141

Closed
wants to merge 3 commits into from
Closed

fix: allow SDK to handle the Horde rc #141

wants to merge 3 commits into from

Conversation

db0
Copy link
Member

@db0 db0 commented Feb 14, 2024

When returning errors, the horde now also sends back an rc along with the message. This appears to cause the SDK to barf

horde_sdk.ai_horde_api.exceptions.AIHordeRequestError: The response type doesn't match expected one! See `object_data` for the raw response.

This is an attempt to handle the rc returned.

@db0 db0 requested a review from tazlin February 14, 2024 21:07
@tazlin
Copy link
Member

tazlin commented Feb 15, 2024

Errors are handled differently because the API didn't publish (in the api docs) the 'message'-style responses. With the latest 'rc'-style error messages, this represents a fairly significant upset to the way this needs to be handled on the sdk side, especially considering it's going to cause errors to propagate to the worker.

Did you test the rc changes to the API with the sdk at all, or this change to the sdk?

@db0
Copy link
Member Author

db0 commented Feb 15, 2024

Well no, because the API didn't change. This is just an addition of an extra key on errors. I keep forgetting how strict the SDK is. The erros didn't change. the "message" should still be there.

@tazlin
Copy link
Member

tazlin commented Feb 15, 2024

The API did change - and pretty significantly. This "message" behavior is an emergent "feature" of the API which I was forced to cope with as a client. In many places, you don't publish the response or response code, and simply return a json string containing that field. Further, if I recall correctly, there are at least some models which also contain 'message' as a field. As a client, using object models, I can't determine which model to coerce the API's json response to given that reality without introducing some sort of check - in this case, that it only contains 'message'.

The API is supposed to be a contract. I can't foresee the future and expect these sorts of changes. For better or for worse, the SDK is inextricably tied to the worker at this time, so I expect that if you make API changes to this effect - at a minimum - the worker is checked against those API changes for backward compatibility.

@db0
Copy link
Member Author

db0 commented Feb 15, 2024

You don't need to scold me dude. It's not a huge deal. In any case, you now have a way to determine actual errors. but I'm not sure where you think I don't return a response code. I don't even think that's possible with http protocol

@tazlin
Copy link
Member

tazlin commented Feb 17, 2024

Superseded by another pull request.

@tazlin tazlin closed this Feb 17, 2024
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.

2 participants