yandex-cloud / python-sdk

Yandex.Cloud Python SDK
MIT License
86 stars 27 forks source link

Protobuf4 support #71

Closed potiuk closed 1 year ago

potiuk commented 1 year ago

In Apache Airflow we have yandexcloud integration that currently holds us back from upgrading to protobuf 4. One of them is yandexcloud dependency.

Having limitation to Requires-Dist: protobuf (<3.21) prevents us from upgrading most of other providers - see https://github.com/apache/airflow/pull/30067, Until yandex cloud fixes the dependency limitation, we will be forced to stop releasing the provider together with the others (it also means it will not be compatible with the other providers and newer versions of airflow when released

Can you please let us know if there are plans to upgrade the yandexcloud python sdk to not limit the protobuf? Otherwise we will have to exclude the yandexcloud provider and stop releasing it.

Thanks in advance!

cc: @Piatachock, @s0neq, @peter-volkov -> those who contributed some "real" changes to the provider.

potiuk commented 1 year ago

FYI. I started discussion in apache-airflow devlist https://lists.apache.org/thread/j98bgw9jo7xr4fvjh27d6bfoyxr1omcm and we are discussing the approach in case the dependency like yandexcloud is holding us back, datails of my proposal there, feel free to chime-in if you would like to have some construcitve proposals.

Piatachock commented 1 year ago

Hi Jarek! Thanks for the detailed information, we will look on the problem and return to you with reply.

@MAnyKey please take a look on protobuf dependency being in discussion. Can we bump it to protobuf4?

l0kix2 commented 1 year ago

I think we can, but it can generate non backward compatible stubs, so existing user code can break. Also we should check if examples in public docs (for sls functions for example) are still working with the new version.

Considering everything mentioned before I'm afraid it is not 5-minute task, maybe we should consider handing it to the public API ecosystem team?

l0kix2 commented 1 year ago

Also I must say that we don't have a test case, that can ensure that proto version used for testing is in sync with the one used in runtime. We had an issue with that in the past and I think we should add such test case.

potiuk commented 1 year ago

FYI - we are just agreeing on our policy for such cases in airflow, so just so you are aware, our policy for "suspending" new releases and tests for such problematic cases is being discussed here https://github.com/apache/airflow/pull/30359

potiuk commented 1 year ago

FYI. (just wanted to make everyone aware, also so you are not surprised here).

The lazy consensus on suspension process for provider in airflow have been reached https://lists.apache.org/thread/g8b3k028qhzgw6c3yz4jvmlc67kcr9hj.

Here is the current version of the process description: https://github.com/apache/airflow/pull/30359 and I have a suspension mechanism PR in review https://github.com/apache/airflow/pull/30422 - we are also (together with google) working hard on removal of the protobuf3 limitations from Google provider as soon as possiblem so unles there will be a release of yandexcloud with protobuf 4 support released, I will initiate suspension consensus request for yandex provider in Airflow devlist (next week likely). You might be able to resume it by basically creating a PR with reverted "suspension" change - once new release with protobuf 4 is out - it will not require any devlist communication.

Also just to explain why we have to do it (it happend that yandex is the first one that falls into this case) - we have a really hard time where protobuf3 limited us from releasing a google provider for ads support after v11 api has been disabled, and the only reason for that was the protobuf3 limitation. So we needed to find a way to (hopefully temporarily) exclude certain providers that are problematic from our regular releases so that we can handle such cases. Because of that we ended up with having to vendor-in new google ads library and this is less-than-ideal even if we find it solves the problem of our users (we want to remove it asap):

Just wanted you to be aware about the reasonind and process, so that you know the reasoning. Hopefully you will be able to fix it soon - protobuf3 Python support ends in Q2 2023 so you would likely have to do it anyway: https://protobuf.dev/support/version-support/#python

potiuk commented 1 year ago

FYI. tomorrow the lazy consensus is ending and we are going to suspend the provider in Airflow

l0kix2 commented 1 year ago

We'll take this issue into the reseach, but it seems we won't be able to look at this in the next month. Am I understanding correctly that from today Apache Airflow won't be integrated with Yandex Cloud at all until we fix the depencency?

Piatachock commented 1 year ago

As far as I understand, new releases of Airflow will not support our integration provider, so - pretty much yes, no YandexCloud in Airflow new versions from now on, until we support protobuf4.

potiuk commented 1 year ago

Not exactly. It's not as bad as it might seem (thus suspending not removing) - we are just suspending yandex from new releases of providers.

Currently released providers will continue to work. Yandex was not released in airflow PROD images by default so it does not change much for already released airflow versions. We are NOT taking down those providers.

There are two potential problems yandex might hit in the future though:

So there are no "immediate" effects only future consequences.

Simply - no more yandex provider releases until the problem is fixed. Also, the longer it is delaying, the more difficult it will be to bring it back. The tests and imports for yandex provider are skipped now, so it is very likely there will be changes that will make it harder and harder to bring the tests back and make the tests green again.

Note that it will be on maintainers of Yandex (or whoever will want to resume yandex) to create a PR and make the tests green again. Nobody wll actively check if yandex cloud is made compatible/released, the yandexcloud maintainers will have to actively resume it.

The process is described here so it should be easy enough to follow it (but then the longer it takes, the more difficult it will be to get it into green):

https://github.com/apache/airflow/blob/main/airflow/providers/SUSPENDING_AND_RESUMING_PROVIDERS.rst

The good news is there will be no need to vote it on devlist - you will be able to resume the suspended provider by just having a maintainer-approved, green merged PR.

More details about the process is here:

https://github.com/apache/airflow/blob/main/PROVIDERS.rst#suspending-releases-for-providers

potiuk commented 1 year ago

Also one more consequence - one of the old PRs to add feature to Yandex provider has been rebased today https://github.com/apache/airflow/pull/29811 but until the problem is solved we are not going to run CI nor merge any of such PRs.

zandreika commented 1 year ago

Hi @potiuk! We have upgraded protobuf to the 4th version

potiuk commented 1 year ago

Yep. Already iterating on th ePR bringing yandex provider back https://github.com/apache/airflow/pull/33574