AIP44 Fix DAG serialization by mhenc · Pull Request #34042 · apache/airflow

mhenc · 2023-09-02T20:58:05Z

DAG serialization using BaseSerialization doesn't encode it with '__type'/'__var' fields, making it not-deserializable (no information about the type).

This object is needed by Internal API to be fully serializable/deserializable.

Also added more tests for BaseSerialization.serialize/deserialize methods to cover all supported object types (and fixed few minor isssues).

^ 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.

kaxil · 2023-09-03T23:32:21Z

airflow/serialization/serialized_objects.py

There is no need to encode this, we expect it to have a top level "dag" unique key as shown in

airflow/tests/serialization/test_dag_serialization.py

Line 123 in bf2d411

"dag": {

Similarly, we don't encode tasks or each operator, because we know the structure that it would be within ["dag"]["tasks"]

Check L445 and L447

When building DAG s10n it was designed to using Airflow internal knowledge to not inflate the final blob and optimize wherever possible. This is the reason we don't store defaults and objects that are None for DAG and operator objects

Also, be careful about backwards-incompatible changes for this.

And if for AIP-44 we absolutely need this sort of change, that will probably be more involved than just the DAG object

Right, but:
if I serialize dag (with BaseSerialization.serialize) then try to deserialize it with BaseSerialization.deserialize it fails - as it doesn't get there
https://github.com/apache/airflow/blob/main/airflow/serialization/serialized_objects.py#L551
because

var = encoded_var[Encoding.VAR] type_ = encoded_var[Encoding.TYPE]

failes (key not exists).

also we may need to send the DAG as part of bigger structure (e.g. https://github.com/apache/airflow/blob/main/airflow/dag_processing/processor.py#L882)

In code I am not able to find any usages of BaseSerialization.serialize on DAG (at least when looking for something like BaseSerialization.serialize.*dag

We are currently working on making sure all other objects types are serializable.

Yeah. I think @mhenc is right - this part of code seems to be not used before - it's not DAG serialization that gets afffected here, it's just serializing the whole dag as part of bigger structure. I have not seen any place in the code where we'd do that before.

kaxil

https://github.com/apache/airflow/pull/34042/files#r1314333805

uranusjr · 2023-09-04T06:29:15Z

airflow/serialization/pydantic/dag_run.py

This field should not be None

I also thought that, but it is None during creation (before submission to DB) - it is assigned by SQLAlchemy after you run

session.add(obj) session.commit()

But when do we ever serialize an unsaved DagRun?

Fair point :)
We may need it for TriggerDag Operator - but this is outside of this PR.
So currently it's only for tests (but I can fix them) and also for consitency - some *Pydantic objects will have id as Optional (e.g. Job #34026) so we may keep it Optional there for consistency

But if you belive it's better to leave it as "int" then I can revert this change.

I’m not sure Job should have an optional ID either. Maybe make this not optional for now and we can punt this until it’s nocessary for a model. It’s always easier to remove a restriction than add one.

Actually Job must have it as in
#34026
we call "add_job" which is actual the case where Id is empty.

Removing it from DagRunPydantic.

potiuk

LGTM. But wait for @kaxil / @uranusjr comments.

mhenc · 2023-09-11T13:26:05Z

@uranusjr @kaxil Do you have any more comments for this PR?

potiuk · 2023-09-22T04:51:02Z

2 tasks failing,

uranusjr · 2023-10-17T23:56:04Z

Is the .orig file left in accidentally?

mhenc · 2023-10-18T11:30:14Z

Is the .orig file left in accidentally?

Yes, by accident after conflict resolution. Removed.

potiuk · 2023-10-18T14:25:31Z

@kaxil ?

potiuk · 2023-10-25T10:29:33Z

@kaxil 🙏

kaxil · 2023-10-26T23:32:15Z

@kaxil 🙏

oof, looking now

boring-cyborg bot added the area:serialization label Sep 2, 2023

mhenc requested review from potiuk and vincbeck September 2, 2023 20:58

mhenc marked this pull request as ready for review September 2, 2023 20:59

mhenc requested review from ashb, bolkedebruin and kaxil as code owners September 2, 2023 20:59

kaxil reviewed Sep 3, 2023

View reviewed changes

kaxil requested changes Sep 3, 2023

View reviewed changes

uranusjr reviewed Sep 4, 2023

View reviewed changes

mhenc force-pushed the fix_dag_serialization branch from 29d7495 to 8635849 Compare September 4, 2023 13:38

mhenc requested review from kaxil and uranusjr September 4, 2023 13:42

potiuk approved these changes Sep 4, 2023

View reviewed changes

mhenc force-pushed the fix_dag_serialization branch from 8635849 to 3285ccd Compare September 6, 2023 11:26

mhenc force-pushed the fix_dag_serialization branch from d0e1f74 to 0d95d5a Compare September 12, 2023 21:24

potiuk approved these changes Sep 22, 2023

View reviewed changes

mhenc force-pushed the fix_dag_serialization branch 2 times, most recently from e037603 to 2de0d75 Compare October 17, 2023 10:00

mhenc force-pushed the fix_dag_serialization branch 5 times, most recently from 957cce5 to 5635bee Compare October 18, 2023 11:29

potiuk approved these changes Oct 18, 2023

View reviewed changes

mhenc requested a review from potiuk October 18, 2023 14:34

vincbeck approved these changes Oct 25, 2023

View reviewed changes

kaxil approved these changes Oct 26, 2023

View reviewed changes

Fix dag serialization

2bf360e

mhenc force-pushed the fix_dag_serialization branch from 5635bee to 2bf360e Compare October 27, 2023 06:59

potiuk merged commit 64a64ab into apache:main Oct 27, 2023

mhenc deleted the fix_dag_serialization branch October 27, 2023 12:14

ephraimbuddy added this to the Airflow 2.7.3 milestone Oct 29, 2023

ephraimbuddy added the type:bug-fix Changelog: Bug Fixes label Oct 29, 2023

ephraimbuddy modified the milestones: Airflow 2.7.3, Airflow 2.8.0 Oct 29, 2023

Conversation

mhenc commented Sep 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

potiuk Sep 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaxil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

potiuk left a comment

Choose a reason for hiding this comment

Uh oh!

mhenc commented Sep 11, 2023

Uh oh!

potiuk commented Sep 22, 2023

Uh oh!

uranusjr commented Oct 17, 2023

Uh oh!

mhenc commented Oct 18, 2023

Uh oh!

potiuk commented Oct 18, 2023

Uh oh!

potiuk commented Oct 25, 2023

Uh oh!

kaxil commented Oct 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mhenc commented Sep 2, 2023 •

edited

Loading

potiuk Sep 4, 2023 •

edited

Loading