Skip to content

feat(cogstack-cohorter): Add helm chart for cogstack-cohorter#75

Merged
jocelyneholdbrook merged 8 commits intomainfrom
feat/cohorter/add-helm-chart
Apr 20, 2026
Merged

feat(cogstack-cohorter): Add helm chart for cogstack-cohorter#75
jocelyneholdbrook merged 8 commits intomainfrom
feat/cohorter/add-helm-chart

Conversation

@jocelyneholdbrook
Copy link
Copy Markdown
Contributor

No description provided.

Comment thread .github/workflows/kubernetes-charts-build.yaml Outdated
Comment thread helm-charts/cogstack-cohorter-helm/Chart.yaml Outdated
Copy link
Copy Markdown
Collaborator

@alhendrickson alhendrickson left a comment

Choose a reason for hiding this comment

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

Approved if you want to merge it and publish it out.

I think the main comment would be if you can redo some based on the output of helm create .., which should generate a bunch of other stuff like the test, httproute etc. Convention seems to be keep it all super standardised

Comment thread helm-charts/cogstack-cohorter-helm/README.md Outdated
pathType: ImplementationSpecific
tls: []

autoscaling:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Move under the webapp?

Comment thread helm-charts/cogstack-cohorter-helm/values.yaml Outdated
Comment thread helm-charts/cogstack-cohorter-helm/templates/ingress.yaml
Copy link
Copy Markdown
Collaborator

@alhendrickson alhendrickson left a comment

Choose a reason for hiding this comment

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

Well - I'd approve if it passed the check anyway!

------------------------------------------------------------------------------------------------------------------------
==> Logs of container cogstack-cohorter-helm-z581pqgk5y-webapp-58587cd997-fcndf
------------------------------------------------------------------------------------------------------------------------
[webapp] ERROR: No data found at /usr/src/app/server/data.
[webapp] Mount a directory containing snomed_terms.json (and related files)
[webapp] or snomed_terms_data.tar.gz via a Docker volume:
[webapp]   -v /your/data:/usr/src/app/server/data

@alhendrickson
Copy link
Copy Markdown
Collaborator

You can add a file like your-chart/ci/ci-values.yaml to set a value for the github check to use

I'm, thinking its this one?

  env:
    RANDOM_DATA: "false"

I think the file suffix must be *-values.yaml, and you can put multiple files in if you have different scenarios to test.

RANDOM_DATA: "true"
persistence:
enabled: false
initContainers:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hey I think this is needed for startup right?

Can you make this the default then?

The aim would be to be able to helm instal oci://...cohorter and it bring up the app with no values or prerequisites required at all.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

For a later improvement, potentially just chang the actual code, eg put this script into the entrypoint or just change the code itself to handle it. I should also be able to docker run .my-image with nothing else required for it to startup successfully

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

thing is I don't want to start supplying the SNOMED data, at least not at this stage. This is how the app behaves today i.e. you the user provide the SNOMED data even when you ask it to generate random patient data. Granted, I think it should operate in one of two modes - fully customised with all user-input data or all generated with it's own code mappings (this could be SNOMED or something made up).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm fine with this for now to merge in

Looking ahead - I'm looking for all the apps to all start up with sensible defaults, so nothing has any requirements (which is a big change from today in general...). This seems to be the pattern that I'd want to follow, for example I can run nginx but it is literally useless, it shows its default index html and 200 OK, it doesnt say "give me a conf file"

So here, even you want to avoid the data generation, I'd still see if we can have the container status "running", even if you then open the page and it says "200 OK, but insert data"

fi
echo "chart_version=$CHART_VERSION" >> "$GITHUB_OUTPUT"

- name: Add Helm repositories
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think you can remove this now it is in ct.yaml

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

removed

Copy link
Copy Markdown
Collaborator

@alhendrickson alhendrickson left a comment

Choose a reason for hiding this comment

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

Approved ! Cheers for the changes. I think anything else mentioned could be done later/never

ollama:
models:
pull:
- gpt-oss:20b # pulled automatically on first startup
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can we default to a smaller one? So it can demo on small hardware / CI easily - for example the Azure tutorial is bringing up just 4GB nodes - would prefer it running with bad results to then justify additional recommended hardware

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yeah - happy to do this as part of a separate task. Would need to assess what can provide comparable results with less etc. At a glance, I'm thinking medgemma (I've heard David mention this) or qwen 3. thoughts?

@jocelyneholdbrook jocelyneholdbrook merged commit 1e23213 into main Apr 20, 2026
3 checks passed
@jocelyneholdbrook jocelyneholdbrook deleted the feat/cohorter/add-helm-chart branch April 20, 2026 15:56
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