Skip to content

scheduler: move OpenAI recording after runner load/release #94

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

Merged
merged 1 commit into from
Jun 27, 2025

Conversation

doringeman
Copy link
Contributor

If the runner is defunct, then loader.load will evict it, which means it'll also remove the recorded OpenAI requests/responses for that model. Hence, record the request after loader.load returns so it doesn't get removed in the loading process.

Without this patch, following these steps will result in a lost pair of request-response, the first one after the runner exited/was killed and had to be evicted because it was defunct.

$ MODEL_RUNNER_PORT=8080 make run # in a separate terminal

$ MODEL_RUNNER_HOST=http://localhost:8080 docker model run ai/smollm2 hi

$ curl -s http://localhost:8080/engines/requests\?model\=ai/smollm2 # all good, 1 record

$ pkill com.docker.llama-server

$ MODEL_RUNNER_HOST=http://localhost:8080 docker model run ai/smollm2 hi

$ # See `ERRO[0011] Model ai/smollm2 not found in records - 200` in the terminal where model-runner is running.
# This is because the request was recorded, then `loader.load` removed it, 
and then via a deferred function we attempted to record the response, but no matching request was found.
# Hence, we lost the first pair of request-response.

$ curl -s http://localhost:8080/engines/requests\?model\=ai/smollm2
No records found for model 'ai/smollm2'

Signed-off-by: Dorin Geman <dorin.geman@docker.com>
@doringeman doringeman requested a review from a team June 26, 2025 12:28
Copy link
Member

@p1-0tr p1-0tr left a comment

Choose a reason for hiding this comment

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

LGTM

@doringeman doringeman merged commit fcf45f4 into docker:main Jun 27, 2025
1 check passed
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.

4 participants