fix(glue): PythonRayExecutableProps has innaccurate properties#28625
fix(glue): PythonRayExecutableProps has innaccurate properties#28625mergify[bot] merged 4 commits intoaws:mainfrom
PythonRayExecutableProps has innaccurate properties#28625Conversation
|
@lpizzinidev Thank you for looking! Could you please approve and merge this if it is okay? |
|
@moomindani Merging requires a core team member, I'm just a community reviewer 🙂 |
|
This PR has been in the MERGE CONFLICTS state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week. |
PythonRayExecutableProps has innaccurate properties
kaizencc
left a comment
There was a problem hiding this comment.
hi @moomindani, I have a few concerns before we merge this PR
| * | ||
| * @default - no extra python files specified. | ||
| * | ||
| * @see `--s3-py-modules` in https://docs.aws.amazon.com/glue/latest/dg/author-job-ray-job-parameters.html |
There was a problem hiding this comment.
@see expects a url after the tag, and nothing else
There was a problem hiding this comment.
It's zipped utils.py which is used for this integration test. Since s3PythonModules accepts only Zip file, we need to have this.
I have verified that other packages in AWS CDK has similar zip files so I do not think this can be blocker.
| * Props for creating a Python Ray job executable | ||
| */ | ||
| export interface PythonRayExecutableProps extends SharedJobExecutableProps, PythonExecutableProps {} | ||
| export interface PythonRayExecutableProps extends SharedJobExecutableProps, RayExecutableProps {} |
There was a problem hiding this comment.
what happens to PythonExecutableProps? a) is it used elsewhere and b) we are not dropping support for any properties right?
There was a problem hiding this comment.
if we are dropping support for properties (as the attached issue suggests) it should have been called out in PR description as a BREAKING CHANGE.
There was a problem hiding this comment.
a) is it used elsewhere
Yes. There are three job types; Spark, Python shell, and Ray. PythonExecutableProps is used for the first two.
b) we are not dropping support for any properties right?
Right. We are not dropping any or working properties.
Only the property we are dropping is extraPythonFiles which was not working for Ray job. This is not breaking change.
Pull request has been modified.
|
@kaizencc Can you take a look and merge it if there are no further update needed? |
paulhcsun
left a comment
There was a problem hiding this comment.
The changes look good to me, nice work @moomindani!
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
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). |
Closes #28570.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license