-
Notifications
You must be signed in to change notification settings - Fork 907
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
support for Hermes-2-Theta-Llama-3-8B
as default OSS model
#424
Conversation
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.
❌ Changes requested. Reviewed everything up to ce2bccf in 1 minute and 17 seconds
More details
- Looked at
646
lines of code in12
files - Skipped
1
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. .env.example:16
- Draft comment:
TheJWT_SHARED_KEY
is set to an empty value, which could lead to security vulnerabilities. Ensure that this key is properly set in production environments to maintain the integrity and security of JWT tokens.
JWT_SHARED_KEY=your_secure_key_here
- Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 50%.
2. .env.example:43
- Draft comment:
TheOPENAI_API_KEY
is set to an empty value, which might cause issues if the key is required for accessing OpenAI services. Ensure that this key is properly set in production environments to maintain functionality.
OPENAI_API_KEY=your_openai_api_key_here
- Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 50%.
3. model-serving/Dockerfile:5
- Draft comment:
TheMODEL_NAME
environment variable is set tojulep-ai/samantha-1-turbo
, which might not reflect the intended default modelHermes-2-Theta-Llama-3-8B
as per the PR title. Update this to ensure consistency across the deployment.
ENV MODEL_NAME julep-ai/Hermes-2-Theta-Llama-3-8B
- Reason this comment was not posted:
Confidence of 20% on close inspection, compared to threshold of 50%.
Workflow ID: wflow_accoWmUHIYzeF3u3
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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.
❌ Changes requested. Incremental review on debe630 in 54 seconds
More details
- Looked at
199
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_RKpv6Xwdh1HM5FWd
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
lgtm, some todos:
|
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.
👍 Looks good to me! Incremental review on e7ab98f in 47 seconds
More details
- Looked at
170
lines of code in6
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. agents-api/docker-compose.yml:64
- Draft comment:
The PR description states that thedocs-text-embeddings-inference
service is removed, but the Dockerfile still references thetext-embeddings-inference
service. Please confirm if this service is intended to be removed or if it should remain. - Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 50%.
Workflow ID: wflow_lHjGznKHEa5p4NSC
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Signed-off-by: Diwank Tomer <[email protected]>
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.
👍 Looks good to me! Incremental review on 5e17da9 in 47 seconds
More details
- Looked at
23
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. agents-api/agents_api/model_registry.py:216
- Draft comment:
The PR description mentions renamingJULEP_MODELS
toLOCAL_MODELS
, but the diff does not reflect this change. Please ensure that the renaming is correctly implemented throughout the codebase. - Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 50%.
Workflow ID: wflow_yZ99LHv667l7rVaM
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
tested |
Closes #410
Closes #413
Closes #406
Summary:
Updated default model to
Hermes-2-Theta-Llama-3-8B
and refactored related environment variables and model handling logic.Key points:
.env.example
to setMODEL_NAME
tojulep-ai/Hermes-2-Theta-Llama-3-8B
and adjusted related environment variables.agents-api/agents_api/activities/embed_docs.py
to useembedding_model_id
instead ofdocs_embedding_model_id
.agents-api/agents_api/activities/summarization.py
to replaceJULEP_MODELS
withLOCAL_MODELS
.agents-api/agents_api/embed_models_registry.py
to consolidate embedding service URLs and model IDs.agents-api/agents_api/env.py
to removedocs_embedding_service_url
anddocs_embedding_model_id
.JULEP_MODELS
toLOCAL_MODELS
inagents-api/agents_api/model_registry.py
and added new local models.agents-api/agents_api/prompt_assets/sys_prompt.yml
for system prompts.agents-api/agents_api/rec_sum/generate.py
to useLOCAL_MODELS
.agents-api/agents_api/routers/sessions/session.py
to handle tool calls and update settings.agents-api/docker-compose.yml
to removedocs-text-embeddings-inference
service.model-serving/Dockerfile
and updated entrypoint.model-serving/docker-compose.yml
to align with new environment variables.Generated with ❤️ by ellipsis.dev