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

add model service check #41

Merged
merged 3 commits into from
Feb 19, 2024
Merged

Conversation

MichaelClifford
Copy link
Collaborator

This PR adds a check to the application to make sure the model service is running before proceeding. Once this PR is approved, we can roll out this same approach on the rest of the AI applications.

@jeffmaury
Copy link
Collaborator

Not sure this is related to this PR but the model service does not start:

image

@jeffmaury
Copy link
Collaborator

jeffmaury commented Feb 12, 2024

And seems the client application has opened its HTTP port:

image

image

@MichaelClifford
Copy link
Collaborator Author

MichaelClifford commented Feb 12, 2024

Not sure this is related to this PR but the model service does not start:

image

This PR does not make any changes to the model service. However, it looks like you may have added the environment variables incorrectly. This model service is the same as the playground, so it should be deployed the same way.

podman run -it -p 8001:8001 -v <LOCAL_PATH>locallm/models:/locallm/models:Z -e MODEL_PATH=models/mistral-7b-instruct-v0.1.Q4_K_M.gguf -e HOST=0.0.0.0 -e PORT=8001 llamacppserver

@MichaelClifford
Copy link
Collaborator Author

And seems the client application has opened its HTTP port:

image

image

yes, but it should not load the chat interface until the model service is ready. Do we need to have a stricter check in place?

@MichaelClifford
Copy link
Collaborator Author

Added an additional commit that incudes the same check in chatbot/ai_applications/chat_ui.py.

Copy link
Collaborator

@sallyom sallyom left a comment

Choose a reason for hiding this comment

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

LGTM

@jeffmaury
Copy link
Collaborator

@MichaelClifford can you rebase it so that it includes my latest patch

@MichaelClifford
Copy link
Collaborator Author

@jeffmaury rebased 😄

@jeffmaury
Copy link
Collaborator

LGTM, but the client application opens port 8501 before the model is loaded and if you use browser, you have this:

image

which is ok but I was wondering is there would be an option to replace checking_model_service by a more user friendly sentence

@jeffmaury
Copy link
Collaborator

Another suggestion: would also be good to have the time spent to get the model service available at the end of loop

@MichaelClifford
Copy link
Collaborator Author

@jeffmaury How does this look?

Screenshot 2024-02-14 at 11 27 00 AM

Copy link
Collaborator

@lstocchi lstocchi left a comment

Choose a reason for hiding this comment

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

Works fine for me

Copy link
Collaborator

@jeffmaury jeffmaury left a comment

Choose a reason for hiding this comment

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

LGTM

@lstocchi
Copy link
Collaborator

merging as it has been approved by everyone. Thanks @MichaelClifford!!

@lstocchi lstocchi merged commit b0b2eca into containers:main Feb 19, 2024
2 checks 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