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

feat(SDK): Add SemaphoreKey and MutexName fields to DSL #11340

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

feat(SDK): Add SemaphoreKey and MutexName fields to DSL #11340

wants to merge 2 commits into from

Conversation

DharmitD
Copy link
Contributor

@DharmitD DharmitD commented Oct 30, 2024 •
edited
Loading

Description of your changes:
This PR introduces semaphoreKey and mutexName fields in PipelineConfig to support pipeline-level concurrency controls in KFP SDK.

This PR should be merged only after #11384 gets merged.

Testing instructions

Create a Python virtualenv and install the SDK and IR YAML API packages locally:

$ python -m venv .venv
$ source .venv/bin/activate
$ pip install wheel setuptools protobuf grpcio grpcio-tools
$ pip install -r sdk/python/requirements-dev.txt
$ pip install -e api/v2alpha1/python
$ pip install -e sdk/python

Use the example code to compile

$ kfp dsl compile --py main.py --output main.yaml

You should be able to compile and find the following snippet in the main.yaml file:

---
platforms:
  kubernetes:
    pipelineConfig:
      mutexName: mutex
      semaphoreKey: semaphore

Checklist:

@gregsheremeta
Copy link
Contributor

couple nitpicks but overall lgtm.

I don't want to merge it until the backend PR is posted and close to shipping.

@DharmitD DharmitD force-pushed the semaphore-mutex branch 3 times, most recently from c3178ce to 15b89b0 Compare October 30, 2024 22:45
gregsheremeta
gregsheremeta reviewed Oct 31, 2024
View reviewed changes
Copy link
Contributor

@gregsheremeta gregsheremeta left a comment •
edited
Loading

Choose a reason for hiding this comment

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

/hold

lgtm but let's wait until the backend PR is ready to go too
edit: need to use the right protoc version when generating the pipeline_spec files

@DharmitD DharmitD force-pushed the semaphore-mutex branch 2 times, most recently from 39665e1 to 8ef1f2d Compare October 31, 2024 18:45
@github-actions github-actions bot added ci-passed All CI tests on a pull request have passed and removed ci-passed All CI tests on a pull request have passed labels Oct 31, 2024
@HumairAK
Copy link
Collaborator

HumairAK commented Nov 4, 2024

can we add a test where we verify the semaphore setting compiles to the expected IR?

@DharmitD DharmitD mentioned this pull request Nov 13, 2024
Open
2 tasks
@google-oss-prow google-oss-prow bot added size/M and removed size/XXL labels Nov 13, 2024
@DharmitD
Copy link
Contributor Author

Edit: Updated to have the DSL generated SemaphoreKey instead of SemaphoreName field, since having a fixed, hardcoded semaphore name and letting users define the semaphore key is the format prescribed in Argo Workflows' synchronization docs here.

@github-actions github-actions bot added the ci-passed All CI tests on a pull request have passed label Nov 14, 2024
@gregsheremeta
Copy link
Contributor

can we add a test where we verify the semaphore setting compiles to the expected IR?

still need to do this. Here's an example:
https://github.com/kubeflow/pipelines/pull/10913/files#diff-cac76397fcf6a375f3865a864a3f6725b023c69b5216fa1ed71a86b334641399

@DharmitD
Copy link
Contributor Author

can we add a test where we verify the semaphore setting compiles to the expected IR?

still need to do this. Here's an example: https://github.com/kubeflow/pipelines/pull/10913/files#diff-cac76397fcf6a375f3865a864a3f6725b023c69b5216fa1ed71a86b334641399

Yes I'm working on this ATM. I'm adding a unit test to the compiler_test.py similar to the platform tests here:

pipelines/sdk/python/kfp/compiler/compiler_test.py

Line 3489 in 8a7b0e5

def test_one_task_one_platform(self):

Have a draft ready, testing locally and pushing it in a bit.

@github-actions github-actions bot added ci-passed All CI tests on a pull request have passed and removed ci-passed All CI tests on a pull request have passed labels Nov 14, 2024
@google-oss-prow google-oss-prow bot removed the size/M label Nov 14, 2024
@DharmitD DharmitD force-pushed the semaphore-mutex branch 4 times, most recently from 57ac1b4 to 487d956 Compare February 14, 2025 00:18
@google-oss-prow google-oss-prow bot added size/L and removed size/M labels Feb 14, 2025
@DharmitD DharmitD force-pushed the semaphore-mutex branch 4 times, most recently from a4eec53 to 171700d Compare February 14, 2025 19:31
@google-oss-prow Google OSS Prow
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign connor-mccarthy for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@DharmitD DharmitD force-pushed the semaphore-mutex branch 6 times, most recently from 1cc5a1f to 1b496cd Compare February 14, 2025 20:47
@DharmitD
Copy link
Contributor Author

/rerun-all

@DharmitD DharmitD force-pushed the semaphore-mutex branch 3 times, most recently from c506967 to 96717b8 Compare February 14, 2025 21:54
@rimolive
Copy link
Member

/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Feb 15, 2025
rimolive
rimolive reviewed Feb 15, 2025
View reviewed changes
Copy link
Member

@rimolive rimolive left a comment

Choose a reason for hiding this comment

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

Good job, Dharmit! Just asked for a nitpick change and you'll be good.

@google-oss-prow Google OSS Prow
Copy link

New changes are detected. LGTM label has been removed.

@google-oss-prow google-oss-prow bot removed the lgtm label Feb 16, 2025
@DharmitD
Copy link
Contributor Author

@chensun @connor-mccarthy could you PTAL? Thank you!

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

@rimolive rimolive rimolive left review comments

@gregsheremeta gregsheremeta gregsheremeta requested changes

@connor-mccarthy connor-mccarthy Awaiting requested review from connor-mccarthy

Assignees

@rimolive rimolive

Labels
ci-passed All CI tests on a pull request have passed do-not-merge/hold size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@DharmitD @gregsheremeta @HumairAK @rimolive