moiseenkov commented on PR #38531:
URL: https://github.com/apache/airflow/pull/38531#issuecomment-2066306300
Thank you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To
potiuk commented on PR #38531:
URL: https://github.com/apache/airflow/pull/38531#issuecomment-2066247355
yes. Python Operator and related are part of the Airflow core.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use
moiseenkov commented on PR #38531:
URL: https://github.com/apache/airflow/pull/38531#issuecomment-2066188528
@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,
potiuk merged PR #38531:
URL: https://github.com/apache/airflow/pull/38531
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail:
moiseenkov commented on code in PR #38531:
URL: https://github.com/apache/airflow/pull/38531#discussion_r1570869596
##
hatch_build.py:
##
@@ -414,6 +414,7 @@
# Blinker use for signals in Flask, this is an optional dependency in
Flask 2.2 and lower.
# In Flask 2.3 it
moiseenkov commented on code in PR #38531:
URL: https://github.com/apache/airflow/pull/38531#discussion_r1570868914
##
airflow/operators/python.py:
##
@@ -58,6 +56,15 @@
from airflow.utils.process_utils import execute_in_subprocess
from airflow.utils.python_virtualenv import
potiuk commented on PR #38531:
URL: https://github.com/apache/airflow/pull/38531#issuecomment-2063711770
> @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.
potiuk commented on code in PR #38531:
URL: https://github.com/apache/airflow/pull/38531#discussion_r1570594374
##
hatch_build.py:
##
@@ -414,6 +414,7 @@
# Blinker use for signals in Flask, this is an optional dependency in
Flask 2.2 and lower.
# In Flask 2.3 it
potiuk commented on code in PR #38531:
URL: https://github.com/apache/airflow/pull/38531#discussion_r1570592867
##
hatch_build.py:
##
@@ -414,6 +414,7 @@
# Blinker use for signals in Flask, this is an optional dependency in
Flask 2.2 and lower.
# In Flask 2.3 it
potiuk commented on code in PR #38531:
URL: https://github.com/apache/airflow/pull/38531#discussion_r1570590368
##
airflow/operators/python.py:
##
@@ -58,6 +56,15 @@
from airflow.utils.process_utils import execute_in_subprocess
from airflow.utils.python_virtualenv import
moiseenkov commented on PR #38531:
URL: https://github.com/apache/airflow/pull/38531#issuecomment-2063310915
@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.
potiuk commented on PR #38531:
URL: https://github.com/apache/airflow/pull/38531#issuecomment-2059261399
> 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
potiuk commented on PR #38531:
URL: https://github.com/apache/airflow/pull/38531#issuecomment-2059244743
> 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
VladaZakharova commented on PR #38531:
URL: https://github.com/apache/airflow/pull/38531#issuecomment-2059177211
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
potiuk commented on PR #38531:
URL: https://github.com/apache/airflow/pull/38531#issuecomment-2058886015
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.
VladaZakharova commented on PR #38531:
URL: https://github.com/apache/airflow/pull/38531#issuecomment-2058820518
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
potiuk commented on PR #38531:
URL: https://github.com/apache/airflow/pull/38531#issuecomment-2052632462
> 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
moiseenkov commented on code in PR #38531:
URL: https://github.com/apache/airflow/pull/38531#discussion_r1562712418
##
airflow/models/taskinstance.py:
##
@@ -1287,7 +1287,7 @@ class TaskInstance(Base, LoggingMixin):
queued_dttm = Column(UtcDateTime)
queued_by_job_id =
moiseenkov commented on code in PR #38531:
URL: https://github.com/apache/airflow/pull/38531#discussion_r1560922438
##
airflow/models/taskinstance.py:
##
@@ -1287,7 +1287,7 @@ class TaskInstance(Base, LoggingMixin):
queued_dttm = Column(UtcDateTime)
queued_by_job_id =
potiuk commented on code in PR #38531:
URL: https://github.com/apache/airflow/pull/38531#discussion_r1559780893
##
airflow/models/taskinstance.py:
##
@@ -1287,7 +1287,7 @@ class TaskInstance(Base, LoggingMixin):
queued_dttm = Column(UtcDateTime)
queued_by_job_id =
moiseenkov commented on code in PR #38531:
URL: https://github.com/apache/airflow/pull/38531#discussion_r1559365639
##
airflow/models/taskinstance.py:
##
@@ -1287,7 +1287,7 @@ class TaskInstance(Base, LoggingMixin):
queued_dttm = Column(UtcDateTime)
queued_by_job_id =
potiuk commented on code in PR #38531:
URL: https://github.com/apache/airflow/pull/38531#discussion_r1556118149
##
airflow/models/taskinstance.py:
##
@@ -1287,7 +1287,7 @@ class TaskInstance(Base, LoggingMixin):
queued_dttm = Column(UtcDateTime)
queued_by_job_id =
VladaZakharova commented on code in PR #38531:
URL: https://github.com/apache/airflow/pull/38531#discussion_r1555416136
##
airflow/models/taskinstance.py:
##
@@ -1287,7 +1287,7 @@ class TaskInstance(Base, LoggingMixin):
queued_dttm = Column(UtcDateTime)
VladaZakharova commented on code in PR #38531:
URL: https://github.com/apache/airflow/pull/38531#discussion_r1555408124
##
airflow/models/taskinstance.py:
##
@@ -1287,7 +1287,7 @@ class TaskInstance(Base, LoggingMixin):
queued_dttm = Column(UtcDateTime)
VladaZakharova commented on code in PR #38531:
URL: https://github.com/apache/airflow/pull/38531#discussion_r1555400293
##
airflow/models/taskinstance.py:
##
@@ -1287,7 +1287,7 @@ class TaskInstance(Base, LoggingMixin):
queued_dttm = Column(UtcDateTime)
potiuk commented on code in PR #38531:
URL: https://github.com/apache/airflow/pull/38531#discussion_r1555046517
##
airflow/models/taskinstance.py:
##
@@ -1287,7 +1287,7 @@ class TaskInstance(Base, LoggingMixin):
queued_dttm = Column(UtcDateTime)
queued_by_job_id =
potiuk commented on code in PR #38531:
URL: https://github.com/apache/airflow/pull/38531#discussion_r1555045432
##
hatch_build.py:
##
@@ -144,6 +144,12 @@
"virtualenv": [
"virtualenv",
],
+"dill": [
+"dill",
Review Comment:
```suggestion
VladaZakharova commented on PR #38531:
URL: https://github.com/apache/airflow/pull/38531#issuecomment-2034922966
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
VladaZakharova commented on code in PR #38531:
URL: https://github.com/apache/airflow/pull/38531#discussion_r1549962879
##
airflow/operators/python.py:
##
@@ -416,8 +418,16 @@ def __init__(
**kwargs,
)
self.string_args = string_args or []
-
VladaZakharova commented on code in PR #38531:
URL: https://github.com/apache/airflow/pull/38531#discussion_r1549959875
##
airflow/operators/python.py:
##
@@ -548,9 +558,9 @@ class
PythonVirtualenvOperator(_BasePythonVirtualenvOperator):
"requirements file" as
hussein-awala commented on code in PR #38531:
URL: https://github.com/apache/airflow/pull/38531#discussion_r1545512710
##
airflow/models/dagpickle.py:
##
@@ -42,7 +42,7 @@ class DagPickle(Base):
"""
id = Column(Integer, primary_key=True)
-pickle =
potiuk commented on code in PR #38531:
URL: https://github.com/apache/airflow/pull/38531#discussion_r1540962108
##
airflow/operators/python.py:
##
@@ -548,9 +558,9 @@ class
PythonVirtualenvOperator(_BasePythonVirtualenvOperator):
"requirements file" as specified by
potiuk commented on PR #38531:
URL: https://github.com/apache/airflow/pull/38531#issuecomment-2022576617
This is cool :). But :)
Certainy we will need to keep backwards-compatibility option.
I think this change needs a bit more:
1) separate use_cloud_pickle from
VladaZakharova commented on PR #38531:
URL: https://github.com/apache/airflow/pull/38531#issuecomment-2022482736
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
34 matches
Mail list logo
