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

add flux operator python sdk example #962

Conversation

vsoch
Copy link
Contributor

@vsoch vsoch commented Jul 7, 2023

This will add a Python example for using Kueue with the Flux Operator! I have most of the MPI Operator done too, but ran into a bug with the ssh secret volume.

What type of PR is this?

/kind documentation

What this PR does / why we need it:

Users likely want to integrate Kueue into Python-based workflow tools, and examples are helpful for that!

Which issue(s) this PR fixes:

Issue has been closed, previous issue is:
Fixes #947

Special notes for your reviewer:

This worked so well! I'm excited to try this out in an actual workflow tool

Does this PR introduce a user-facing change?

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/documentation Categorizes issue or PR as related to documentation. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 7, 2023
@netlify
Copy link

netlify bot commented Jul 7, 2023

Deploy Preview for kubernetes-sigs-kueue ready!

Name Link
🔨 Latest commit 73fcbba
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/64b684748490270008e27b9b
😎 Deploy Preview https://deploy-preview-962--kubernetes-sigs-kueue.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 7, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @vsoch. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 7, 2023
@kannon92
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 10, 2023
Copy link
Contributor

@kannon92 kannon92 left a comment

Choose a reason for hiding this comment

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

This looks good! But one thing I wonder is of should we have the same script duplicated in two places? Can you either have the task readme just run the python file or use the inline definition in the readme

@vsoch
Copy link
Contributor Author

vsoch commented Jul 10, 2023

This looks good! But one thing I wonder is of should we have the same script duplicated in two places? Can you either have the task readme just run the python file or use the inline definition in the readme

Yeah agree it should be in one place - it was the same for the first example added (already duplicated). I think they are important in both places, so my plan is to get all three in (in this manner) and then update the site to have some javascript that will retrieve the file dynamically. It's how the examples / files are rendered here: https://converged-computing.github.io/flux-go/#pancakes-examples so hopefully I can figure out the same with hugo (I assume I can put html in markdown?)

@alculquicondor
Copy link
Contributor

/assign @moficodes

@vsoch vsoch force-pushed the docs/add-python-flux-operator-example branch from 8265dd1 to aea166b Compare July 10, 2023 18:41
@alculquicondor
Copy link
Contributor

let's merge #969 first

@vsoch
Copy link
Contributor Author

vsoch commented Jul 11, 2023

let's merge #969 first

Nice!! Thank you @moficodes for adding that, I totally love it.

@moficodes
Copy link
Contributor

@vsoch The include shortcode PR got merged. Could you rebase your PR and use include shortcode for the samples.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 12, 2023
@vsoch
Copy link
Contributor Author

vsoch commented Jul 12, 2023

I'll update this one after #967 is merged to make rebasing simpler (only done once!)

@vsoch vsoch force-pushed the docs/add-python-flux-operator-example branch from aea166b to e655fad Compare July 13, 2023 17:56
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 13, 2023
@vsoch
Copy link
Contributor Author

vsoch commented Jul 13, 2023

okay we are rebased!

@vsoch vsoch force-pushed the docs/add-python-flux-operator-example branch from b9171d4 to 73fcbba Compare July 18, 2023 12:24
@trasc
Copy link
Contributor

trasc commented Jul 18, 2023

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 18, 2023
Copy link
Contributor

@alculquicondor alculquicondor left a comment

Choose a reason for hiding this comment

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

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, vsoch

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 18, 2023
@k8s-ci-robot k8s-ci-robot merged commit 10ae081 into kubernetes-sigs:main Jul 18, 2023
@k8s-ci-robot k8s-ci-robot added this to the v0.4 milestone Jul 18, 2023

def main():
"""
Run an MPI job. This requires the MPI Operator to be installed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this accurate? Shouldn't it say flux operator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep that's a typo. I'll do a quick PR to fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wooo I got PR number 1000!! #1000 Do I get a special prize? 🍪

Copy link
Contributor

Choose a reason for hiding this comment

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

hahaha nice! Uhm.... Here's a 🥇

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can I eat that? 😆

Just kidding :) Thanks for the quick review! And overall for everything you do for these projects. <3

@vsoch vsoch deleted the docs/add-python-flux-operator-example branch July 18, 2023 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/documentation Categorizes issue or PR as related to documentation. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Python Examples
6 participants