[AIRFLOW-6162] Add back serialization as a module#6718
Conversation
There was a problem hiding this comment.
Yes. MyPy complains if it's not there. Maybe it's worth looking whether there is a good reason for that @kaxil ?
There was a problem hiding this comment.
Cool, other than that I am fine with this PR :)
Thanks @potiuk
There was a problem hiding this comment.
I think it's worth looking at it @kaxil -> there are still some parts of the code that needs some more typing to catch potential errors (DagBag especially) but this one looks a bit fishy to me.
There was a problem hiding this comment.
I think the root cause is that .subdag field is only set in SerializedBaseOperator and that's why the type is forced here.
nemkin
left a comment
nemkin
left a comment
There was a problem hiding this comment.
Tried this locally and it fixed the missing serialization module after running pip install . Thank you Jarek :)
|
Yes unfortunately, subdag field is only set in 2 places. 1) In
SubDagOperator and 2) In SerializedBaseOperator.
…On Tue, Dec 3, 2019, 12:04 Jarek Potiuk ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In airflow/serialization/serialized_dag.py
<#6718 (comment)>:
> @@ -101,7 +102,7 @@ def deserialize_dag(cls, encoded_dag: dict) -> "SerializedDAG":
setattr(dag, 'full_filepath', dag.fileloc)
for task in dag.task_dict.values():
task.dag = dag
- serializable_task: SerializedBaseOperator = task
+ serializable_task: SerializedBaseOperator = cast(SerializedBaseOperator, task)
I think the root cause is that .subdag field is only set in
SerializedBaseOperator and that's why the type is forced here.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6718?email_source=notifications&email_token=ACDHIJRK2BUGT2I7RAVJOELQWZDOTA5CNFSM4JUWMCP2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCNX3QWQ#discussion_r353139748>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACDHIJUQCSU3UYLNHUZJQ63QWZDOVANCNFSM4JUWMCPQ>
.
|
|
In my upcoming fix I also merge serialized_dag and serialized_baseoperator into one module 'serialized_objects' - because there are cyclic dependencies (again detected by pylint) between those two classes as well and it makes perfect sense to group them in one module. |
|
@kaxil @ashb @mik-laj -> Just pushed fixup which removes the cast and fixes it "properly" I think. I moved subdag to BaseOperator and added few missing type annotations. This has let to more cyclic dependencies (between SerializedDag and SerializedBaseOperator). I believe those two classes are so closely coupled that it makes sense to put them in one module (basically that's what pylint is complaining about when telling about "cyclic imports" - that classes in separate modules are so closely coupled that they are referring each other). BTW. I think the more type annotations we add the more couplings we detect and there will be another set of splits/merges between the core classes. We have still quite a few of not-annotated classes that are hiding such couplings. |
|
I will rebase it on top of #6596 to see if it works with hose fixes. |
Codecov Report
@@ Coverage Diff @@
## master #6718 +/- ##
==========================================
- Coverage 83.84% 83.51% -0.34%
==========================================
Files 669 668 -1
Lines 37692 37677 -15
==========================================
- Hits 31603 31465 -138
- Misses 6089 6212 +123
Continue to review full report at Codecov.
|
potiuk
force-pushed
the
AIRFLOW-6162-add-back-serialization-module
branch
from
fdb06ca to
42db902
Compare
|
Rebased now on top of #6596 |
potiuk
force-pushed
the
AIRFLOW-6162-add-back-serialization-module
branch
3 times, most recently
from
c8ff842 to
eeeb99e
Compare
|
And some more pylint complaints (rightfully so) I also moved BaseSerialization in the same "serialization_objects.py". Now this module is really what it should be :). |
potiuk
force-pushed
the
AIRFLOW-6162-add-back-serialization-module
branch
from
eeeb99e to
256e216
Compare
7 participants
Make sure you have checked all steps below.
Jira
Description
Tests
Commits
Documentation