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

"cannot import make_kwargs_callable" occurs when using airflow.providers.http.operators.http in 1.10.14. #15198

Closed
odajun opened this issue Apr 5, 2021 · 15 comments
Assignees
Labels
kind:bug This is a clearly a bug

Comments

@odajun
Copy link

odajun commented Apr 5, 2021

Apache Airflow version: 1.10.14

Kubernetes version (if you are using kubernetes) (use kubectl version): 1.19

Environment:

  • Cloud provider or hardware configuration: Openstack, 4 cores, 8GB RAM
  • OS (e.g. from /etc/os-release): /etc/centos-release CnetOS Linux release 7.9.2009
  • Kernel (e.g. uname -a): Linux
  • Install tools: Docker container (own build), Airflow is installed via
pip install apache-airflow[celery,hive,jdbc,mysql,redis,s3]==1.10.14
pip install hdfs,requests_kerberos,prometheus-client,apache-airflow-upgrade-check,sqlalchemy=1.3.23,apache-airflow-backport-providers-apache-hive,apache-airflow-backport-providers-http

What happened:

While preparing for the upgrade by following the steps in the documentation, I was faced with an error at the point where I implemented the following changes.

before : from airflow.operators.http_operator import SimpleHttpOperator
after  : from airflow.providers.http.operators.http import SimpleHttpOperator

error message

ERROR - cannot import name 'make_kwargs_callable' from 'airflow.utils.operators_helpers'

Anything else we need to know:

I suspect the following commit is required for 1.10.x version as well.
badd890#diff-d2d9b3696f554c1d2aee440470%5B%E2%80%A6%5D27dc193705e85b833b1f739271e81b14

There are other people who are encountering the same error.
https://apache-airflow.slack.com/archives/CCQB40SQJ/p1616504379010200

@odajun odajun added the kind:bug This is a clearly a bug label Apr 5, 2021
@boring-cyborg
Copy link

boring-cyborg bot commented Apr 5, 2021

Thanks for opening your first issue here! Be sure to follow the issue template!

@alexInhert
Copy link

This is an issue for me too. It's a blocker for migration to Airflow 2.
The backport HTTP provider is not working because of this.

make_kwargs_callable should be added to a release of Airflow 1.10 or the backport provider should not call this function.

@mik-laj
Copy link
Member

mik-laj commented Apr 5, 2021

Sorry, but we have already given up releasing new updates for backport providers.
https://lists.apache.org/thread.html/rb34212bd13c44aa61e793d0fef14f4ab9d9408939b1b98dc814cf274%40%3Cdev.airflow.apache.org%3E

This operator didn't have major breaking changes though, so you can keep the import to the airflow.operators package on Airflow 2.0.

@mik-laj mik-laj closed this as completed Apr 5, 2021
@kurtqq
Copy link
Contributor

kurtqq commented Apr 5, 2021

to jump in... i don't think it's issue of major or minor changes. It's issue of changes existed.
Upgrading a major release in production system used by so many different teams is challenge enough.
Please don't make things harder if you don't have to.

As suggested you can cherry pick the updated airflow/utils/operator_helpers.py #11922 to the next 1.10 release

Can you at least explore this before dismissing it completely?

@odajun
Copy link
Author

odajun commented Apr 6, 2021

@mik-laj Thank you for confirming this.

As for SimpleHttpOperator, I will try to upgrade it to 2.0.x without changing the import source.

 from airflow.operators.http_operator import SimpleHttpOperator

@mik-laj
Copy link
Member

mik-laj commented Apr 6, 2021

Can you at least explore this before dismissing it completely?

There have been drastic differences between the Airflow 2.0 and Airflow 1.10 releases, and cherry-picking changes are very problematic and time-consuming e.g. Airflow 1.10 still supports Python 2.7, so we need to add backward compatibility during cherry-picking. Besides, we haven't made any changes to operators for over a year now, because we started releasing backport providers, to make migrations to Airflow 2.0 a bit easier, but also so that project maintainers don't have to cherry-pick any operators related code. If we wanted to release a fix for this bug, we would therefore have to release it as backport providers.

The code from the backport providers has already been removed so we would have to start creating a separate branch that would only contain the one fix. Based on this one branch, we had to prepare a new release of backport package, start voting, and then release this package. It probably took a few man-hours and took a week or more if we wanted to do it according to all the procedures we have in our community. This is an enormous amount of work that is at odds with what was previously agreed upon by the community. None of the project maintainers will be interested in doing this.

If you need to fix this error, you can prepare the package yourself and publish it in your organization's private python repository. The branch to start from is legacy-backport-cutoff-point. The documentation and tools to build the backports are there.

@mik-laj
Copy link
Member

mik-laj commented Apr 6, 2021

If you are using Docker, you don't even need to prepare the pip package, but you can overwrite the required files in your image.

@potiuk
Copy link
Member

potiuk commented Apr 6, 2021

While we don't officially release the backports and as @mik-laj mentioned it is quite an overhead to release one.

However we have to take into account that the Http provider is one that is very commonly used, so we might try to think about doing a one time release to fix it as it might be a recurring problem for other users.

This is an exceptional case - it is not a bug to fix but our own backwards incompatibility introduced in November. The reason for the problem was that - unfortunately - the make_kwargs_callable was implemented via local imports, thus bypassing the protection we added to catch this kind of problem (it was indeed added in #11922 and it slipped under the radar of reviews). I can imagine few ways how it can be "quickly" implemented in an out-of-bands release for just http backport provider - without a lot of overhead. Happy to do it actually.

There are two things to do - one tactical for now for you and more strategic what we do for the provider,

  1. For the tactical part - in the meantime @kurtqq and @odajun and @alexInhert there are two mitigations:

a) you can simply stay with the old import when testing migration. It should work with deprecation warning as long as you do not make use of the make_kwargs_callable so far and you can change the imports after the migration even one-by-one.

b) You can install apache-airflow-backports-provider-http==2020.10.5 - it has no offending code, and it should work fine. There were mostly refactorings/standardizations/documentation changes since then in HTTP provider. So it should just work.

  1. For the strategi solution - I see two ways:

a) I think It will be rather easy to move the "make_kwargs_callable" to inside the http backport provider (this is only problem with http provider it seems) and release an out-of-band backport provider, creating a branch for it branching off the legacy-backport-cutoff-point. I think the biggest overhead there is not doing the changes but testing it. And here is my ask @odajun @kurtqq @alexInhert - if I release it later today or tomorrow as RC - would you volunteer to test it and report it back please ? That could help a lot with our decision of releasing it.

B) another option for strategic solution is simply to yank the releases after 2020.10.5 and leave only that version. Not nicest, but I think it should work and have no side effects.

@mik-laj @kaxil @ashb WDYT?

@potiuk potiuk reopened this Apr 6, 2021
@odajun
Copy link
Author

odajun commented Apr 6, 2021

@potiuk Thank you for your reply.

I think the biggest overhead there is not doing the changes but testing it. And here is my ask @odajun @kurtqq @alexInhert - if I release it later today or tomorrow as RC - would you volunteer to test it and report it back please ? That could help a lot with our decision of releasing it.

I will probably test in my environment and report.

It would be helpful if you could provide it in a form that allows installation using pip.

@kaxil
Copy link
Member

kaxil commented Apr 6, 2021

Yea we can probably follow (a) move the "make_kwargs_callable" to inside the http backport provider. And release one-off HTTP backport provider since it is very popular. But even

But like Jarek said this will need help from you'll in testing. And also note that the Backport Providers are only to help users transition to 2.0 and is provided with "best effort" and like Kamil said there is a bit of overhead to it. So please help us test this one and try to migrate to 2.0 ASAP, I know it is difficult for organization to upgrade to a major version but this one comes with huge performance gain. Airflow 1.10.x will reach EOL in June too.

@potiuk potiuk self-assigned this Apr 6, 2021
potiuk added a commit to potiuk/airflow that referenced this issue Apr 6, 2021
Fixes failing import problem of the http backport
provider with Airflow 1.10.* series.

A problem was introduced in apache#11922 which cause the http provider
to stop working (local import was not caught at the review time
and as local import it has not been caught by the test harness).

Since the http provider is defunct and is very popular, we
decided to release an out-of-band release of the http provider
even if backport providers reached end-of-life.

This PR copies implementation of make_kwargs_callable into http
backport provider to deal with the incompatibility.

Fixes: apache#15198
potiuk added a commit that referenced this issue Apr 6, 2021
Fixes failing import problem of the http backport
provider with Airflow 1.10.* series.

A problem was introduced in #11922 which cause the http provider
to stop working (local import was not caught at the review time
and as local import it has not been caught by the test harness).

Since the http provider is defunct and is very popular, we
decided to release an out-of-band release of the http provider
even if backport providers reached end-of-life.

This PR copies implementation of make_kwargs_callable into http
backport provider to deal with the incompatibility.

Fixes: #15198
@potiuk
Copy link
Member

potiuk commented Apr 6, 2021

Hey @odajun @kurtqq @alexInhert : I just released https://pypi.org/project/apache-airflow-backport-providers-http/2021.4.10rc2/ that should fix the problem.

Can you please run pip install apache-airflow-backport-providers-http==2021.4.10rc2 and let me know if the problem is fixed?

@kaxil
Copy link
Member

kaxil commented Apr 6, 2021

Please also add a non-binding vote if that works for you in https://lists.apache.org/thread.html/r5e870c593ca558c5e05417dc20b9dc55a48bbcb485483b63cd9b416c%40%3Cdev.airflow.apache.org%3E

@odajun
Copy link
Author

odajun commented Apr 8, 2021

@potiuk Thank you for your support.

After installing apache-airflow-backport-providers-http==2021.4.10rc2 in my environment, SimpleHttpOperator worked fine and I did not have any problems.

@potiuk
Copy link
Member

potiuk commented Apr 10, 2021

Released :) https://pypi.org/project/apache-airflow-backport-providers-http/2021.4.10/

Thanks again @odajun for both reporting and testing. We need more users like you.

@potiuk
Copy link
Member

potiuk commented Apr 10, 2021

And as @kaxil mentioned - if this was a blocker to migrate to 2.0, you are unblocked now! And please migrate ASAP. You won't regret!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug This is a clearly a bug
Projects
None yet
Development

No branches or pull requests

7 participants