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

Improve Helm Chart #3638

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Improve Helm Chart #3638

wants to merge 13 commits into from

Conversation

hofq
Copy link

@hofq hofq commented Aug 14, 2024

Pull Request Template

⚠️ Before Submitting a PR, Please Review:

  • Please ensure that you have thoroughly read and understood the Contributing Docs before submitting your Pull Request.

⚠️ Documentation Updates Notice:

  • Kindly note that documentation updates are managed in this repository: librechat.ai

Summary

For now, it's a draft to document my progress.
CC: @danny-avila, @iamNoah1

Done

  • Subcharts for Dependencies
  • Create RAG_API Subchart
  • Provide full getting started Guide
  • Keep Chart 1:1 Compatible to old Chart/ Document Changes with Version Change
  • Move Guide to Docs

Todo

  • Add Rag_Api Guide

Change Type

Please delete any irrelevant options.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Translation update

Testing

Please describe your test process and include instructions so that we can reproduce your test. If there are any important variables for your testing configuration, list them here.

Test Configuration:

Checklist

Please delete any irrelevant options.

  • I have performed a self-review of my own code
  • I have commented in any complex areas of my code
  • I have made pertinent documentation changes
  • I have written tests demonstrating that my changes are effective or that my feature works
  • Any changes dependent on mine have been merged and published in downstream modules.
  • A pull request for updating the documentation has been submitted.

@hofq hofq marked this pull request as draft August 14, 2024 14:09
@hofq
Copy link
Author

hofq commented Aug 15, 2024

@danny-avila @iamNoah1
I'm struggling a bid of choosing a way of the config syntax.

So in our chart for example we chose to place env config under librechat.configEnv and in the official chart its config.env.

No big deal, but everyone that uses the other chart has to manually migrate his config to the new format. Should I decide based on official/unofficial, user count of the chart, or make some dual-solution that just takes both config variants (maybe with an additional deprecation warning)?

@danny-avila
Copy link
Owner

Should I decide based on official/unofficial, user count of the chart, or make some dual-solution that just takes both config variants (maybe with an additional deprecation warning)?

I think this is pretty recent so whatever you choose shouldn’t be too disruptive. Maybe both just to be safe but I don’t think it’s totally necessary

@iamNoah1
Copy link
Contributor

iamNoah1 commented Aug 27, 2024

@hofq I don't have a strong opinion here. I would vote for easy solution, and a dual supporting structure seems too much for me. I would stick with what is there, unless there is a good reason to disrupt.

Copy link
Contributor

@iamNoah1 iamNoah1 left a comment

Choose a reason for hiding this comment

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

Wow, I made it through ^^ It took forever, sorry. But I think we can and should move on now :) Happy to help any further. @hofq what you think?!

# This is the chart version. This version number should be incremented each time you make changes
# to the chart and its templates, including the app version.
# Versions are expected to follow Semantic Versioning (https://semver.org/)
version: 0.1.5
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
version: 0.1.5
version: 0.1.0

Would suggest that we start with this version.

Copy link
Author

Choose a reason for hiding this comment

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

The Chart is already at another version in our chart repo. We should keep the versioning at that so we break less instances.

Copy link
Contributor

Choose a reason for hiding this comment

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

understood, makes sense

@@ -0,0 +1,4 @@
# Librechat RAG API Helm CHart
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if we really need this readme file

Copy link
Author

Choose a reason for hiding this comment

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

For Artifacthub its a nice to have, as it shows how to get started with the chart directly where you get it. https://artifacthub.io/packages/helm/librechat/librechat

Copy link
Contributor

Choose a reason for hiding this comment

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

understood

# modelDisplayLabel: "OpenRouter"


# # name of existing Yaml configmap, key must be librechat.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# # name of existing Yaml configmap, key must be librechat.yaml
# name of existing Yaml configmap, key must be librechat.yaml

fullnameOverride: ""

librechat:
# LibreChat allows Configuration in 2 Ways: Environment Variables and a Config file. For easier Deployment the needed values are predifined here but should be adjusted to your needs.
Copy link
Contributor

Choose a reason for hiding this comment

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

This should go into docs if not already present

Copy link
Author

