Conversation
8f1fdde to
4108e07
Compare
4108e07 to
34443de
Compare
|
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
|
@tvalentyn can you please take a look? |
|
Assigning reviewers. If you would like to opt out of this review, comment R: @jrmccluskey for label python. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
damccorm
left a comment
There was a problem hiding this comment.
Thanks - this looks reasonable to me, but I will defer to @tvalentyn
| @@ -0,0 +1,29 @@ | |||
| Copyright (c) 2012-now, CloudPickle developers and contributors. | |||
There was a problem hiding this comment.
I'm not sure how we've done this in the past, but I think it probably makes sense to colocate the license with the source. That will also ensure it actually gets packed into the Python container.
There was a problem hiding this comment.
Actually, I think we need to just add it to https://github.com/apache/beam/blob/master/LICENSE directly
There was a problem hiding this comment.
I checked what is included in containers images by searching find . -name *LICENSE*:
Seeing:
-
./opt/apache/beam/third_party_licenses/cloudpickle/LICENSE-- this would probably disappear if cloudpickle is not a dependency, unless we modify sdks/python/container/license_scripts/manual_licenses to copy it, which may not be necessary if we add the license in one of the two below places: -
./opt/apache/beam/LICENSE -
./opt/apache/beam/LICENSE.python-- we could addLICENSE.cloudpickleas proposed in this PR
I don't have preference which file to add it to, we can see what is simpler and easier to import internally.
There was a problem hiding this comment.
Licenses are not imported from this repo internally, they are manually created, so from an import point of view I don't think it matters where we put it.
Does LICENSE.cloudpickle work for building containers? Or should I put it in LICENSE.python instead?
There was a problem hiding this comment.
Does LICENSE.cloudpickle work for building containers?
yes, it works, we can check that it is included in the container build. for example, by running smth like
gradlew :sdks:python:container:py310:docker
docker run --rm -it --entrypoint=/bin/bash apache/beam_python3.10_sdk:2.65.0.dev
and inspecting the image content.
There was a problem hiding this comment.
feel free to follow up in another PR if this needs another change.
* Add vendored cloudpickle. * Remove references to third party cloudpickle. * Fix precommits. * Fix isort lint error. * Fix extra space. * Remove extra quote * Change vendored version to 3.1.1 --------- Co-authored-by: Claude <cvandermerwe@google.com>
Vendors cloudpickle 3.1.1 and adds it to apache_beam codebase.
Removes the no-longer-necessary cloudpickle dependency.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.