You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/11/26 00:04:02 UTC

[GitHub] [airflow] potiuk opened a new pull request #12636: Upgrade to stable constraints

potiuk opened a new pull request #12636:
URL: https://github.com/apache/airflow/pull/12636


   Once #12635 gets merged, This PR should cleanly build with pip-check (:crossed_fingers: ). 
   
   This change upgrades setup.py to provide non-conflicting
   `pip check` valid set of constraints for CI image.
   
   <!--
   Thank you for contributing! Please make sure that your code changes
   are covered with tests. And in case of new features or big changes
   remember to adjust the documentation.
   
   Feel free to ping committers for the review!
   
   In case of existing issue, reference it using one of the following:
   
   closes: #ISSUE
   related: #ISSUE
   
   How to write a good git commit message:
   http://chris.beams.io/posts/git-commit/
   -->
   
   ---
   **^ Add meaningful description above**
   
   Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
   In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
   In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
   In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [airflow] turbaszek commented on a change in pull request #12636: Upgrade to stable constraints

Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #12636:
URL: https://github.com/apache/airflow/pull/12636#discussion_r531496346



##########
File path: setup.py
##########
@@ -243,11 +245,11 @@ def write_version(filename: str = os.path.join(*[my_dir, "airflow", "git_version
 flask_oauth = [
     'Flask-OAuthlib>=0.9.1,<0.9.6',  # Flask OAuthLib 0.9.6 requires Flask-Login 0.5.0 - breaks FAB
     'oauthlib!=2.0.3,!=2.0.4,!=2.0.5,<3.0.0,>=1.1.2',
-    'requests-oauthlib==1.1.0',
+    'requests-oauthlib<1.2.0',
 ]
 google = [
     'PyOpenSSL',
-    'google-ads>=4.0.0',
+    'google-ads>=4.0.0,<8.0.0',

Review comment:
       ```suggestion
       'google-ads>=3.3.0,<8.0.0',
   ```
   Should support 3.6 and it has v2 API




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [airflow] ashb commented on a change in pull request #12636: Upgrade to stable constraints

Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12636:
URL: https://github.com/apache/airflow/pull/12636#discussion_r531521780



##########
File path: setup.py
##########
@@ -243,11 +245,11 @@ def write_version(filename: str = os.path.join(*[my_dir, "airflow", "git_version
 flask_oauth = [
     'Flask-OAuthlib>=0.9.1,<0.9.6',  # Flask OAuthLib 0.9.6 requires Flask-Login 0.5.0 - breaks FAB
     'oauthlib!=2.0.3,!=2.0.4,!=2.0.5,<3.0.0,>=1.1.2',
-    'requests-oauthlib==1.1.0',
+    'requests-oauthlib<1.2.0',
 ]
 google = [
     'PyOpenSSL',
-    'google-ads>=4.0.0',
+    'google-ads>=4.0.0,<8.0.0',

Review comment:
       Ahahahahahaah.
   
   No, don't think so :( I'd like to be wrong here though.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [airflow] kaxil commented on pull request #12636: Upgrade to stable constraints

Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #12636:
URL: https://github.com/apache/airflow/pull/12636#issuecomment-734792353


   This is ready for review / merge right? Or are you waiting on something @potiuk ?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [airflow] potiuk edited a comment on pull request #12636: Upgrade to stable constraints

Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #12636:
URL: https://github.com/apache/airflow/pull/12636#issuecomment-734846939


   OK. I figured out EXACTLY what the problem with eager upgrade strategy was and wy we had conflicting requirements at the end of it.
   
   PIP first figures out the requirements for airflow itself and after that it adds extra.
   
   So when it first tries to get the requirements to be installed it installs them for the (`install_requires` in setup.cfg). And whatever is installed there, will not be downgraded even if a lower version is required by one of the extras. 
   
   This happened with `requests` - airflow "install_requires" figures that the best version to install is 2.25.0. But when snowlake and other deps have < 2.24.0 - requests will not be downgraded to 2.23.0, even if this produces version conflict. 
   
   That's why "eager" upgrade strategy upgraded requests to 2.25.0 -> because "install_requires" deps told it to do so. 
   
   The solution to all such conflicting requirements is simply to limit them in "install_requires" section :D. Then --upgrade-strategy eager strategy just works as expected.
   
   
   
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [airflow] potiuk edited a comment on pull request #12636: Upgrade to stable constraints

Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #12636:
URL: https://github.com/apache/airflow/pull/12636#issuecomment-734882158


   Now - there is only one import problem with azure, but it should be handled with #12188 - I disable those classes for now.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [airflow] github-actions[bot] commented on pull request #12636: Upgrade to stable constraints

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12636:
URL: https://github.com/apache/airflow/pull/12636#issuecomment-734900778


   [The Workflow run](https://github.com/apache/airflow/actions/runs/387412376) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [airflow] turbaszek commented on a change in pull request #12636: Upgrade to stable constraints

Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #12636:
URL: https://github.com/apache/airflow/pull/12636#discussion_r531507952



##########
File path: setup.py
##########
@@ -243,11 +245,11 @@ def write_version(filename: str = os.path.join(*[my_dir, "airflow", "git_version
 flask_oauth = [
     'Flask-OAuthlib>=0.9.1,<0.9.6',  # Flask OAuthLib 0.9.6 requires Flask-Login 0.5.0 - breaks FAB
     'oauthlib!=2.0.3,!=2.0.4,!=2.0.5,<3.0.0,>=1.1.2',
-    'requests-oauthlib==1.1.0',
+    'requests-oauthlib<1.2.0',
 ]
 google = [
     'PyOpenSSL',
-    'google-ads>=4.0.0',
+    'google-ads>=4.0.0,<8.0.0',

Review comment:
       > If so we'll need a more complex thing here including
   
   Is it necessary? Won't pip resolver use the right version for each python?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [airflow] github-actions[bot] commented on pull request #12636: Upgrade to stable constraints

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12636:
URL: https://github.com/apache/airflow/pull/12636#issuecomment-734288765


   [The Workflow run](https://github.com/apache/airflow/actions/runs/385270658) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [airflow] ashb commented on pull request #12636: Upgrade to stable constraints

Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #12636:
URL: https://github.com/apache/airflow/pull/12636#issuecomment-734847551


   Nice find.
   
   "Shocked" to discover that pip doesn't have an actual dependency resolver, but some random code instead :) 


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [airflow] potiuk commented on pull request #12636: Upgrade to stable constraints

Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #12636:
URL: https://github.com/apache/airflow/pull/12636#issuecomment-734235723


   I can split it further into two PRs, but I want to make it pass and see if indeed PIP check works @turbaszek !


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [airflow] potiuk commented on pull request #12636: Upgrade to stable constraints

Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #12636:
URL: https://github.com/apache/airflow/pull/12636#issuecomment-734955957


   OK. Just pushed the new constraints, So I merge this one and let it build. 


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [airflow] potiuk edited a comment on pull request #12636: Upgrade to stable constraints

Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #12636:
URL: https://github.com/apache/airflow/pull/12636#issuecomment-734812786


   > This is ready for review / merge right? Or are you waiting on something @potiuk ?
   
   I am looking at the result of it now and try to figure out the final version of it. I still see that even if we remove/reinstall the packages, pip will - for example install request == 2.25.1 even if we explicitly stated `requests< 2.24.0`. 
   
   The ``eager`` update will simply break the pip check now, I have to figure out how to prevent this.
   
   I already figured out good set of constraints for Py3.6 and I now have to get it also for 3.7 and 3.8. 
   
   Also - I temporarily switched to`constraints-pipcheck` branch and once we merge it and update the master constraints to match I will have to reverse it. So few more steps are needed.
   
   
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [airflow] potiuk commented on pull request #12636: Upgrade to stable constraints

Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #12636:
URL: https://github.com/apache/airflow/pull/12636#issuecomment-734852300


   > Nice find.
   > 
   > "Shocked" to discover that pip doesn't have an actual dependency resolver, but some random code instead :)
   
   I am also going to try the new resolver now, when I have good set of instal/extra requirements.
   
   The nice thing about it, is that likely we will not have to do any "tricks" with pipcheck constraints. Once I put the right requirements in install_requires, it seems that we will get "upgraded" pip-check compliant constraints 


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [airflow] ashb commented on a change in pull request #12636: Upgrade to stable constraints

Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12636:
URL: https://github.com/apache/airflow/pull/12636#discussion_r531503105



##########
File path: setup.py
##########
@@ -243,11 +245,11 @@ def write_version(filename: str = os.path.join(*[my_dir, "airflow", "git_version
 flask_oauth = [
     'Flask-OAuthlib>=0.9.1,<0.9.6',  # Flask OAuthLib 0.9.6 requires Flask-Login 0.5.0 - breaks FAB
     'oauthlib!=2.0.3,!=2.0.4,!=2.0.5,<3.0.0,>=1.1.2',
-    'requests-oauthlib==1.1.0',
+    'requests-oauthlib<1.2.0',
 ]
 google = [
     'PyOpenSSL',
-    'google-ads>=4.0.0',
+    'google-ads>=4.0.0,<8.0.0',

Review comment:
       Does 4.0 drop support for Py 3.6?
   
   If so we'll need a more complex thing here including `; python_version...`




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [airflow] potiuk commented on pull request #12636: Upgrade to stable constraints

Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #12636:
URL: https://github.com/apache/airflow/pull/12636#issuecomment-734882158


   Now - there is only one import problem with snowflake, but it should be an easy fix!


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [airflow] github-actions[bot] commented on pull request #12636: Upgrade to stable constraints

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12636:
URL: https://github.com/apache/airflow/pull/12636#issuecomment-734267048


   The PR needs to run all tests because it modifies core of Airflow! Please rebase it to latest master or ask committer to re-run it!


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [airflow] github-actions[bot] commented on pull request #12636: Upgrade to stable constraints

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12636:
URL: https://github.com/apache/airflow/pull/12636#issuecomment-734273650


   [The Workflow run](https://github.com/apache/airflow/actions/runs/385218698) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [airflow] potiuk commented on pull request #12636: Upgrade to stable constraints

Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #12636:
URL: https://github.com/apache/airflow/pull/12636#issuecomment-734812786


   > This is ready for review / merge right? Or are you waiting on something @potiuk ?
   
   I am looking at the result of it now and try to figure out the final version of it. I still see that even if we remove/install the packages, pip will - for example install request == 2.25.1 even if we explicitly stated `requests< 2.24.0`. I already figured out god set of constraints for Py3.6 and I now have to get it also for 3.7 and 3.8. 
   
   Also - I temporarily switched to`constraints-pipcheck` branch and once we merge it and update the master constraints to match I will have to reverse it. So few more steps are needed.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [airflow] potiuk commented on a change in pull request #12636: Upgrade to stable constraints

Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12636:
URL: https://github.com/apache/airflow/pull/12636#discussion_r531534845



##########
File path: setup.py
##########
@@ -243,11 +245,11 @@ def write_version(filename: str = os.path.join(*[my_dir, "airflow", "git_version
 flask_oauth = [
     'Flask-OAuthlib>=0.9.1,<0.9.6',  # Flask OAuthLib 0.9.6 requires Flask-Login 0.5.0 - breaks FAB
     'oauthlib!=2.0.3,!=2.0.4,!=2.0.5,<3.0.0,>=1.1.2',
-    'requests-oauthlib==1.1.0',
+    'requests-oauthlib<1.2.0',
 ]
 google = [
     'PyOpenSSL',
-    'google-ads>=4.0.0',
+    'google-ads>=4.0.0,<8.0.0',

Review comment:
       See here: 4.0.0 still supports 3.6.7+  https://pypi.org/project/google-ads/4.0.0/
   
   setup.py:
   ```
       python_requires='>=3.6',
   ```




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [airflow] potiuk edited a comment on pull request #12636: Upgrade to stable constraints

Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #12636:
URL: https://github.com/apache/airflow/pull/12636#issuecomment-734306269


   ```
   ERROR    airflow.models.dagbag.DagBag:dagbag.py:297 Failed to import: /opt/airflow/airflow/providers/google/ads/example_dags/example_ads.py
   Traceback (most recent call last):
     File "/opt/airflow/airflow/models/dagbag.py", line 294, in _load_modules_from_file
       loader.exec_module(new_module)
     File "<frozen importlib._bootstrap_external>", line 728, in exec_module
     File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
     File "/opt/airflow/airflow/providers/google/ads/example_dags/example_ads.py", line 24, in <module>
       from airflow.providers.google.ads.operators.ads import GoogleAdsListAccountsOperator
     File "/opt/airflow/airflow/providers/google/ads/operators/ads.py", line 24, in <module>
       from airflow.providers.google.ads.hooks.ads import GoogleAdsHook
     File "/opt/airflow/airflow/providers/google/ads/hooks/ads.py", line 25, in <module>
       from google.ads.google_ads.v2.types import GoogleAdsRow
   ModuleNotFoundError: No module named 'google.ads.google_ads.v2'
   ```
   
   Python 3.7 and 3.8 only !


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [airflow] ashb commented on a change in pull request #12636: Upgrade to stable constraints

Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12636:
URL: https://github.com/apache/airflow/pull/12636#discussion_r531545429



##########
File path: setup.py
##########
@@ -243,11 +245,11 @@ def write_version(filename: str = os.path.join(*[my_dir, "airflow", "git_version
 flask_oauth = [
     'Flask-OAuthlib>=0.9.1,<0.9.6',  # Flask OAuthLib 0.9.6 requires Flask-Login 0.5.0 - breaks FAB
     'oauthlib!=2.0.3,!=2.0.4,!=2.0.5,<3.0.0,>=1.1.2',
-    'requests-oauthlib==1.1.0',
+    'requests-oauthlib<1.2.0',
 ]
 google = [
     'PyOpenSSL',
-    'google-ads>=4.0.0',
+    'google-ads>=4.0.0,<8.0.0',

Review comment:
       Oh I'm confusing the case where we don't want the module for a python version at all. Sorry!




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [airflow] potiuk commented on pull request #12636: Upgrade to stable constraints

Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #12636:
URL: https://github.com/apache/airflow/pull/12636#issuecomment-734306269


   ERROR    airflow.models.dagbag.DagBag:dagbag.py:297 Failed to import: /opt/airflow/airflow/providers/google/ads/example_dags/example_ads.py
   Traceback (most recent call last):
     File "/opt/airflow/airflow/models/dagbag.py", line 294, in _load_modules_from_file
       loader.exec_module(new_module)
     File "<frozen importlib._bootstrap_external>", line 728, in exec_module
     File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
     File "/opt/airflow/airflow/providers/google/ads/example_dags/example_ads.py", line 24, in <module>
       from airflow.providers.google.ads.operators.ads import GoogleAdsListAccountsOperator
     File "/opt/airflow/airflow/providers/google/ads/operators/ads.py", line 24, in <module>
       from airflow.providers.google.ads.hooks.ads import GoogleAdsHook
     File "/opt/airflow/airflow/providers/google/ads/hooks/ads.py", line 25, in <module>
       from google.ads.google_ads.v2.types import GoogleAdsRow
   ModuleNotFoundError: No module named 'google.ads.google_ads.v2'


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [airflow] potiuk commented on pull request #12636: Upgrade to stable constraints

Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #12636:
URL: https://github.com/apache/airflow/pull/12636#issuecomment-734952381


   I am about to merge this one @kaxil @ash - if you want to take a  look. I xfailed the WasbHook tests and handle import failure for now. This should not stop teh WasbHook from working (it just cannot be tested when snowflake is installed) and @ephraimbuddy works on the #12188. After this is merged, we will restore proper tests/imports.
   
   I still have to make 'pip check' failure fail the build, but I will add separate PR for that once I merge this one and push updated constraints - I believe once the new constraints will be pushed, also production image will pass the 'pip check'.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [airflow] potiuk commented on pull request #12636: Upgrade to stable constraints

Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #12636:
URL: https://github.com/apache/airflow/pull/12636#issuecomment-734303627


   @turbaszek -> final (????) test.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [airflow] potiuk commented on pull request #12636: Upgrade to stable constraints

Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #12636:
URL: https://github.com/apache/airflow/pull/12636#issuecomment-734846939


   OK. I figured out EXACTLY what the problem with eager upgrade strategy was and wy we had conflicting requirements at the end of it.
   
   PIP first figures out the requirements for airflow itself and after that it adds extra.
   
   So when it first tries to get the requirements to be installed it installs them for the (`install_requires` in setup.cfg). And whatever is installed there, will not be downgraded even if a lower version is required by one of the extras. 
   
   This happened with `requests` - airflow "install_requires" figures that the best version to install is 2.25.0. But when snowlake and other deps have < 2.24.0 - requests will not be downgraded to 2.23.0, even if this produces version conflict. 
   
   That's why "eager" upgrade strategy upgraded requests to 2.25.0 -> because "install_requires" deps told it to do so. 
   
   The solution to all such conflicting requirements is simply to limit them in "install_requires" section :D. Then --upgrade-staregy eager strategy just works as expected.
   
   
   
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [airflow] potiuk commented on pull request #12636: Upgrade to stable constraints

Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #12636:
URL: https://github.com/apache/airflow/pull/12636#issuecomment-733999863


   @turbaszek - I moved your changes to "Wait for images" step. As soon as #12635 gets merged, this PR (with "upgrade to latest dependencies" label added) should (I believe) cleanly build and successfully run pip-check (at least for the CI Images). Once the constraints are updated, we can also likely enable prod image check.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [airflow] potiuk commented on pull request #12636: Upgrade to stable constraints

Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #12636:
URL: https://github.com/apache/airflow/pull/12636#issuecomment-734881892


   3.8: docker run --rm --entrypoint /bin/bash apache/airflow:master-python3.8-ci -c pip check
   No broken requirements found.
   3:7: docker run --rm --entrypoint /bin/bash apache/airflow:master-python3.7-ci -c pip check
   No broken requirements found.
   3.6: docker run --rm --entrypoint /bin/bash apache/airflow:master-python3.6-ci -c pip check
   No broken requirements found.
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [airflow] potiuk merged pull request #12636: Upgrade to stable constraints

Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #12636:
URL: https://github.com/apache/airflow/pull/12636


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [airflow] potiuk commented on pull request #12636: Upgrade to stable constraints

Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #12636:
URL: https://github.com/apache/airflow/pull/12636#issuecomment-734267849


   I still disable the result of the pip check - they will be printed but won't fail the build. Working on making sure all those are solved


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [airflow] github-actions[bot] commented on pull request #12636: Upgrade to stable constraints

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12636:
URL: https://github.com/apache/airflow/pull/12636#issuecomment-734887454


   [The Workflow run](https://github.com/apache/airflow/actions/runs/387358954) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [airflow] potiuk edited a comment on pull request #12636: Upgrade to stable constraints

Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #12636:
URL: https://github.com/apache/airflow/pull/12636#issuecomment-734306269


   ```
   ERROR    airflow.models.dagbag.DagBag:dagbag.py:297 Failed to import: /opt/airflow/airflow/providers/google/ads/example_dags/example_ads.py
   Traceback (most recent call last):
     File "/opt/airflow/airflow/models/dagbag.py", line 294, in _load_modules_from_file
       loader.exec_module(new_module)
     File "<frozen importlib._bootstrap_external>", line 728, in exec_module
     File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
     File "/opt/airflow/airflow/providers/google/ads/example_dags/example_ads.py", line 24, in <module>
       from airflow.providers.google.ads.operators.ads import GoogleAdsListAccountsOperator
     File "/opt/airflow/airflow/providers/google/ads/operators/ads.py", line 24, in <module>
       from airflow.providers.google.ads.hooks.ads import GoogleAdsHook
     File "/opt/airflow/airflow/providers/google/ads/hooks/ads.py", line 25, in <module>
       from google.ads.google_ads.v2.types import GoogleAdsRow
   ModuleNotFoundError: No module named 'google.ads.google_ads.v2'
   ```


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [airflow] potiuk edited a comment on pull request #12636: Upgrade to stable constraints

Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #12636:
URL: https://github.com/apache/airflow/pull/12636#issuecomment-734812786


   > This is ready for review / merge right? Or are you waiting on something @potiuk ?
   
   I am looking at the result of it now and try to figure out the final version of it. I still see that even if we remove/reinstall the packages, pip will - for example install request == 2.25.1 even if we explicitly stated `requests< 2.24.0`. I already figured out god set of constraints for Py3.6 and I now have to get it also for 3.7 and 3.8. 
   
   Also - I temporarily switched to`constraints-pipcheck` branch and once we merge it and update the master constraints to match I will have to reverse it. So few more steps are needed.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [airflow] potiuk commented on a change in pull request #12636: Upgrade to stable constraints

Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12636:
URL: https://github.com/apache/airflow/pull/12636#discussion_r531533792



##########
File path: setup.py
##########
@@ -243,11 +245,11 @@ def write_version(filename: str = os.path.join(*[my_dir, "airflow", "git_version
 flask_oauth = [
     'Flask-OAuthlib>=0.9.1,<0.9.6',  # Flask OAuthLib 0.9.6 requires Flask-Login 0.5.0 - breaks FAB
     'oauthlib!=2.0.3,!=2.0.4,!=2.0.5,<3.0.0,>=1.1.2',
-    'requests-oauthlib==1.1.0',
+    'requests-oauthlib<1.2.0',
 ]
 google = [
     'PyOpenSSL',
-    'google-ads>=4.0.0',
+    'google-ads>=4.0.0,<8.0.0',

Review comment:
       The 8.0.0 limitation is needed, because the Ads operators are using v2 version of the API and support for those have been removed in 8.0.0 (very recently - in November). The operator should be updated (but not now).
   
   The 4.0.0 still supports 3.6.7+. Support for 3.7 has been removed in one of the 4.1.* versions. PYPI solves it nicely on their own as those non-3.7 packages have the limitation to Python>=3.7 set and pypi will not go beyond that version in 3.6. 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [airflow] ashb commented on a change in pull request #12636: Upgrade to stable constraints

Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12636:
URL: https://github.com/apache/airflow/pull/12636#discussion_r531521780



##########
File path: setup.py
##########
@@ -243,11 +245,11 @@ def write_version(filename: str = os.path.join(*[my_dir, "airflow", "git_version
 flask_oauth = [
     'Flask-OAuthlib>=0.9.1,<0.9.6',  # Flask OAuthLib 0.9.6 requires Flask-Login 0.5.0 - breaks FAB
     'oauthlib!=2.0.3,!=2.0.4,!=2.0.5,<3.0.0,>=1.1.2',
-    'requests-oauthlib==1.1.0',
+    'requests-oauthlib<1.2.0',
 ]
 google = [
     'PyOpenSSL',
-    'google-ads>=4.0.0',
+    'google-ads>=4.0.0,<8.0.0',

Review comment:
       Ahahahahahaah.
   
   No, don't think so :(




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [airflow] potiuk edited a comment on pull request #12636: Upgrade to stable constraints

Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #12636:
URL: https://github.com/apache/airflow/pull/12636#issuecomment-734952381


   I am about to merge this one @kaxil @ash - if you want to take a  look. I xfailed the WasbHook tests and handle import failure for now. This should not stop the WasbHook from working (it just cannot be tested when snowflake is installed) and @ephraimbuddy works on the #12188. After this is merged, we will restore proper tests/imports.
   
   I still have to make 'pip check' failure fail the build, but I will add separate PR for that once I merge this one and push updated constraints - I believe once the new constraints will be pushed, also production image will pass the 'pip check'.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [airflow] potiuk edited a comment on pull request #12636: Upgrade to stable constraints

Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #12636:
URL: https://github.com/apache/airflow/pull/12636#issuecomment-734812786


   > This is ready for review / merge right? Or are you waiting on something @potiuk ?
   
   I am looking at the result of it now and try to figure out the final version of it. I still see that even if we remove/reinstall the packages, pip will - for example install requests == 2.25.1 even if we explicitly stated `requests< 2.24.0`. 
   
   The ``eager`` update will simply break the pip check now, I have to figure out how to prevent this.
   
   I already figured out good set of constraints for Py3.6 and I now have to get it also for 3.7 and 3.8. 
   
   Also - I temporarily switched to`constraints-pipcheck` branch and once we merge it and update the master constraints to match I will have to reverse it. So few more steps are needed.
   
   
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org