Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Declare data interval fields as serializable #19616

Merged

Conversation

uranusjr
Copy link
Member

So they are available in PythonVirtualenvOperator. Also added a few more keys that are trivially serializable but was missing previously.

A test is added to make sure we declare all keys so this does not happen again in the future.

See #9394

I’m not really sure why exactly ti and task_instance are not being serialised. Those exist back when #9394 was implemented so they seem to be omitted intentionally? But I didn’t see any discussion on the decision.

ds was also omitted for some reason, but it’s just a string so I just added it.

cc @eladkal

So they are available in PythonVirtualenvOperator. Also added a few more
keys that are trivially serializable but was missing previously.

A test is added to make sure we declare all keys so this does not happen
again in the future.
@boring-cyborg boring-cyborg bot added the area:core-operators Operators, Sensors and hooks within Core Airflow label Nov 16, 2021
Copy link
Contributor

@eladkal eladkal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @uranusjr !

LGTM

I’m not really sure why exactly ti and task_instance are not being serialised. Those exist back when #9394 was implemented so they seem to be omitted intentionally? But I didn’t see any discussion on the decision.

@feluelle maybe you can provide some insight on this as the author of #9394

@eladkal eladkal added this to the Airflow 2.2.3 milestone Nov 16, 2021
@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Nov 16, 2021
@github-actions
Copy link

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@uranusjr
Copy link
Member Author

#12985 is another issue related to the ti key. If we end up adding it to the list of serialisable fields, we can close that issue as fixed.

@potiuk
Copy link
Member

potiuk commented Nov 16, 2021

Isn't that the same case here: #19618

@potiuk
Copy link
Member

potiuk commented Nov 16, 2021

Isn't that the same case here: #19618

Doesn't look like - as execution_date was alredy there ?

@uranusjr
Copy link
Member Author

Yeah that one is a different issue, probably due to we changing the variable to a lazy proxy.

@uranusjr uranusjr merged commit 264cb09 into apache:main Nov 16, 2021
@uranusjr uranusjr deleted the data-interval-fields-to-virtualenv-context branch November 25, 2021 06:32
@jedcunningham jedcunningham added the type:bug-fix Changelog: Bug Fixes label Dec 6, 2021
jedcunningham pushed a commit that referenced this pull request Dec 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:core-operators Operators, Sensors and hooks within Core Airflow full tests needed We need to run full set of tests for this PR to merge type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants