feat: added helm chart #4065

Arvind644 · 2023-10-27T08:10:49Z

No description provided.

for more information, see https://pre-commit.ci

gabrielmbmb

Hi @Arvind644, thanks for the PR! Could we make the Elasticsearch deployment optional? I would like to toggle the Elasticsearch deployment based on a value of the Helm template.

k8s/helm chart/argilla/templates/argilla-server-deployment.yaml

k8s/helm chart/argilla/templates/argilla-server-hpa.yaml

Arvind644 · 2023-10-30T08:52:43Z

Hi @Arvind644, thanks for the PR! Could we make the Elasticsearch deployment optional? I would like to toggle the Elasticsearch deployment based on a value of the Helm template.

@gabrielmbmb Does I have to make optional deployment for all of these - elasticsearch-deployment.yaml , elasticsearch-pvc.yaml, elasticsearch-service.yaml or only for elasticsearch-deployment.yaml file.

…evelop

for more information, see https://pre-commit.ci

Arvind644 · 2023-11-02T15:34:07Z

@gabrielmbmb I have done all the changes, please review my PR.

jakemiller649

I've added a few suggestions from having deployed Argilla recently (I'm not part of Argilla team)

jakemiller649 · 2023-11-06T17:41:43Z

k8s/helm chart/argilla/templates/argilla-server-deployment.yaml

+          image: argilla/argilla-server:latest
+          env:
+            - name: {{ .Values.argilladeployment.name }}
+              value: "http://elasticsearch:9200"


This should also be Values.argilladeployment.elasticurl

https://github.com/bikash119/argilla/blob/70059f21bfd977ee726978ba74f44a4d3d026767/examples/deployments/k8s/argilla-chart/templates/deployment.yaml#L33-L35

jakemiller649 · 2023-11-06T17:42:26Z

k8s/helm chart/argilla/templates/argilla-server-deployment.yaml

+        - name: argilla-server
+          image: argilla/argilla-server:latest
+          env:
+            - name: {{ .Values.argilladeployment.name }}


This env name is required by Argilla and wouln't change between deployments. Leaving it as ARGILLA_ELASTICSEARCH should suffice.

https://github.com/bikash119/argilla/blob/70059f21bfd977ee726978ba74f44a4d3d026767/examples/deployments/k8s/argilla-chart/templates/deployment.yaml#L33

jakemiller649 · 2023-11-06T17:43:47Z

k8s/helm chart/argilla/templates/argilla-server-deployment.yaml

+    {{- end }}
+      containers:
+        - name: argilla-server
+          image: argilla/argilla-server:latest


Suggestion: swap latest for something like Values.imageTag in case users want to pin the image version (e.g., to 1.16 or something)

https://github.com/bikash119/argilla/blob/70059f21bfd977ee726978ba74f44a4d3d026767/examples/deployments/k8s/argilla-chart/templates/deployment.yaml#L31

jakemiller649 · 2023-11-06T17:44:54Z

k8s/helm chart/argilla/templates/argilla-server-deployment.yaml

+              value: "http://elasticsearch:9200"
+          ports:
+            - containerPort: 6900
+          resources:


Suggestions: resources often vary among deployments, and so making these into Values values is probably a good practice

https://github.com/bikash119/argilla/blob/70059f21bfd977ee726978ba74f44a4d3d026767/examples/deployments/k8s/argilla-chart/templates/deployment.yaml#L61-L62
https://github.com/bikash119/argilla/blob/70059f21bfd977ee726978ba74f44a4d3d026767/examples/deployments/k8s/argilla-chart/values.yaml#L6-L10

jakemiller649 · 2023-11-06T17:48:07Z

k8s/helm chart/argilla/templates/argilla-server-hpa.yaml

@@ -0,0 +1,18 @@
+apiVersion: autoscaling/v2


Suggestion: wrap HPAs in something like {{- if .Values.autoscaling.enabled }}

https://github.com/bikash119/argilla/blob/70059f21bfd977ee726978ba74f44a4d3d026767/examples/deployments/k8s/argilla-chart/templates/ingress.yaml#L1

jakemiller649 · 2023-11-06T17:50:41Z

k8s/helm chart/argilla/templates/argilla-server-deployment.yaml

@@ -0,0 +1,38 @@
+apiVersion: apps/v1


Suggestion: I would consider adding a PVC for the Argilla deployment so that workspaces/users persist beyond restarts/redeployments and mount it to ARGILLA_HOME_PATH.

https://github.com/bikash119/argilla/blob/70059f21bfd977ee726978ba74f44a4d3d026767/examples/deployments/k8s/argilla-chart/templates/deployment.yaml#L23-L28
https://github.com/bikash119/argilla/blob/70059f21bfd977ee726978ba74f44a4d3d026767/examples/deployments/k8s/argilla-chart/templates/deployment.yaml#L56-L59
https://github.com/bikash119/argilla/blob/70059f21bfd977ee726978ba74f44a4d3d026767/examples/deployments/k8s/argilla-chart/values.yaml#L15-L20

