bug fix in inference_endpoint wait function for proper waiting on update #2867
Conversation
There was a problem hiding this comment.
Hi @Ajinkya-25, thanks a lot for your contribution, I left comments to make the approach more robust :)
also, could you add a test in huggingface_hub/tests/test_inference_endpoints.py similar to this one please?
| if response.status_code == 200 and self.status not in [ | ||
| InferenceEndpointStatus.UPDATING, | ||
| InferenceEndpointStatus.UPDATE_FAILED, | ||
| ]: |
There was a problem hiding this comment.
this could allow some intermediate states (like INITIALIZING) to pass through if they return a 200 status code (not sure if it's the case)
There was a problem hiding this comment.
Thanks For Reviewing And Suggesting The Changes. I have implemented the suggested changes as they provide a more robust solution to the bug.
improve code clarity and added logging based on review Co-authored-by: Célina <[email protected]>
…ddition of test case in test_inference_endpoint for testing changes in wait function
Ajinkya-25
force-pushed
the
fix-endpoint-wait-update
branch
from
b0e8889 to
e565f45
Compare
There was a problem hiding this comment.
Good and clean PR, thanks @Ajinkya-25 ! I've left two comments in the test section as I think we should focus the test on "if an endpoint is updating, we should not return until it is "running" again, even if an URL is already provisioned for it".
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
| responses = chain( | ||
| [InferenceEndpoint.from_raw(MOCK_UPDATE, namespace="foo")] * 3, | ||
| repeat(InferenceEndpoint.from_raw(MOCK_RUNNING, namespace="foo")), | ||
| ) | ||
| mock_get_inference_endpoint.side_effect = lambda *args, **kwargs: next(responses) |
There was a problem hiding this comment.
Though this is perfectly valid, I prefer the way you did it previously with an explicit list (it makes it more readable IMO). Is the problem that you need to repeat the last side effect (RUNNING) an unknown number of times?
There was a problem hiding this comment.
I first used a list with three MOCK_UPDATE responses, but it caused a StopIteration error. After some research, I realized it was because the iterator ran out of values. Switching to chain() and repeat() fixed it by making sure the test keeps returning MOCK_RUNNING after the updates, preventing the error. Still learning testing, but this worked well! 🚀. Also this test case needs to set status_code to 200 for testing and as i am writing test case first time it was hard to set it as 200 can some function be added to testing files to set all this parameters(maybe a new feature). Thanks for the support.
There was a problem hiding this comment.
LGTM but let's wait for @hanouticelina 's final review before merging :)
#2767
Description of Changes in InferenceEndpoint.wait()
Problem Statement:
Previously, the wait() function in the InferenceEndpoint class waited for the endpoint to become available but did not properly handle the case where the endpoint was updating. This could lead to premature exits before the update was fully completed.
Changes Implemented:
Added a check for InferenceEndpointStatus.UPDATING and InferenceEndpointStatus.UPDATE_FAILED
Before considering the endpoint as ready, the function now ensures that the endpoint is not in the updating state.
This prevents returning an endpoint that is still in the process of updating.
Potential Issues Fixed:
Previously, wait() could return immediately after an update request, even if the endpoint was still being updated.
The function is now more robust and ensures that users do not interact with an endpoint that is still updating.