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

Vendors dill #23870

Closed
wants to merge 55 commits into from
Closed

Conversation

ryanthompson591
Copy link
Contributor

@ryanthompson591 ryanthompson591 commented Oct 27, 2022

Pulls a copy of the dill library into the beam sdk. This version is the same as the current version (0.3.1.1.).

The main reason to do this is to keep the version of dill on the worker and runner the same.

More detailed information in design doc.

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Choose reviewer(s) and mention them in a comment (R: @username).
  • Mention the appropriate issue in your description (for example: 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, comment fixes #<ISSUE NUMBER> instead.
  • Update CHANGES.md with noteworthy changes.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

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)

Build python source distribution and wheels
Python tests
Java tests
Go tests

See CI.md for more information about GitHub Actions CI.

@codecov
Copy link

codecov bot commented Oct 27, 2022

Codecov Report

Merging #23870 (4046a62) into master (90fffc0) will decrease coverage by 1.26%.
The diff coverage is 27.08%.

@@            Coverage Diff             @@
##           master   #23870      +/-   ##
==========================================
- Coverage   73.46%   72.21%   -1.26%     
==========================================
  Files         714      726      +12     
  Lines       96497    99155    +2658     
==========================================
+ Hits        70889    71600     +711     
- Misses      24286    26233    +1947     
  Partials     1322     1322              
Flag Coverage Δ
python 81.01% <27.08%> (-2.16%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
sdks/python/apache_beam/coders/coders.py 87.48% <0.00%> (ø)
sdks/python/apache_beam/vendor/dill/__diff.py 0.00% <0.00%> (ø)
sdks/python/apache_beam/vendor/dill/_objects.py 0.00% <0.00%> (ø)
sdks/python/apache_beam/vendor/dill/source.py 7.21% <7.21%> (ø)
sdks/python/apache_beam/vendor/dill/detect.py 10.50% <10.50%> (ø)
sdks/python/apache_beam/vendor/dill/temp.py 15.84% <15.84%> (ø)
sdks/python/apache_beam/vendor/dill/pointers.py 16.66% <16.66%> (ø)
sdks/python/apache_beam/vendor/dill/__init__.py 48.14% <48.14%> (ø)
sdks/python/apache_beam/vendor/dill/_dill.py 53.70% <53.70%> (ø)
sdks/python/apache_beam/vendor/dill/settings.py 71.42% <71.42%> (ø)
... and 9 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@github-actions
Copy link
Contributor

Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment assign set of reviewers

aromanenko-dev and others added 23 commits October 28, 2022 09:29
Bumps [github.com/spf13/cobra](https://github.com/spf13/cobra) from 1.6.0 to 1.6.1.
- [Release notes](https://github.com/spf13/cobra/releases)
- [Commits](spf13/cobra@v1.6.0...v1.6.1)

---
updated-dependencies:
- dependency-name: github.com/spf13/cobra
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…e#23823)

Bumps [cloud.google.com/go/pubsub](https://github.com/googleapis/google-cloud-go) from 1.25.1 to 1.26.0.
- [Release notes](https://github.com/googleapis/google-cloud-go/releases)
- [Changelog](https://github.com/googleapis/google-cloud-go/blob/main/CHANGES.md)
- [Commits](googleapis/google-cloud-go@pubsub/v1.25.1...pubsub/v1.26.0)

---
updated-dependencies:
- dependency-name: cloud.google.com/go/pubsub
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* Include pkg.go.dev in release vote email

* Use RELEASE_VERSION consistently.
* Add Go usage instructions to download page.

* whitepsace
* Use Akvelon editor (apache#23415)

* Upgrade Akvelon editor (apache#23415)
* tmp

* fix mock

* datastore_p1

* mock_repo

* auth_store

* nit

* workflow

* vars

* nit

* README
…hConverter` (apache#23455)

* Add arrow_type_compatibility

A library for relating Beam schemas to/from Arrow schemas, and Beam Rows
to/from Arrow RecordBatches (as a BatchConverter).

* Add microbenchmark for PyarrowBatchConverter.produce_batch

* Test schema options at more levels

* fixup! Add arrow_type_compatibility

* fixup! Add microbenchmark for PyarrowBatchConverter.produce_batch

* Address review comments
…le (apache#23548)

* Migrate BINARY, VARBINARY, CHAR, VARCHAR jdbc logical types to portable

* Move jdbc logical type to portable logical types in Java

* Create portable logical types in Python

* Support value_from_runner_api and value_to_runner_api in Python
  SchemaTransform (currently only support atomic type values)

Fix nullable/test/leftovers

* Fix typos

* Add standard coder test

* Fix RowCoderImpl cannot encode bytes column in cython compiled

* Set coder_impl.is_compiled=True when running on compiled stream
  module

* Add docstring, add todo and warnings for unsupported
…e Streams connector to test transaction tags filtering in the Change Stream records (apache#23284)

* Added Change Stream filter by transaction tag IT test

* Improved filter check using both contains and not contains clauses

* Ran ./gradlew spotlessApply

Co-authored-by: Andrew Galad <agalad@google.com>
* Use --release 8 for builds targeting Java 8

This ensures cross compilation works correctly when building on JDK 11
and targeting Java 8.

* Update buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy

Co-authored-by: Lukasz Cwik <lcwik@google.com>

* Review feedback

* Update buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy

Co-authored-by: Lukasz Cwik <lcwik@google.com>

* Adjust javaVersion configuration

Co-authored-by: Lukasz Cwik <lcwik@google.com>
@github-actions github-actions bot added the build label Oct 28, 2022
@github-actions github-actions bot removed the build label Nov 11, 2022
@ryanthompson591
Copy link
Contributor Author

Run Python 3.7 PostCommit

@ryanthompson591
Copy link
Contributor Author

Run Python 3.8 PostCommit

@ryanthompson591
Copy link
Contributor Author

R: @tvalentyn

Can you give this a first pass. There is still more testing to do, but here is the first version.

@github-actions
Copy link
Contributor

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control

Copy link
Contributor

@tvalentyn tvalentyn left a comment

Choose a reason for hiding this comment

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

Mostly looks good. small nits. a few questions;

  1. Can we add a readme for what it takes to upgrade to newer version?
  2. Are postcommit failures related?
  3. Did you run TGP w/ this change? (Can discuss offline).

sdks/python/apache_beam/vendor/__init__.py Outdated Show resolved Hide resolved
sdks/python/scripts/run_pylint.sh Outdated Show resolved Hide resolved
sdks/python/setup.py Outdated Show resolved Hide resolved
build.gradle.kts Outdated Show resolved Hide resolved
@ryanthompson591
Copy link
Contributor Author

Run Python 3.8 PostCommit

@eddie-scio
Copy link

Hello friends, just checking in if there are any blockers here!

@tvalentyn
Copy link
Contributor

tvalentyn commented Dec 9, 2022

Ryan is currently OOO for a bit and therefore not actively working on it.

@tvalentyn
Copy link
Contributor

tvalentyn commented Dec 9, 2022

I'll see if I can pick it up in between of other efforts to push forward, but no promises at this time. Sorry.

@eddie-scio
Copy link

Ah, thanks for the update! That's already enormously helpful so we can adjust our blocked efforts accordingly. Appreciate the transparency.

@ryanthompson591
Copy link
Contributor Author

ryanthompson591 commented Dec 9, 2022 via email

@eddie-scio
Copy link

Hey folks, happy new year! Just wanted to check in on this effort and if we can push through.

@eddie-scio
Copy link

Hey @ryanthompson591 @tvalentyn don't want to be that guy, but just checking in on any ETA here, or if there's any way we can help unblock

@tvalentyn
Copy link
Contributor

Hi, thanks for the ping. Unfortunately this effort currently doesn't have an owner and I can't provide an ETA at this time. Can you remind me of the nature of the blocking issue that you have? Perhaps I could suggest a workaround.

@eddie-scio
Copy link

Really appreciate the transparency! Also x-linking the issue: #22893

Here's our dependency chain:

  1. We want to upgrade to Spacy V3 to leverage their new v3 models.
  2. We also use allennlp==2.9.3, which has spacy<3.3,>=2.1.0.
  3. There's a bug leading to large latency issues in Spacy V3. The fix was only released in spacy==3.3.0.
  4. allennlp==2.10.0 has spacy>=2.1.0,<3.4 which gets around (4), but it also has dill>=0.3.4, which leads us here, since Beam still has dill<0.3.2.

Since allennlp is now in maintenance mode, we're unlikely to see any changes there, so the only other avenue I see is to remove our allennlp dependency fully, which is quite high lift on our end. And since we saw this dill version cap from Beam was pretty old, we figured you guys were going to work on this anyways soon.

@tvalentyn
Copy link
Contributor

we plan to lift the bound before next release, i am taking a look.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 7, 2023

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@beam.apache.org list. Thank you for your contributions.

@github-actions github-actions bot added the stale label Jun 7, 2023
@github-actions
Copy link
Contributor

This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

@github-actions github-actions bot closed this Jun 15, 2023
@janheinrichmerker
Copy link

Any chance that the conflict between the dill versions of apache-beam and datasets (see #24458 and huggingface/datasets#5613) will be resolved?

@tvalentyn
Copy link
Contributor

Eventually yes but it is not straightforward. #22893 has the discussion and suggestions (you can force-install a newer version of dill in submission and runtime environment in the meantime).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.