Replace dill package to use cloudpickle

[VladaZakharova]

Related issue: #35307

---

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

[VladaZakharova]

Hi @potiuk @eladkal !
I have found pretty old issue related to upgrade of dill package. As I understood, replacement of dill with cloudpickle here makes more sense since we will not break beam dependencies in this case.
Can you please take a look on changes?
Thanks!

[potiuk]

This is cool :). But:

Certainy we will need to keep backwards-compatibility option (and I do not think dill should be replaced really - they should be two equivalent options).

I think this change needs a bit more:

1. separate use_cloud_pickle from use_dill (and failure when both are true)
2. turning dill into an optional extra (Similar to virtualenv in hatch_build.py
3. similarly adding cloudpickle into an optional extra rather than mandatory requirement
4. Everywhere where dill and cloudpickle are used we need to raise an exception (on ImportError) explaining that this is an optional feature and how to enable it (by using [dill], [cloudpickle] extras respectively or installing the dill/cloudpickle dependencies
5. significant newsfragment explaining that dill is no longer automatically installed and that [dill] extra is needed to use dill
6. Some more documentation explaining differences between the two (somewhere here : https://airflow.apache.org/docs/apache-airflow/stable/howto/operator/python.html#pythonvirtualenvoperator)

This way we will:

• stop depending on dill as mandatory requirement
• will not start depending on cloudpickle as mandatory requirement
• have an easy way for users to continue using dill (add [dill] extra - without them having to adapt touse cloudpickle that might introduce various incompatibilities

[VladaZakharova]

Thank you @potiuk and @hussein-awala for your comments!
I have updated the code to follow all suggestions.
Right now thinking about this migration script. Right now both dill and cloudpickle will be optional, so the best idea here from my point of view will be to use just pickle, if none of the optional modules are provided. WDYT?

[potiuk]

Unlike dill and cloudpickle, serde's serialize() method loses an object's type information similar to what json.dumps() does:

It shoudl not . @bolkedebruin -> i believe serde should be good for it and we should be able to do round-trip serialization of most types

[VladaZakharova]

Hi @bolkedebruin @potiuk !
Friendly reminder on this PR, we are actually waiting for some response to start implementation, since as I understand there will be a lot of changes to make until we actually will be able to close this ticket :)
Maybe someone else can also be involved in this conversation here?

[potiuk]

I think if you have no confirmation from @bolkedebruin on the proposed path, the way to go is to implement POC and see if it works with the current executor configs @VladaZakharova. There is no better way to confirm the approach.

[VladaZakharova]

@potiuk If we are talking about migration and changing the way we serialize, should we consider changing dill to use json? Are there some limitations here? By doing this, we can try to avoid the same problem in the future for other Python versions. WDYT?

Also regarding the original issue with incorrect serialization for Python 3.11, is it a problem only with serialization or deserialization too? If we can use dill for Python 3.11 only for deserialization, and cloudpickle for serialization, is there will be a problem?

@hussein-awala @Taragolis Can you give us some details here? Thanks!

[potiuk]

Are there some limitations here?

As mentioned before - we need to be able to handle different serializers - to handle the K8S configuration problem described above in #38531 (comment). This is the reason we have dill in the first place. If serde will not solve the problem (seems not) - then the solution with storing pickler together with serialized value seems to be good direction - providing that migration scenarios will be part of the solution of course.

[potiuk]

Also regarding the original issue with incorrect serialization for Python 3.11, is it a problem only with serialization or deserialization too? If we can use dill for Python 3.11 only for deserialization, and cloudpickle for serialization, is there will be a problem?

The Python 3.11 issue is really a "test" issue - I am not really sure if this has the effect in production.

BTW. I think there is a little misunderstanding here. If the only reason for this change is supporting cloudpickle in Python Virtualenv Operato and not getting rid of dill, then we can likely leave dill as mandatory dependency of Airflow and do not worry about the configuration for executor - and only get cloudpickle as optional dependency for Python virtualenv operator.

Maybe I was assuming too much of a reason for that change when I was commending on dill used in the executor. And we can leave that part altogether - concentrating back on just Python Virtualenv Operator.

[moiseenkov]

@potiuk ,
Thank you for helping with this PR. We see that at this point modifying Airflow core has really high cost and the data migration brings additional risks for customers.
If we come back to the original issue, as you mentioned, the problem is failing a test for a particular operator. And apparently, it can be solved without core modification, because the PythonVirtualenvOperator's logic (virtual environment preparation) has nothing to do with Airflow models serialization. Therefore I updated this PR and left only changes in the operator level. So the Airflow core remains unchanged. It should resolve the initial problem. Please take a look.
And thank you once again!

[potiuk]

@potiuk ,
Thank you for helping with this PR. We see that at this point modifying Airflow core has really high cost and the data migration brings additional risks for customers.
If we come back to the original issue, as you mentioned, the problem is failing a test for a particular operator. And apparently, it can be solved without core modification, because the PythonVirtualenvOperator's logic (virtual environment preparation) has nothing to do with Airflow models serialization. Therefore I updated this PR and left only changes in the operator level. So the Airflow core remains unchanged. It should resolve the initial problem. Please take a look.
And thank you once again!

Yes, sorry for the confusion - I think indeed we should limit that change to only that - all the complexity that replacing core executor config might still remain. It might not help with some dependency issues (I.e. dill will still be a core depenency) - but it will give the users a way to handle their pickling for PVO / External Python Operator better.

[moiseenkov]

@potiuk , hi
Thanks for reviewing and merging this. We have only one short question here if you don't mind.
Because these changes are not contained by any provider package, does it mean that they will be released with the Airflow itself?

[potiuk]

yes. Python Operator and related are part of the Airflow core.

[moiseenkov]

Thank you!