Remove mysql-connector-python

[moiseenkov]

Turn the package mysql-connector-python optional to exclude it from dependencies due to conflicts with the newer protobuf versions coming with Google SDK upgrade #30067

[eladkal]

Wait.

In https://lists.apache.org/thread/j98bgw9jo7xr4fvjh27d6bfoyxr1omcm we agreed to exclude providers that block us. We did not agree to remove capabilities completely.

There are users who use this functionality and possibly users of Mysql that dont care about Google provider.
It's one thing to hold releases for MySql it's another thing to remove capabilities all together.

Mysql provider changes are not that frequent. Why can't we suspend the provider and see if the proto dependency is resolved given more time? I don't think we are in the step where we need to choose between dropping capabilities vs adding new feature to the provider.

[potiuk]

Agree with @eladkal - this is far too much. We should either suspend the provider or (better) turn mysql-connector-python into optional feature.

[moiseenkov]

Thank you all, I misunderstood the point.

I will leave the code that uses mysql-connector-python, but is it true that we have to exclude this package from dependencies anyway? If I get it right, leaving mysql-connector-python here will cause dependency conflicts.

My suggestion is removing the mysql-connector-python from the dependencies, and throwing an exception with a readable message whenever the package is used. Those users, who will face this issue, are always able to install this package manually and resolve possible conflicts on their own, or transition to the recommended library. @eladkal , @potiuk WDYT?

[potiuk]

Thank you all, I misunderstood the point.

I will leave the code that uses mysql-connector-python, but is it true that we have to exclude this package from dependencies anyway? If I get it right, leaving mysql-connector-python here will cause dependency conflicts.

My suggestion is removing the mysql-connector-python from the dependencies, and throwing an exception with a readable message whenever the package is used. Those users, who will face this issue, are always able to install this package manually and resolve possible conflicts on their own, or transition to the recommended library. @eladkal , @potiuk WDYT?

You can add optional dependencies (like mysql-connector-python in this case) via provider.yaml. We already have several examples of it. For example google provider has extra "[leveldb]" which brings in plyvel dependency.

https://github.com/apache/airflow/blob/main/airflow/providers/google/provider.yaml#L1121

And you should move mysql-connector-python there from main dependencies - following the same pattern.

There are a few more changes that will need to be added:

• The tests that are using mysql-connector-python will start failing, and that's fine - we need to separate them to a separate module and run `pytest.importorskip("mysql.connector") for that file - this will skip them on our CI, but if you manually add "mysql-connector-python" and run them, we could potentially have a separate job to run it on CI separately (and I will likely add it later) but for now skipping it is fine.

• In the original operator you should raise "AirflowOptionalProviderFeatureException" when import of mysql.connector fails (with some description that you need mysql-connector-python to be installed.

That should remove mysql-connector-python from installed dependencies and turn it's usage into, effectively optional feature.

[moiseenkov]

Thanks for the explanation. I updated the PR, please re-review.

[potiuk]

This looks cool and has the desired effect, mysql-connector-python is removed from the image:

https://github.com/apache/airflow/actions/runs/4636423980/jobs/8204304593?pr=30487#step:5:18767

2c2
< # This constraints file was automatically generated on 2023-04-06T12:47:16Z
---
> # This constraints file was automatically generated on 2023-04-07T07:51:23Z
77c77
< aws-sam-translator==1.63.0
---
> aws-sam-translator==1.64.0
81c81
< azure-core==1.26.3
---
> azure-core==1.26.4
88c88
< azure-mgmt-core==1.3.2
---
> azure-mgmt-core==1.4.0
141c141
< coverage==7.2.2
---
> coverage==7.2.3
175c175
< filelock==3.10.7
---
> filelock==3.11.0
321c321
< mypy-boto3-rds==1.26.102
---
> mypy-boto3-rds==1.26.108
325d324
< mysql-connector-python==8.0.32
473c472
< slack-sdk==3.20.2
---
> slack-sdk==3.21.0

However the side-effect is that some new warnings are generated while importing requests (See the failing testts - it contains explanation what to do to mark the warnings as "known" - those do not seem as harmful ones.

[potiuk]

LGTM. Pending fixing the new warnings generated during import.

[potiuk]

Nice! @eladkal ?

[eladkal]

This is a breaking change for mysql provider. We should update provider yaml version + add entry in change log explaining to users who relay on this lib how they should migrate (install the extra)

[eladkal]

Also please update PR title + description so users won't get the wrong impression

[potiuk]

I am not 100% sure if it is breaking to be honest. We already agreed that dependency changes are non-breaking and assuming somoene already has installed mysql-connector-python, it will continue to work after the upgrade.

Breaking means - if somoene upgrades the provider, things will break. Which is not necessarily true - if someone upgrade from past provider, things will continue to work (because the user will have mysql-connector-python already installed). The user might be prevented from installing the newer provider (because of protobuf for example) but this also does not mean that it's a "breaking change".

I think, technically speaking, it's not "strongly" breaking - the usual "breaking" pattern when things break when you upgrade does not "technically" apply.

There are other scenarios where things might break - for example when you upgrade airflow image and you expect mysql operators to continue to work. But also technically it is not breaking, because images are "convenience" binaries, and while people might use them in charts, they are not even versioned with breakin/non-breaking (because they contain combination of airflow + possibly breaking providers).

So I am not really sure if we should classify that change as breaking.

[potiuk]

We might still of course "classify" it as breaking (nothing prevents us from doing so, we can announce any release breaking if we think doing so will prevent some common problems from our users).

But I personally don't think it is strong enough case to classify it as such.

[eladkal]

I am not 100% sure if it is breaking to be honest. We already agreed that dependency changes are non-breaking and assuming somoene already has installed mysql-connector-python, it will continue to work after the upgrade.

I'm ok with not listing it as breaking as long as we have a change log entry that explains what is changed. 2-3 lines in enough

[moiseenkov]

@potiuk , hi, can we merge it please?

[potiuk]

I saw that for some reasons checks were not running. I rebased to rebuild it.

[potiuk]

Rebased once more after merging the #30625

[potiuk]

FYI. MySQL does not really collaboarate. They released a new version of mysql-connector-python 8.0.33` two days ago, but it still has < 3.20 limit on protobuf. Also, I got this - completely useless and apparently automatically generated -response today to my issue:

Here are the general steps to upgrade to a newer version of protobuf:

Check compatibility: Before upgrading to a newer version of protobuf, check if the new version is compatible with your current codebase and other dependencies. Review the release notes and documentation to see if there are any changes or deprecations that could affect your code.

Update protobuf files: If you have any custom protobuf files, update them to the latest version. You can use the protoc compiler to generate updated code from the new protobuf files.

Update dependencies: If you are using a language-specific implementation of protobuf (such as the Python protobuf package or the Java protobuf library), update your dependencies to the latest version.

Update code: Update your code to use the new version of protobuf. This may involve updating method signatures, changing import statements, and updating serialization/deserialization code.

Test thoroughly: After making the updates, test your code thoroughly to ensure that everything works as expected. Test both positive and negative cases to ensure that there are no regressions or new issues introduced by the upgrade.

Roll back if necessary: If you encounter any issues or compatibility problems, be prepared to roll back to the previous version of protobuf until the issues can be resolved.

By following these steps, you should be able to successfully upgrade to a newer version of protobuf while minimizing the risk of compatibility issues and regressions.

I answered appropriately : https://forums.mysql.com/read.php?50,708413,708803#msg-708803

[notatallshaw-gts]

FYI I see they actually replied in a more useful manner on this post that they are facing a bug in protobuf with trying to upgrade to protobuf 4: https://forums.mysql.com/read.php?50,708753,708764#msg-708764

We are currently working on upgrading Python Protobuf to 4.x in the upcoming release, but we are facing a blocker. A bug in the latest protobuf version (protocolbuffers/protobuf#12373) is affecting our implementation.

So at least that answer contains information.

[potiuk]

So at least that answer contains information.

Let's see. Yes it's interesting that they respond differently to different issues. But - it is not an issue for us any more. It's issue for them after we merged this PR.

[nmariz]

(Disclaimer: I'm the lead developer of Connector/Python)
Like mentioned by @notatallshaw-gts, MySQL Connector/Python has a blocker (see protocolbuffers/protobuf#12373) for upgrading to Protobuf 4.22.x.
We are trying our best to find workarounds. However, in our case, it only affects us with Protobuf +4.22.0. So, in case Protobuf bug is not fixed on time, we are adding support until Protobuf 4.21.12 in the upcoming release.

@potiuk I'm sorry very about the late reply on the forum. This indeed is the same issue.

[potiuk]

Cool. Great to know it! @nmariz . Thanks for letting us know - looking forward to getting it fixed :)