-
-
Notifications
You must be signed in to change notification settings - Fork 102
[Platform] Implement RateLimitExceededException
handling across AI platforms
#538
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
Conversation
afa7b21
to
d07b962
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be nice to document ResultConverterInterface::convert that it can throws exception then.
With something like
/**
* @param array<string, mixed> $options
*
* @throws RateExceededException when rate limit exceeeded
* @throws RuntimeException when another error occured
*/
public function convert(RawResultInterface $result, array $options = []): ResultInterface;
ba6e445
to
416c955
Compare
lets call it |
@OskarStark installation of PHPStan is flakky, don't know if you can launch again the CI. |
RateLimitExceededException
handling across AI platforms
Agreed, will include it in a try to have a higher level ResultConverter. |
/** | ||
* @author Floran Pagliai <floran.pagliai@gmail.com> | ||
*/ | ||
final class RateLimitExceededException extends \RuntimeException implements ExceptionInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't see this as a RuntimeException tho
final class RateLimitExceededException extends \RuntimeException implements ExceptionInterface | |
final class RateLimitExceededException extends \InvalidArgumentException implements ExceptionInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get your point, but I’m not sure \InvalidArgumentException is the best fit here. A rate limit being exceeded isn’t really about invalid input, it’s more of an external condition depending of usage. That’s why I went with \RuntimeException.
Another option could be to extend directly from a generic \Exception or define a dedicated base exception for API-related errors ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe my point of view, but i consider
- a LogicException being an exception which should never happen unless bad code is written, and the dev should fix his code. (Bad param, bad call, case which shouldn't exist, etc)
- a RuntimeException being an exception which is not avoidable and might occurs under some circonstances
Here, it seems to be a Runtime one to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But what about extending directly the symfony/ai RuntimeException
https://github.com/symfony/ai/blob/main/src/platform/src/Exception/RuntimeException.php
rather than extending \RuntimeException
and implementing the ExceptionInterface ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Went with directly extending https://github.com/symfony/ai/blob/main/src/platform/src/Exception/RuntimeException.php
40d44b4
to
52b43da
Compare
Thank you @floranpagliai. |
…ntLanglet) This PR was merged into the main branch. Discussion ---------- [Agent][Platform] Improve `ResultConverter`PHPDoc | Q | A | ------------- | --- | Bug fix? | no | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | Docs? | yes | Issues | Fix #... | License | MIT Since #538 exceptions thrown by `ResultConverterInterface::convert` are more useful and should be documented. I personally use them when doing `Agent::call`, and they are accessible because Agent::call use `getResult` which use `await` which use `convert`. I added some basic documentation about them then. Commits ------- 10ce6b3 Improve ResultConverter phpdoc
RateLimitExceededException
to handle 429 across all AI platforms