Skip to content

Conversation

Alex-Welsh
Copy link
Member

No description provided.

@Alex-Welsh Alex-Welsh requested a review from sd109 January 29, 2025 16:29
@sd109
Copy link
Member

sd109 commented Feb 12, 2025

CI is failing due to critical CVE in built container images which is fixed in #73 so once that's merged we can rebase this and make sure the CI passes.

@sd109 sd109 changed the title Fix llmparams default values, add CI Fix llmparams default values, add additional chart values to CI test matrix Feb 12, 2025
@sd109
Copy link
Member

sd109 commented Feb 12, 2025

Actually, I think the changes in this PR might contradict this comment so maybe we need to think about how to set sensible defaults more carefully.

I think that means we probably just want to set the default LLM params here for the chat app only, rather than at the level of the base LLMParams python object which is shared by both the chat and image-analysis apps.

@Alex-Welsh
Copy link
Member Author

Actually, I think the changes in this PR might contradict this comment so maybe we need to think about how to set sensible defaults more carefully.

I think that means we probably just want to set the default LLM params here for the chat app only, rather than at the level of the base LLMParams python object which is shared by both the chat and image-analysis apps.

Should we do this for all params or just top_k?

@sd109
Copy link
Member

sd109 commented Feb 13, 2025

I guess it probably makes sense to set all of them to chat app Azimuth defaults documented here for consistency.

Copy link
Member

@sd109 sd109 left a comment

Choose a reason for hiding this comment

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

LGTM

@sd109 sd109 changed the title Fix llmparams default values, add additional chart values to CI test matrix Various bug fixes Feb 13, 2025
@sd109
Copy link
Member

sd109 commented Feb 18, 2025

Superceded by #76

@sd109 sd109 closed this Feb 18, 2025
@sd109 sd109 deleted the llmparams-defaults branch February 18, 2025 14:12
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