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

Add reason for enabling XCOM pickling #747

Merged
merged 16 commits into from
Sep 6, 2022

Conversation

sunank200
Copy link
Contributor

Description

What is the current behavior?

Missing reason in docs and readme for enabling XCOM pickling

closes: #739

What is the new behavior?

  • Add reason for enabling XCOM pickling in docs and README

Does this introduce a breaking change?

No

Checklist

  • Created tests which fail without the change (if possible)
  • Extended the README / documentation, if necessary

@codecov
Copy link

codecov bot commented Aug 25, 2022

Codecov Report

Merging #747 (9d08b30) into main (8d44e35) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #747   +/-   ##
=======================================
  Coverage   93.26%   93.26%           
=======================================
  Files          43       43           
  Lines        1855     1855           
  Branches      232      232           
=======================================
  Hits         1730     1730           
  Misses         97       97           
  Partials       28       28           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Collaborator

@utkarsharma2 utkarsharma2 left a comment

Choose a reason for hiding this comment

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

LGTM. cc: @kaxil can you check this as well?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@dimberman dimberman left a comment

Choose a reason for hiding this comment

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

Please check my comments for potential changes.

@sunank200 sunank200 force-pushed the document-enabling-XCom-pickling-reason branch from 0eec9e7 to f548961 Compare August 29, 2022 10:51
@sunank200
Copy link
Contributor Author

@dimberman please re-review

README.md Outdated

Currently, custom XCom backends are limited to data types that are json serializable. Since Dataframes are not json serializable, we need to enable XCom pickling to store dataframes.

The data format used by pickle is Python-specific. This has the advantage that there are no restrictions imposed by external standards such as JSON or XDR (which can’t represent pointer sharing); however it means that non-Python programs may not be able to reconstruct pickled Python objects.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would add in context to user for why we need this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

however it means that non-Python programs may not be able to reconstruct pickled Python objects.

I mean this line, we don't need that one ^^. But I will let you decide

Copy link
Member

Choose a reason for hiding this comment

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

Maybe if someone wants to opt-out and use airflow rest api?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not understand @feluelle. Could you elaborate on this, please?

Copy link
Member

Choose a reason for hiding this comment

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

If you read data from xcom via the airflow rest api you would not be able to reconstruct it or? Maybe only with a python rest api client - but not the go client for example or? (that is what the sentence is saying, right? So maybe it is useful to leave it there.)

@sunank200 sunank200 requested a review from kaxil September 2, 2022 10:56
Copy link
Collaborator

@kaxil kaxil left a comment

Choose a reason for hiding this comment

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

Approving it but do check my comment

README.md Outdated Show resolved Hide resolved
@kaxil
Copy link
Collaborator

kaxil commented Sep 6, 2022

@kaxil
Copy link
Collaborator

kaxil commented Sep 6, 2022

Can you rebase your branch on main too please

@sunank200
Copy link
Contributor Author

sunank200 commented Sep 6, 2022

Docs build is still failing - https://github.com/astronomer/astro-sdk/runs/8207855916?check_suite_focus=true

@kaxil commit should fix this.

@sunank200
Copy link
Contributor Author

@kaxil please re-review this. I think its good to merge.

@sunank200 sunank200 merged commit 1f5f16c into main Sep 6, 2022
@sunank200 sunank200 deleted the document-enabling-XCom-pickling-reason branch September 6, 2022 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add note on the reason for enabling XCom Pickling
6 participants