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

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

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

Conversation

DharmitD
Copy link
Contributor

@DharmitD DharmitD commented Oct 30, 2024

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
Copy link
Contributor

@gregsheremeta gregsheremeta left a comment

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
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:

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 c4b0c43 to 78adc95 Compare November 14, 2024 22:56
@DharmitD
Copy link
Contributor Author

DharmitD commented Nov 15, 2024

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:

def test_one_task_one_platform(self):

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

Added a unit test to verify semaphore and mutex setting, the test works as expected and passes on my local venv, but fails in this PR's CI.

Wondering if it's because the .proto changes haven't been merged into the code base yet. @gregsheremeta could you take a look and confirm?
And if it is so, then should the test be added in a follow up PR?

@gregsheremeta
Copy link
Contributor

ml_pipelines.PipelineConfig" has no field named "semaphoreKey" at "SinglePlatformSpec.pipelineConfig".

So yes. The test can stay here. Tests just won't pass until the proto is merged and this is rebased. It's the normal/usual/expected flow.

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

@google-oss-prow google-oss-prow bot added size/M and removed size/XXL labels Nov 16, 2024
@DharmitD DharmitD changed the title feat(sdk+api) Implement Semaphores and Mutexes Pipeline Configuration feat(sdk) Add SemaphoreKey and MutexName fields to DSL Nov 16, 2024
@DharmitD DharmitD changed the title feat(sdk) Add SemaphoreKey and MutexName fields to DSL feat(SDK) Add SemaphoreKey and MutexName fields to DSL Nov 16, 2024
@DharmitD DharmitD changed the title feat(SDK) Add SemaphoreKey and MutexName fields to DSL feat(SDK): Add SemaphoreKey and MutexName fields to DSL Nov 16, 2024
@DharmitD
Copy link
Contributor Author

/hold until #11384 gets merged.

@DharmitD
Copy link
Contributor Author

ml_pipelines.PipelineConfig" has no field named "semaphoreKey" at "SinglePlatformSpec.pipelineConfig".

So yes. The test can stay here. Tests just won't pass until the proto is merged and this is rebased. It's the normal/usual/expected flow.

Edit: Removed proto/API changes and created a separate PR for those here: #11384
So as @gregsheremeta mentioned, once proto changes are merged and this PR is rebased, the test will work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-passed All CI tests on a pull request have passed do-not-merge/hold size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants