feat(scheduler-targets-alpha): `SageMakerStartPipelineExecution` Target by go-to-k · Pull Request #28927 · aws/aws-cdk

go-to-k · 2024-01-30T16:23:12Z

This PR adds SageMakerStartPipelineExecution Target for EventBridge Scheduler.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

add an unit test

kaizencc · 2024-02-06T16:04:03Z

packages/@aws-cdk/aws-scheduler-targets-alpha/lib/sage-maker-start-pipeline-execution.ts

+  private readonly pipelineArn: string;
+
+  constructor(
+    private readonly pipeline: CfnPipeline,


Everything is fine except for this... it should be an IPipeline but that doesn't exist yet. The problem is that this locks in people to the L1 pipeline, and ideally we'd want this to be interoperable with a community L2 if that existed.

So how about adding a very barebones IPipeline construct to aws-sagemaker. Nothing controversial, just kind of a placeholder. In the future, if we develop a Pipeline L2, it can build off of the IPipeline as can any community L2. As long as we don't make any crazy decisions, this can be stable from day 1. We did something similar in IEndpoint.

Good idea.
The IEndpoint is also creating an interface in the aws-sagemaker-alpha module, but that is to ensure that the existing implementation is not affected, so is it correct to create it only in the aws-sagemaker module for this PR?

kaizencc

@go-to-k one comment about how we are using sagemaker And one other comment -- can you make sure that the references to sagemaker are SageMaker with the capital M?

go-to-k · 2024-02-07T09:29:56Z

@kaizencc

Thanks for your review.

can you make sure that the references to sagemaker are SageMaker with the capital M?

Oh, It seems that the CFn documents SageMaker with the capital M.
But I had been written Sagemaker without the capital M in some points, so I will fix them.

https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-sagemaker-pipeline.html

go-to-k · 2024-02-07T09:36:51Z

It seems that the CFn documents SageMaker with the capital M.

However, it seems that many places have already written Sagemaker in some codes. Let's not swing to them and use SageMaker only in this PR.

https://github.com/go-to-k/aws-cdk/blob/64275fd095cd52daaeba852512a12d6df9664b28/packages/@aws-cdk/aws-sagemaker-alpha/lib/endpoint.ts#L24

https://github.com/go-to-k/aws-cdk/blob/64275fd095cd52daaeba852512a12d6df9664b28/packages/%40aws-cdk/aws-sagemaker-alpha/README.md?plain=1#L26

etc...

PS) I changed.

249220a

…o-k/aws-cdk into scheduler-targets-sagemaker

Pull request has been modified.

go-to-k · 2024-02-07T12:36:22Z

packages/@aws-cdk/aws-scheduler-targets-alpha/test/integ.sage-maker-start-pipeline-execution.ts

+import { CfnPipeline } from 'aws-cdk-lib/aws-sagemaker';
+import { IPipeline } from 'aws-cdk-lib/aws-sagemaker';


I got the following eslint error when I did the following here, so here it is

import { CfnPipeline, IPipeline } from 'aws-cdk-lib/aws-sagemaker';

