-
Notifications
You must be signed in to change notification settings - Fork 258
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
feat/multi llm enhancement #412
Conversation
for more information, see https://pre-commit.ci
…ract into FEAT/multi-llm-enhancement
for more information, see https://pre-commit.ci
…ract into FEAT/multi-llm-enhancement
for more information, see https://pre-commit.ci
@jagadeeswaran-zipstack Make sure to update the branch with latest main and resolve frontend build issues. Also there were some pre-commit issues. |
for more information, see https://pre-commit.ci
…ract into FEAT/multi-llm-enhancement
backend/prompt_studio/prompt_studio_core/prompt_studio_helper.py
Outdated
Show resolved
Hide resolved
backend/prompt_studio/prompt_studio_core/prompt_studio_helper.py
Outdated
Show resolved
Hide resolved
backend/prompt_studio/prompt_studio_core/prompt_studio_helper.py
Outdated
Show resolved
Hide resolved
for more information, see https://pre-commit.ci
backend/prompt_studio/prompt_studio_core/prompt_studio_helper.py
Outdated
Show resolved
Hide resolved
…ract into FEAT/multi-llm-enhancement
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.
LGTM - left some NIT comments. Make sure the polling values for FE is reviewed by @hari-kuriakose
frontend/src/components/custom-tools/prompt-card/PromptCard.jsx
Outdated
Show resolved
Hide resolved
frontend/src/components/custom-tools/prompt-card/PromptCard.jsx
Outdated
Show resolved
Hide resolved
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.
LGTM. However, I have a major concern about the long polling. It would be better to either move it to the front end or implement a webhook method.
cc: @jagadeeswaran-zipstack @hari-kuriakose
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.
Suggested a few improvements. Will approve the PR once they're addressed.
frontend/src/components/custom-tools/combined-output/CombinedOutput.jsx
Outdated
Show resolved
Hide resolved
frontend/src/components/custom-tools/combined-output/JsonView.jsx
Outdated
Show resolved
Hide resolved
frontend/src/components/custom-tools/output-for-doc-modal/OutputForDocModal.jsx
Outdated
Show resolved
Hide resolved
frontend/src/components/custom-tools/prompt-card/PromptCardItems.jsx
Outdated
Show resolved
Hide resolved
|
|
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.
LGTM
What
Multi llm enhancement consists of the following changes:
Why
How
Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
Database Migrations
Env Config
INDEXING_FLAG_TTL=1800
Relevant Docs
Related Issues or PRs
Dependencies Versions
Notes on Testing
Screenshots
Checklist
I have read and understood the Contribution Guidelines.