davidberenstein1957 · 2023-11-13T11:28:55Z

Thanks a lot for your help @jakemiller649 and @Arvind644 💪🏽

davidberenstein1957 · 2023-11-22T12:55:59Z

@Arvind644, are you able to continue work on this or can @gabrielmbmb, further fine-tune the contribution?

davidberenstein1957 · 2023-11-30T10:38:58Z

As per some suggestions of Robert Pack:

Ingress assumes NGINX, and would usually be optional
Allow for using Elastic operator to handle elastic deployment
Make “build in” elastic configurable – e.g. use VolumeClaim template
Improve parameter naming
Align HPA config with “conventions” – I.e. enable via flag and have static replicaCount variable if not activated.
Allow for injecting some configuration via configmaps / secrets etc …

bikash119 · 2024-09-21T03:49:24Z

As per some suggestions of Robert Pack:

* Ingress assumes NGINX, and would usually be optional

* Allow for using Elastic operator to handle elastic deployment

* Make “build in” elastic configurable – e.g. use VolumeClaim template

* Improve parameter naming

* Align HPA config with “conventions” – I.e. enable via flag and have static replicaCount variable if not activated.

* Allow for injecting some configuration via configmaps / secrets etc …

Ingress classname is configurable from values.yaml : ingressClassName: {{ .Values.argilla.ingress.className }}
Elastic Operator is used to have "built -in" ES : elasticsearch-operator.yaml
use PVC for "built in" ES : pvc.
static replicaCount
Use cm to inject configurations : /etc/hosts

davidberenstein1957 · 2024-10-17T07:25:20Z

closing because of #5512 . Thanks @bikash119 !

pre-commit-ci bot and others added 3 commits October 27, 2023 08:11

[pre-commit.ci] auto fixes from pre-commit.com hooks

8494dda

for more information, see https://pre-commit.ci

added helm chart

a3bcc4a

fixed indentation in Chart.yaml

cc31bb2

davidberenstein1957 requested a review from gabrielmbmb October 27, 2023 18:27

gabrielmbmb reviewed Oct 30, 2023

View reviewed changes

k8s/helm chart/argilla/templates/argilla-server-deployment.yaml Outdated Show resolved Hide resolved

k8s/helm chart/argilla/templates/argilla-server-hpa.yaml Outdated Show resolved Hide resolved

davidberenstein1957 added the team: ml Indicates that the issue or pull request is owned by the Machine Learning (ML) team label Oct 31, 2023

davidberenstein1957 assigned gabrielmbmb and Arvind644 Oct 31, 2023

Arvind644 and others added 4 commits November 2, 2023 20:51

fixes helm chart

5ecad78

Merge branch 'develop' of https://github.com/Arvind644/argilla into d…

984d753

…evelop

fixed helm chart readme

68b39aa

[pre-commit.ci] auto fixes from pre-commit.com hooks

387e480

for more information, see https://pre-commit.ci

davidberenstein1957 changed the title ~~added helm chart~~ feat: added helm chart Nov 6, 2023

jakemiller649 reviewed Nov 6, 2023

View reviewed changes

davidberenstein1957 requested a review from jakemiller649 November 13, 2023 11:29

bikash119 mentioned this pull request Sep 21, 2024

Helm chart #5512

Merged

davidberenstein1957 closed this Oct 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: added helm chart #4065

feat: added helm chart #4065

Arvind644 commented Oct 27, 2023

gabrielmbmb left a comment

Arvind644 commented Oct 30, 2023

Arvind644 commented Nov 2, 2023

jakemiller649 left a comment

jakemiller649 Nov 6, 2023

bikash119 Sep 21, 2024

jakemiller649 Nov 6, 2023

bikash119 Sep 21, 2024

jakemiller649 Nov 6, 2023

bikash119 Sep 21, 2024

jakemiller649 Nov 6, 2023

bikash119 Sep 21, 2024

jakemiller649 Nov 6, 2023

bikash119 Sep 21, 2024

jakemiller649 Nov 6, 2023

bikash119 Sep 21, 2024

davidberenstein1957 commented Nov 13, 2023 •

edited

Loading

davidberenstein1957 commented Nov 22, 2023

davidberenstein1957 commented Nov 30, 2023

bikash119 commented Sep 21, 2024

davidberenstein1957 commented Oct 17, 2024

feat: added helm chart #4065

feat: added helm chart #4065

Conversation

Arvind644 commented Oct 27, 2023

gabrielmbmb left a comment

Choose a reason for hiding this comment

Arvind644 commented Oct 30, 2023

Arvind644 commented Nov 2, 2023

jakemiller649 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidberenstein1957 commented Nov 13, 2023 • edited Loading

davidberenstein1957 commented Nov 22, 2023

davidberenstein1957 commented Nov 30, 2023

bikash119 commented Sep 21, 2024

davidberenstein1957 commented Oct 17, 2024

davidberenstein1957 commented Nov 13, 2023 •

edited

Loading