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.

Sign up for GitHub

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

Jump to bottom

Add dagster deployment option using helm chart on local stack #144

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

Add dagster deployment option using helm chart on local stack #144

wants to merge 3 commits into from

Conversation

ara-25
Copy link
Contributor

@ara-25 ara-25 commented Oct 30, 2024

This PR proposes the following changes

  • Add dagster orchestrator tool option for local deployment
  • Add *.pyc in .gitignore

Closes #93

PR Checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Tests

aliabbasjaffri
aliabbasjaffri reviewed Nov 3, 2024
View reviewed changes
Copy link
Collaborator

@aliabbasjaffri aliabbasjaffri left a comment

Choose a reason for hiding this comment

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

PR looks great. However, did you managed to test and see if the dagster webserver can be reached on the endpoint?

src/mlinfra/modules/applications/local/orchestrator/dagster/tf_module/main.tf
@@ -0,0 +1,22 @@
locals {
dagster_helmchart_set = [{
Copy link
Collaborator

Choose a reason for hiding this comment

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

You'd also need to pass in ingress.enabled as true for the ingress to work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For context, I have used these options to allow a custom hostname to be set for the Webserver (as allowed also in the prefect and milvus deployments).

On second thoughts using ingress config to define a custom URL endpoint is going to create an ingress automatically, which the user might not want to do. Would that be fine?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is intentionally set on all the helm charts deployed on the local deployment environment to allow users to easily access their applications by just using. <application>.localhost url in the browser. I have been able to use this for all applications that exist for the local stack and would want dagster to follow suite.

@aliabbasjaffri aliabbasjaffri added the feature This label describes features which can significantly update the experience of using mlinfra label Nov 3, 2024
@ara-25
Copy link
Contributor Author

ara-25 commented Nov 4, 2024 •
edited
Loading

PR looks great. However, did you managed to test and see if the dagster webserver can be reached on the endpoint?

I tested it using the minikube provider and was able to successfully view the webserver UI.

Although, I had to portforward the webserver port from that pod using kubectl. If there is any other way I should be using to test it, then please let me know (I am new to k8s).

@aliabbasjaffri
Copy link
Collaborator

aliabbasjaffri commented Nov 4, 2024 •
edited
Loading

Although, I had to portforward the webserver port from that pod using kubectl. If there is any other way I should be using to test it, then please let me know (I am new to k8s).

I don't get it; were you able to access the url ( dagster.localhost ) in the browser by opening the minikube tunnel via minikube tunnel --profile=<minikube-cluster-name> ? I tested it with kind cluster, and i wasn't able to access it. Seems like there were some configs missing. Let me check it with minikube, but i'm quite sure if it didn't work for kind, it wouldn't work for minikube either.

EDIT: Yep, i tested it with minikube tunnel and i wasn't able to access the dagster dashboard via dagster.localhost dns endpoint.

@ara-25
Copy link
Contributor Author

ara-25 commented Nov 5, 2024

Understood; I did not test it like that. Will try to get it running with that.

The way I tested it was by doing kubectl port-forward <dagster-webser-pod-name> 9801:80 -n dagster and then view it using the localhost:9801 address on browser

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aliabbasjaffri aliabbasjaffri aliabbasjaffri left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
feature This label describes features which can significantly update the experience of using mlinfra
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature]: Deploy dagster helm chart on local kind stack
2 participants
@ara-25 @aliabbasjaffri