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

[Bug] Duplicate dependency specifications means prod and dev version of protobuf might differ #9830

Closed
2 tasks done
niteshy opened this issue Mar 27, 2024 · 5 comments · Fixed by #9838
Closed
2 tasks done
Labels
bug Something isn't working

Comments

@niteshy
Copy link
Contributor

niteshy commented Mar 27, 2024

Is this a new bug in dbt-core?

  • I believe this is a new bug in dbt-core
  • I have searched the existing issues, and I could not find an existing issue for this bug

Current Behavior

community Slack #adapter-ecosystem thread

As a part of #9630 PR, we restricted the protobuf to 4.x versions but the manual backport on v1.5 (#9676) & v1.6 (#9675) branches were incorrect because the restriction is only applied to dev-requirements rather than at the setup.py. As we can see in v1.5.10 and v1.6.10 This resulted in breaking the downstream adapters compatibilities as they started including protobuf > 5.x which is not compatible with dbt-core. It doesn't surface in dbt-core tests because dev-requirements always ensure that protobuf 4.x is installed.
Eg for errors happen in adapters

 def msg_to_dict(msg: EventMsg) -> dict:
        msg_dict = dict()
        try:
>           msg_dict = MessageToDict(
                msg, preserving_proto_field_name=True, including_default_value_fields=True  # type: ignore
            )
E           TypeError: MessageToDict() got an unexpected keyword argument 'including_default_value_fields'

/dbt/events/functions.py:227: TypeError

Another concern, we should not have duplicate dependencies listed in setup.py and dev-requirements.txt because these manual mistakes can happen again in the future.

Expected Behavior

All the downstream adapters in v1.5.10 & v1.6.10 should be using the protobuf 4.x versions to avoid the above errors.

Steps To Reproduce

  1. Use any adapter with latest v1.5 or v1.6 branches eg dbt-core v1.5.10
  2. Install the dependencies
  3. Check in the pip list if protobuf > 5. If it is then it is not compatible with dbt-core.

Relevant log output

No response

Environment

- OS: Ubuntu 20.04
- Python:3.8.16
- dbt:1.5

Which database adapter are you using with dbt?

other (mention it in "Additional Context")

Additional Context

No response

@niteshy niteshy added bug Something isn't working triage labels Mar 27, 2024
@niteshy niteshy changed the title [Bug] Backport for restricting the protobuf <5 are not done correctly on v1.5.x & v1.6.x branches [Bug] v1.5.10 & v1.6.10 branches are using protobuf > 5 Mar 27, 2024
@niteshy niteshy changed the title [Bug] v1.5.10 & v1.6.10 branches are using protobuf > 5 [Bug] v1.5.10 & v1.6.10 versions are using protobuf > 5 in prod Mar 27, 2024
@QMalcolm
Copy link
Contributor

QMalcolm commented Mar 28, 2024

Hi, thank you for opening this issue! We've already merged backports into 1.5.latest and 1.6.latest to fix this 🙂 We're doing patch releases for 1.5 and 1.6 today which will make the fix available via pypi

@dbeatty10 dbeatty10 removed the triage label Mar 28, 2024
@niteshy
Copy link
Contributor Author

niteshy commented Mar 28, 2024

I have put the fix for this #9831. This is the first time we are trying to put a fix on the dbt-core.
@QMalcolm can you help me to explain why tests are failing in #9831?

@dbeatty10
Copy link
Contributor

Thanks again for your help alerting about this issue and identifying the fix @niteshy 🏆

I'm going to close this issue since it is addressed in the following PRs:

@QMalcolm
Copy link
Contributor

QMalcolm commented Apr 4, 2024

@dbeatty10 & @niteshy I think we should actually re-open this, but slimmed down and re-titled to focus on

Another concern, we should not have duplicate dependencies listed in setup.py and dev-requirements.txt because these manual mistakes can happen again in the future.

The title could be altered to [House Keeping] Remove duplicate protobuf dependency specification in dev-requirements.txt

And the description should be something along the lines of

## Current Behavior
Currently we specify the protobuf dependency in both `setup.py` and `dev-requirements.txt`. This is unnecessary because whenever we install `dev-requirements.txt` we also install the requirements for core which pulls in protobuf from `setup.py`. It's good that when we run `make dev`, core's requirements are pulled in. However, it's bad that we update the protobuf specification in one place and not the other, then the version of protobuf we end up with in tests and production might significantly differ. 

@QMalcolm
Copy link
Contributor

QMalcolm commented Apr 4, 2024

Additionally @niteshy already has an open PR which takes care of the issue #9838

@QMalcolm QMalcolm reopened this Apr 4, 2024
@QMalcolm QMalcolm changed the title [Bug] v1.5.10 & v1.6.10 versions are using protobuf > 5 in prod [Bug] Duplicate dependency specifications means prod and dev version of protobuf might differ Apr 4, 2024
QMalcolm pushed a commit that referenced this issue Apr 5, 2024
* Removing unwanted dev dependency

* updating change log

* Remove duplicate dependency of protobuf in dev-requirements to resolve #9830

---------

Co-authored-by: Doug Beatty <44704949+dbeatty10@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants