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

Fix PythonVirtualenvOperator not working with Airflow context #9394

Merged
merged 1 commit into from
Jul 30, 2020

Conversation

feluelle
Copy link
Member

@feluelle feluelle commented Jun 18, 2020

Primarly this PR fixes an issue with PythonVirtualenvOperator not working in conjunction with airflow context arg.

Relates to #8177
Relates to #8256

But it also has a QoL improvement. You don't need to manually pass dill as requirement anymore if you have set use_dill to True. So it won't raise an Error if you forgot to add it.


Make sure to mark the boxes below before creating PR: [x]

  • Description above provides context of the change
  • Unit tests coverage for changes (not needed for documentation changes)
  • Target Github ISSUE in description if exists
  • Commits follow "How to write a good git commit message"
  • Relevant documentation is updated including usage instructions.
  • I will engage committers as explained in Contribution Workflow Example.

In case of fundamental code change, 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 UPDATING.md.
Read the Pull Request Guidelines for more information.

cc @maganaluis

@feluelle feluelle requested a review from mik-laj June 18, 2020 20:49
@feluelle feluelle force-pushed the refactor-python-venv-operator branch 2 times, most recently from 441ed1a to 0253d8d Compare June 19, 2020 07:25
@mik-laj
Copy link
Member

mik-laj commented Jun 19, 2020

@feluelle I am a bit busy with other changes. I'm afraid I may not find time to look at it.

@Fokko Can you look at it? I know you are more familiar with this code.

@feluelle feluelle changed the title Refactor PythonVirtualenvOperator [WIP] Refactor PythonVirtualenvOperator Jun 19, 2020
@feluelle feluelle force-pushed the refactor-python-venv-operator branch from 0253d8d to 04903ec Compare June 19, 2020 14:49
@feluelle feluelle changed the title [WIP] Refactor PythonVirtualenvOperator Fix PythonVirtualenvOperator not working with Airflow context Jun 19, 2020
@feluelle feluelle force-pushed the refactor-python-venv-operator branch 5 times, most recently from 22533e8 to 3043290 Compare June 22, 2020 14:10
@feluelle feluelle requested review from ashb and turbaszek June 22, 2020 15:41
airflow/operators/python.py Outdated Show resolved Hide resolved
@feluelle feluelle force-pushed the refactor-python-venv-operator branch 2 times, most recently from cd92619 to c3c644c Compare June 24, 2020 10:55
@feluelle feluelle force-pushed the refactor-python-venv-operator branch from c3c644c to b4ee6f7 Compare June 24, 2020 13:23
@feluelle
Copy link
Member Author

PTAL @turbaszek @maganaluis @ashb @Fokko

Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

Looks good, thanks @feluelle

airflow/operators/python.py Outdated Show resolved Hide resolved
@feluelle feluelle changed the title Fix PythonVirtualenvOperator not working with Airflow context [WIP] Fix PythonVirtualenvOperator not working with Airflow context Jul 10, 2020
@feluelle feluelle force-pushed the refactor-python-venv-operator branch from b4ee6f7 to 46bf566 Compare July 10, 2020 11:59
@feluelle feluelle changed the title [WIP] Fix PythonVirtualenvOperator not working with Airflow context Fix PythonVirtualenvOperator not working with Airflow context Jul 10, 2020
@feluelle
Copy link
Member Author

Ready for another round @turbaszek @Fokko

@feluelle feluelle force-pushed the refactor-python-venv-operator branch from 46bf566 to fe43c31 Compare July 16, 2020 11:36
Copy link
Member

@turbaszek turbaszek left a comment

Choose a reason for hiding this comment

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

LGTM 🚀 @Fokko @kaxil ?

@feluelle
Copy link
Member Author

@maganaluis if you want to backport this fix to 1.10 branch let me know if I can help you with it. :)

I changed the description so it won't automatically close your issue and PR, because you want to target 1.10.x directly, correct?

- automatically add dill requirement if use_dill=True
- add howto docs
- refactor

Co-authored-by: Luis Magana <maganaluis@users.noreply.github.com>
@feluelle feluelle force-pushed the refactor-python-venv-operator branch from fe43c31 to f91bf8f Compare July 29, 2020 11:34
@feluelle
Copy link
Member Author

@Fokko @kaxil last chance after 🟢 :)

@feluelle feluelle merged commit 3f2eee1 into apache:master Jul 30, 2020
@maganaluis
Copy link
Contributor

@feluelle Thank you for adding this fix, not sure if this change will work in the 1.10 branch? We forked the repo into GitHub Enterprise and added the fix there, I'm good with closing the issue and PR. I have interest to move to Airflow 2.0 because the REST API so I'm looking forward to it.

@feluelle feluelle deleted the refactor-python-venv-operator branch August 13, 2020 13:27
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.

None yet

5 participants