Choose a reason for hiding this comment

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

as above it should be documented both in docs and here

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed




# # For adding a custom config yaml-file you can set the contents in this var. See https://www.librechat.ai/docs/configuration/librechat_yaml/example
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# # For adding a custom config yaml-file you can set the contents in this var. See https://www.librechat.ai/docs/configuration/librechat_yaml/example
# For adding a custom config yaml-file you can set the contents in this var. See https://www.librechat.ai/docs/configuration/librechat_yaml/example

nameOverride: ""
fullnameOverride: ""

librechat:
Copy link
Contributor

Choose a reason for hiding this comment

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

Generally I am a bit confused why we have global.librechat and librechat?! Feels a bit redundant, can we merge them and have only one? Preferably only librechat on the root level.

Copy link
Author

Choose a reason for hiding this comment

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

global is a directive defined by helm. If there are values that should be available everywhere you opt for the global directive. The Secret Names and API Key may also be used in the subchart

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we the librechat stuff from the root level then also to the global.librechat?

Copy link
Author

Choose a reason for hiding this comment

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

hmm don't know if i'd like that. would be a bid of for moving every var in global just for 2 secret names that have to be declared here. maybe we could use yaml anchors and move the vars into the normal librechat block?

@hofq
Copy link
Author

hofq commented Dec 21, 2024

Thanks @iamNoah1 :) I will look into it if i find time to rebase that whole pr. There where some Changes to our chart repo in the past which also should be included here.

Additionally maybe putting renovate on the table for that PR would also be nice

Copy link
Author

@hofq hofq left a comment

Choose a reason for hiding this comment

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

Just some notes, there are some changes already done in https://github.com/bat-bs/helm-charts which i have to adapt.

Thanks for the Review :)

# This is the chart version. This version number should be incremented each time you make changes
# to the chart and its templates, including the app version.
# Versions are expected to follow Semantic Versioning (https://semver.org/)
version: 0.1.5
Copy link
Author

Choose a reason for hiding this comment

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

The Chart is already at another version in our chart repo. We should keep the versioning at that so we break less instances.

@@ -0,0 +1,4 @@
# Librechat RAG API Helm CHart
Copy link
Author

Choose a reason for hiding this comment

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

For Artifacthub its a nice to have, as it shows how to get started with the chart directly where you get it. https://artifacthub.io/packages/helm/librechat/librechat

enabled: true
# nameOverride: vectordb
image:
registry: ghcr.io
Copy link
Author

Choose a reason for hiding this comment

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

I don't get this comment sorry

librechat:
# existing Secret for all envs/ only Passwords. Can be locally generated with: kubectl create secret generic librechat-secret-envs --from-env-file=.env.example --dry-run=client -o yaml > secret-envs.yaml
## For better maintainabillity, you can put all vars directly in the config Section and only overwrite Secrets with this if nessesary.
# Required Values:
Copy link
Author

Choose a reason for hiding this comment

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

Additionally, yeah. But setting up an app with a helm chart should be possible without looking in the docs.

nameOverride: ""
fullnameOverride: ""

librechat:
Copy link
Author

Choose a reason for hiding this comment

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

global is a directive defined by helm. If there are values that should be available everywhere you opt for the global directive. The Secret Names and API Key may also be used in the subchart

fullnameOverride: ""

librechat:
# LibreChat allows Configuration in 2 Ways: Environment Variables and a Config file. For easier Deployment the needed values are predifined here but should be adjusted to your needs.
Copy link
Author

Choose a reason for hiding this comment

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

as above it should be documented both in docs and here

@hofq
Copy link
Author

hofq commented Feb 10, 2025

@hofq
Copy link
Author

hofq commented Feb 10, 2025

Hi Sorry for my Late Comment and Changes :/

I'm no longer employed with Blue Atlas as the Company will not persist to exist. However i will assist and move the Migration of our Chart forward in my free time.

I added most of your suggested changes @iamNoah1 bumped our base chart and added error handling. Hope you will find time to have a look again :)

@hofq hofq marked this pull request as ready for review February 10, 2025 00:34
@hofq hofq requested a review from iamNoah1 February 10, 2025 00:35
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