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

Go client for the k8s operator #529

Closed
bstadlbauer opened this issue Jun 27, 2022 · 9 comments · Fixed by #731
Closed

Go client for the k8s operator #529

bstadlbauer opened this issue Jun 27, 2022 · 9 comments · Fixed by #731

Comments

@bstadlbauer
Copy link
Collaborator

As part of the dask backend plugin work for flyte that I am doing, I have also started creating a golang client for the operator in dask-kubernetes. There is an initial rough draft in this repo, which only implements the API so far.
I was wondering, once it is more mature, if you guys would like to have this as part of a new or existing dask repo?

cc @jacobtomlinson

@jacobtomlinson
Copy link
Member

Many thanks for this, this is great to see. I think this is far enough out of scope for dask-kubernetes that it might not be good to merge it here. Given that it is a community maintained tools maybe it is a good candidate to migrate to dask-contrib?

@bstadlbauer
Copy link
Collaborator Author

Sounds good 👍
I'll post an update here once the project has matured a bit :-)

@bstadlbauer
Copy link
Collaborator Author

@jacobtomlinson Following up here, with the Flyte plugin merged we're currently switching our company internal tooling to use it. If I see that being stable, I'd be open to move it to dask-contrib (happy to keep maintaining it).

Do you think it should follow the same release cycle as dask-kubernetes or should I just use semver and list compatibilities?
Also, would you mind notifying me in case the CRD changes? Then I'll make sure to pass these on to the go code

@jacobtomlinson
Copy link
Member

If there is value in being released in lockstep with the rest of dask-kubernetes (which is very ad-hoc) and it needs to be kept up to date with the CRDs then maybe we should just merge it in here.

We could add you to a CODEOWNERS file for the CRDs so you are notified of any PRs relating to them. I'd also happily give you expanded permissions on the whole repo if you wanted to engage more, it's been great having you here.

The only hurdles I see are getting CI configured so that it only runs when appropriate and getting the pre-commit stuff set up correctly. But that seems like a small lift. It would also need to go somewhere in the tree like dask_kubernetes/operator/go-client

What do you think?

@bstadlbauer
Copy link
Collaborator Author

If there is value in being released in lockstep with the rest of dask-kubernetes (which is very ad-hoc) and it needs to be kept up to date with the CRDs then maybe we should just merge it in here.

Happy to merge things into here 👍 I might not get to it immediately, but should have some time in the upcoming weeks.

We could add you to a CODEOWNERS file for the CRDs so you are notified of any PRs relating to them. I'd also happily give you expanded permissions on the whole repo if you wanted to engage more, it's been great having you here.

Thank you for the kind words! Both options work for me, so in case you need another hand, happy to get more involved 👍

The only hurdles I see are getting CI configured so that it only runs when appropriate and getting the pre-commit stuff set up correctly. But that seems like a small lift.

We should be able to manage that 👍 Especially with regards to the pre-commit stuff, that might just go in to a separate script (as the go tools don't fit as nicely in to pre-commit)

@jacobtomlinson
Copy link
Member

Ok sounds good. Let me know when there is something to merge.

I've sent you an invite for write permissions on this repo :). You may find the dask maintainer guidelines and dask-kubernetes release docs useful. If you have time to review PRs and generally help out with maintenance it would be much appreciated. Feel free to merge things that you think look good. We try to get two pairs of eyes on each PR (author and reviewer), but sometimes due to capacity that's not always possible so self-merging is ok with a 24-hour warning.

@jsuwala
Copy link

jsuwala commented May 9, 2023

Hi @bstadlbauer - just chiming in here as we also need a go client for the dask-kubernetes operator for our project. Anything we can do to help move your repo to be mainstreamed here?

@bstadlbauer
Copy link
Collaborator Author

Hi @jsuwala!

I've mostly not prioritized this as I've been thinking that we were the only user. I should be able to get to this in the next few days in case that's not too late?

@bstadlbauer
Copy link
Collaborator Author

@jsuwala Sorry for this taking so long, but I fought against go auto-generation tooling more than I would have liked. My changes are up in #731

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants