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

fix: Protobuf lower bound to 3.20 to alert that Feast is incompatible with tensorflow #3476

Merged
merged 2 commits into from
Feb 17, 2023

Conversation

gbmarc1
Copy link
Contributor

@gbmarc1 gbmarc1 commented Feb 1, 2023

What this PR does / why we need it:
protobuf in python sdk should be lower bound to 3.20 because they introduced the builder in 3.20.

feast/protos/feast/types/Value_pb2.py:5
      1 # -*- coding: utf-8 -*-
      2 # Generated by the protocol buffer compiler.  DO NOT EDIT!
      3 # source: feast/types/Value.proto
      4 """Generated protocol buffer code."""
----> 5 from google.protobuf.internal import builder as _builder
      6 from google.protobuf import descriptor as _descriptor
      7 from google.protobuf import descriptor_pool as _descriptor_pool

ImportError: cannot import name 'builder' from 'google.protobuf.internal' /lib/python3.9/site-packages/google/protobuf/internal/__init__.py)

Which issue(s) this PR fixes:

Fixes #3287

@gbmarc1 gbmarc1 changed the title build: protobuf lower bound to 3.20 fix: protobuf lower bound to 3.20 Feb 1, 2023
@gbmarc1
Copy link
Contributor Author

gbmarc1 commented Feb 3, 2023

/assign @sfc-gh-madkins

@achals achals changed the title fix: protobuf lower bound to 3.20 fix: Protobuf lower bound to 3.20 Feb 3, 2023
Copy link
Member

@achals achals left a comment

Choose a reason for hiding this comment

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

I think you'll have to regenerate the requirements.txt files. There's a command in the makefile that you can use to do that!

@gbmarc1
Copy link
Contributor Author

gbmarc1 commented Feb 13, 2023

@achals Done!

@adchia
Copy link
Collaborator

adchia commented Feb 13, 2023

/lgtm

@feast-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adchia, gbmarc1

The full list of commands accepted by this bot can be found here.

The pull request process is described 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

@adchia
Copy link
Collaborator

adchia commented Feb 14, 2023

@cburroughs mentioned that Tensorflow requires a Protobuf version < 3.20

@cburroughs
Copy link
Contributor

Hey sorry I missed the github notification for this earlier!

At least for me the original impetuous for #3287 was that tensorflow requires protobuf < 3.20. If you download the latest version on PyPi today (tensorflow==2.11.0)

    # TODO(b/182876485): Protobuf 3.20 results in linker errors on Windows
    # Protobuf 4.0 is binary incompatible with what C++ TF uses.
    # We need ~1 quarter to update properly.
    # See also: https://github.com/tensorflow/tensorflow/issues/53234
    # See also: https://github.com/protocolbuffers/protobuf/issues/9954
    # See also: https://github.com/tensorflow/tensorflow/issues/56077
    # This is a temporary patch for now, to patch previous TF releases.
    'protobuf >= 3.9.2, < 3.20',

Tensorflow is a pretty popular machine learning library and I'd like to be able to use it with Feast.

I'm not a protobuf expert but my understanding from protocolbuffers/protobuf#9778 is that if you use version X to generate the code, then it is only guaranteed to work with version >=X. So if Feast as a requirement on >=3.20 the minimum version isn't' "3.20" but "whatever version >=3.20 that happened to be resolved when the release process runs". That means a similar problem could occur in the future any time Feast cuts a release. I think a separate step (in the makefile?) will need to to pin to an explicit version.

@adchia adchia removed the approved label Feb 14, 2023
@adchia adchia changed the title fix: Protobuf lower bound to 3.20 fix: Protobuf lower bound to 3.20 to alert that Feast is incompatible with tensorflow Feb 15, 2023
@gbmarc1
Copy link
Contributor Author

gbmarc1 commented Feb 15, 2023

@cburroughs,
Actually, current Feast version is not compatible with protobuf<3.20. Installing a virtual environment with feast and tensorflow==2.11 would not raise an incompatibility error. But when importing Feast, an error would raise because there would be a missing method in protobuf.

      1 # -*- coding: utf-8 -*-
      2 # Generated by the protocol buffer compiler.  DO NOT EDIT!
      3 # source: feast/types/Value.proto
      4 """Generated protocol buffer code."""
----> 5 from google.protobuf.internal import builder as _builder
      6 from google.protobuf import descriptor as _descriptor
      7 from google.protobuf import descriptor_pool as _descriptor_pool

ImportError: cannot import name 'builder' from 'google.protobuf.internal' (/opt/miniconda/envs/env/lib/python3.9/site-packages/google/protobuf/internal/__init__.py)

Note that they updated the protobuf version in tensorflow==2.12. It is already merged on master and available on this release candiate.

@cburroughs
Copy link
Contributor

Actually, current Feast version is not compatible with protobuf<3.20.

Correct! That is the regression between 0.25.0 and 0.25.1 reported in #3287

@adchia adchia force-pushed the lower-bound-protobuf branch 2 times, most recently from d689639 to 6765eee Compare February 17, 2023 07:11
Signed-off-by: gbmarc1 <marcantoine.belanger@shopify.com>
@adchia
Copy link
Collaborator

adchia commented Feb 17, 2023

/lgtm

Signed-off-by: Danny Chiao <danny@tecton.ai>
@feast-ci-bot feast-ci-bot removed the lgtm label Feb 17, 2023
@adchia
Copy link
Collaborator

adchia commented Feb 17, 2023

/lgtm

@adchia adchia merged commit 9ca59e3 into feast-dev:master Feb 17, 2023
kevjumba pushed a commit that referenced this pull request Mar 17, 2023
# [0.30.0](v0.29.0...v0.30.0) (2023-03-17)

### Bug Fixes

* Add description attribute to the Field.from_proto method ([#3469](#3469)) ([473f8d9](473f8d9))
* Add filesystem kwargs when read prev_table on FileRetrievalJob (… ([#3491](#3491)) ([dca4745](dca4745)), closes [#3490](#3490)
* Feature view `entities` from_proto type ([#3524](#3524)) ([57bbb61](57bbb61))
* Fix missing requests requirement after GCP requirement removed. Make BigQuerySource not require gcp extra ([2c85421](2c85421))
* Fix SQL Registry cache miss ([#3482](#3482)) ([3249b97](3249b97))
* Fixed path inside quickstart notebook ([#3456](#3456)) ([66edc32](66edc32))
* Improve BQ point-in-time joining scalability ([#3429](#3429)) ([ff66784](ff66784))
* Pin typeguard to 2.13.3 which is what we are currently using. ([#3542](#3542)) ([61f6fb0](61f6fb0))
* Protobuf lower bound to 3.20 to alert that Feast is incompatible with tensorflow ([#3476](#3476)) ([9ca59e3](9ca59e3))
* Spark kafka processor sorting ([#3479](#3479)) ([f2cbf43](f2cbf43))
* UI working behind base url ([#3514](#3514)) ([9a3fd98](9a3fd98))
* Update go dependencies ([#3512](#3512)) ([bada97c](bada97c))

### Features

* Add Rockset as an OnlineStore ([#3405](#3405)) ([fd91cda](fd91cda))
* Add Snowflake Registry ([#3363](#3363)) ([ec1e61d](ec1e61d))
* Adding query timeout to `to_df` and `to_arrow` retrieval methods ([#3505](#3505)) ([bab6644](bab6644))
* adds k8s config options to Bytewax materialization engine ([#3518](#3518)) ([1883f55](1883f55))
achals pushed a commit that referenced this pull request Mar 24, 2023
# [0.30.0](v0.29.0...v0.30.0) (2023-03-24)

### Bug Fixes

* Add description attribute to the Field.from_proto method ([#3469](#3469)) ([473f8d9](473f8d9))
* Add filesystem kwargs when read prev_table on FileRetrievalJob (… ([#3491](#3491)) ([dca4745](dca4745)), closes [#3490](#3490)
* Bytewax image pull secret config ([#3547](#3547)) ([d2d13b1](d2d13b1))
* Clean up Rockset Online Store for use ([#3549](#3549)) ([a76c6d0](a76c6d0))
* Feature view `entities` from_proto type ([#3524](#3524)) ([57bbb61](57bbb61))
* Fix missing requests requirement after GCP requirement removed. Make BigQuerySource not require gcp extra ([2c85421](2c85421))
* Fix SQL Registry cache miss ([#3482](#3482)) ([3249b97](3249b97))
* Fixed path inside quickstart notebook ([#3456](#3456)) ([66edc32](66edc32))
* Improve BQ point-in-time joining scalability ([#3429](#3429)) ([ff66784](ff66784))
* Pin typeguard to 2.13.3 which is what we are currently using. ([#3542](#3542)) ([61f6fb0](61f6fb0))
* Protobuf lower bound to 3.20 to alert that Feast is incompatible with tensorflow ([#3476](#3476)) ([9ca59e3](9ca59e3))
* Spark kafka processor sorting ([#3479](#3479)) ([f2cbf43](f2cbf43))
* UI working behind base url ([#3514](#3514)) ([9a3fd98](9a3fd98))
* Update go dependencies ([#3512](#3512)) ([bada97c](bada97c))

### Features

* Add Rockset as an OnlineStore ([#3405](#3405)) ([fd91cda](fd91cda))
* Add Snowflake Registry ([#3363](#3363)) ([ec1e61d](ec1e61d))
* Added SnowflakeConnection caching ([#3531](#3531)) ([f9f8df2](f9f8df2))
* Adding query timeout to `to_df` and `to_arrow` retrieval methods ([#3505](#3505)) ([bab6644](bab6644))
* adds k8s config options to Bytewax materialization engine ([#3518](#3518)) ([1883f55](1883f55))
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.

feast>=0.25.1 breaks compatibility with protobuf<3.20
6 participants