❯ yarn lint yarn run v1.22.17 $ cdk-lint Oops! Something went wrong! :( ESLint: 7.32.0 Error: ENOENT: no such file or directory, open '/Users/goto/github/aws-cdk/packages/aws-cdk-lib/aws-sagemaker/package.json' Occurred while linting /Users/goto/github/aws-cdk/packages/@aws-cdk/aws-scheduler-targets-alpha/test/integ.sage-maker-start-pipeline-execution.ts:6 at Object.openSync (node:fs:600:3) at Object.readFileSync (node:fs:468:35) at isAlphaPackage (/Users/goto/github/aws-cdk/tools/@aws-cdk/eslint-plugin/lib/rules/invalid-cfn-imports.js:139:31) at checkIfImportedLocationIsAnAlphaPackage (/Users/goto/github/aws-cdk/tools/@aws-cdk/eslint-plugin/lib/rules/invalid-cfn-imports.js:118:16) at ImportDeclaration (/Users/goto/github/aws-cdk/tools/@aws-cdk/eslint-plugin/lib/rules/invalid-cfn-imports.js:55:74) at /Users/goto/github/aws-cdk/node_modules/eslint/lib/linter/safe-emitter.js:45:58 at Array.forEach (<anonymous>) at Object.emit (/Users/goto/github/aws-cdk/node_modules/eslint/lib/linter/safe-emitter.js:45:38) at NodeEventGenerator.applySelector (/Users/goto/github/aws-cdk/node_modules/eslint/lib/linter/node-event-generator.js:293:26) at NodeEventGenerator.applySelectors (/Users/goto/github/aws-cdk/node_modules/eslint/lib/linter/node-event-generator.js:322:22) Error: /Users/goto/github/aws-cdk/node_modules/eslint/bin/eslint.js . --ext=.ts --resolve-plugins-relative-to=/Users/goto/github/aws-cdk/tools/@aws-cdk/cdk-build-tools/lib exited with error code 2 Linting failed. error Command failed with exit code 1. info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

But duplicated no-duplicate-imports error... I will investigate later.

@aws-cdk/aws-scheduler-targets-alpha: /codebuild/output/src294405110/src/github.com/aws/aws-cdk/packages/@aws-cdk/aws-scheduler-targets-alpha/test/integ.sage-maker-start-pipeline-execution.ts @aws-cdk/aws-scheduler-targets-alpha: 7:1 error 'aws-cdk-lib/aws-sagemaker' import is duplicated no-duplicate-imports @aws-cdk/aws-scheduler-targets-alpha: 笨� 1 problem (1 error, 0 warnings)

The error (Error: ENOENT: no such file or directory...) is about the lint rules.

The error was reduced by changing the settings to match other alpha modules like baseConfig.rules['@aws-cdk/invalid-cfn-imports'] = 'off';.

55fbb20

using IPipeline grantStartPipelineExecution tweak tweak tweak fix eslint error tweak tweak tweak

go-to-k · 2024-02-08T06:16:56Z

@kaizencc

The build was successful, reflecting your comments. Could you please check?

kaizencc

Thanks @go-to-k

mergify · 2024-02-13T21:15:11Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

mergify · 2024-02-13T21:18:31Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2024-02-13T22:04:28Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 1fc2be5
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2024-02-13T22:05:43Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

…et (#28927) This PR adds SageMakerStartPipelineExecution Target for EventBridge Scheduler. Closes #27457 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

feat(scheduler-targets-alpha): Target

c4f2ae3

github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 distinguished-contributor [Pilot] contributed 50+ PRs to the CDK labels Jan 30, 2024

aws-cdk-automation requested a review from a team January 30, 2024 16:23

aws-cdk-automation previously requested changes Jan 30, 2024

View reviewed changes

go-to-k added 2 commits January 31, 2024 01:26

new line

64275fd

integ test and snapshot

21513e3

go-to-k marked this pull request as ready for review February 5, 2024 11:35

add unit test

5a30af5

add an unit test

go-to-k force-pushed the scheduler-targets-sagemaker branch from cc5b6cd to 5a30af5 Compare February 5, 2024 11:43

go-to-k added 3 commits February 5, 2024 20:48

Merge branch 'main' into scheduler-targets-sagemaker

5529915

import order in integs

e41f5f5

fix rosetta

e2e3a37

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Feb 5, 2024

kaizencc reviewed Feb 6, 2024

View reviewed changes

kaizencc previously requested changes Feb 6, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Feb 6, 2024

go-to-k added 2 commits February 7, 2024 18:40

fix to SageMaker with capital M

249220a

Merge branch 'scheduler-targets-sagemaker' of https://github.com/go-t…

05b0668

…o-k/aws-cdk into scheduler-targets-sagemaker

revert IPipeline

bb687e6

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Feb 7, 2024

go-to-k requested a review from kaizencc February 7, 2024 10:55

go-to-k force-pushed the scheduler-targets-sagemaker branch 2 times, most recently from 42b6946 to d6d3c59 Compare February 7, 2024 12:03

go-to-k force-pushed the scheduler-targets-sagemaker branch 4 times, most recently from 0e29910 to 0b1df62 Compare February 7, 2024 12:35

go-to-k commented Feb 7, 2024

View reviewed changes

add IPipeline

7671cca

using IPipeline grantStartPipelineExecution tweak tweak tweak fix eslint error tweak tweak tweak

go-to-k force-pushed the scheduler-targets-sagemaker branch from 0b1df62 to 7671cca Compare February 7, 2024 12:43

go-to-k added 2 commits February 7, 2024 21:53

Merge branch 'main' into scheduler-targets-sagemaker

91d0a2f

change eslint settings and an integ test

55fbb20

Update integ.sage-maker-start-pipeline-execution.ts

0f0e67e

kaizencc approved these changes Feb 13, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Feb 13, 2024

Merge branch 'main' into scheduler-targets-sagemaker

1fc2be5

mergify bot merged commit db260b0 into aws:main Feb 13, 2024

lorenzwalthert mentioned this pull request Aug 8, 2024

(aws-scheduler-targets-alpha): Add SageMakerStartPipelineExecution Target #27457

Closed

2 tasks

		import { CfnPipeline } from 'aws-cdk-lib/aws-sagemaker';
		import { IPipeline } from 'aws-cdk-lib/aws-sagemaker';

Conversation

go-to-k commented Jan 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

kaizencc Feb 6, 2024

Choose a reason for hiding this comment

Uh oh!

go-to-k Feb 7, 2024

Choose a reason for hiding this comment

Uh oh!

go-to-k Feb 8, 2024

Choose a reason for hiding this comment

Uh oh!

kaizencc left a comment

Choose a reason for hiding this comment

Uh oh!

go-to-k commented Feb 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

go-to-k commented Feb 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

go-to-k Feb 7, 2024

Choose a reason for hiding this comment

Uh oh!

go-to-k Feb 7, 2024

Choose a reason for hiding this comment

Uh oh!

go-to-k Feb 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

go-to-k commented Feb 8, 2024

Uh oh!

kaizencc left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Feb 13, 2024

Uh oh!

mergify bot commented Feb 13, 2024

Uh oh!

aws-cdk-automation commented Feb 13, 2024

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Feb 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

go-to-k commented Jan 30, 2024 •

edited

Loading

go-to-k commented Feb 7, 2024 •

edited

Loading

go-to-k commented Feb 7, 2024 •

edited

Loading

go-to-k Feb 8, 2024 •

edited

Loading