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 2019/12/27 18:54:09 UTC

[GitHub] [airflow] nuclearpinguin opened a new pull request #6929: [AIRFLOW-6373] Make airflow/utils pylint compatible

nuclearpinguin opened a new pull request #6929: [AIRFLOW-6373] Make airflow/utils pylint compatible
URL: https://github.com/apache/airflow/pull/6929

Make sure you have checked _all_ steps below.

### Jira

- [ ] My PR addresses the following [Airflow Jira](https://issues.apache.org/jira/browse/AIRFLOW/) issues and references them in the PR title. For example, "\[AIRFLOW-XXX\] My Airflow PR"
- https://issues.apache.org/jira/browse/AIRFLOW-6367

### Description

- [ ] Here are some details about my PR, including screenshots of any UI changes:
This PR make `airflow/utils` pylint compatible.

### Tests

- [ ] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:

### Commits

- [ ] My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"

### Documentation

- [ ] In case of new functionality, my PR adds documentation that describes how to use it.
- All the public functions and the classes in the PR contain docstrings that explain what it does
- If you implement backwards incompatible changes, please leave a note in the [Updating.md](https://github.com/apache/airflow/blob/master/UPDATING.md) so we can assign it to a appropriate release

----------------------------------------------------------------
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

With regards,
Apache Git Services

[GitHub] [airflow] potiuk commented on issue #6929: [AIRFLOW-6373] Make airflow/utils pylint compatible

Posted by GitBox <gi...@apache.org>.

potiuk commented on issue #6929: [AIRFLOW-6373] Make airflow/utils pylint compatible
URL: https://github.com/apache/airflow/pull/6929#issuecomment-569417232
 
 
   I think pylint is VERY right about those cyclic imports.
   
   For me the problem is the db module itself.. It's typical manifestation of cyclical imports where you thing various methods are coupled and put them together for a wrong reason. 
   If you look what db module now is: "let's use it for any kind of db  operations". 
   
   But in fact there are two different purpose it serves now:
   * provide methods that are used everywhere for DB operations (including models) - `provide_session` is such method. Those methods are USED by the models.
   * provide methods that are used to group together db methods for multiple models (those methods are USING models).
   
   Putting those two types of methods introduces coupling between them unnecessarily. That's exactly what is causing the cyclic imports. 
   
   We should split the db module into two independent modules. I am going to do just that shortly.

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [airflow] potiuk commented on issue #6929: [AIRFLOW-6373] Make airflow/utils pylint compatible

Posted by GitBox <gi...@apache.org>.

potiuk commented on issue #6929: [AIRFLOW-6373] Make airflow/utils pylint compatible
URL: https://github.com/apache/airflow/pull/6929#issuecomment-569416440
 
 
   Yep. I have not finished yet. In the last one I brought back the key_validation back to helpers (now we can do it as the BaseOperator is not used there).

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [airflow] potiuk commented on issue #6929: [AIRFLOW-6373] Make airflow/utils pylint compatible

Posted by GitBox <gi...@apache.org>.

potiuk commented on issue #6929: [AIRFLOW-6373] Make airflow/utils pylint compatible
URL: https://github.com/apache/airflow/pull/6929#issuecomment-570099308
 
 
   Still a few failures... But we are getting there! 

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [airflow] nuclearpinguin commented on a change in pull request #6929: [AIRFLOW-6373][WIP] Make airflow/utils pylint compatible

Posted by GitBox <gi...@apache.org>.

nuclearpinguin commented on a change in pull request #6929: [AIRFLOW-6373][WIP] Make airflow/utils pylint compatible
URL: https://github.com/apache/airflow/pull/6929#discussion_r361810450
 
 

 ##########
 File path: airflow/utils/tests.py
 ##########
 @@ -28,19 +27,11 @@
 from airflow.utils.decorators import apply_defaults
 
 
-def skipUnlessImported(module, obj):
 
 Review comment:
   Was never used in project.

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [airflow] nuclearpinguin commented on issue #6929: [AIRFLOW-6373][WIP] Make airflow/utils pylint compatible

Posted by GitBox <gi...@apache.org>.

nuclearpinguin commented on issue #6929: [AIRFLOW-6373][WIP] Make airflow/utils pylint compatible
URL: https://github.com/apache/airflow/pull/6929#issuecomment-569427011
 
 
   After a discussion I extracted session related stuff to a new `session` module #6938 . I will rebase after there merge.
   
   Thanks @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


With regards,
Apache Git Services

[GitHub] [airflow] potiuk merged pull request #6929: [AIRFLOW-6373] Make airflow/utils pylint compatible

Posted by GitBox <gi...@apache.org>.

potiuk merged pull request #6929: [AIRFLOW-6373] Make airflow/utils pylint compatible
URL: https://github.com/apache/airflow/pull/6929
 
 
   

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [airflow] potiuk commented on issue #6929: [AIRFLOW-6373] Make airflow/utils pylint compatible

Posted by GitBox <gi...@apache.org>.

potiuk commented on issue #6929: [AIRFLOW-6373] Make airflow/utils pylint compatible
URL: https://github.com/apache/airflow/pull/6929#issuecomment-569405772
 
 
   The reasons are .... cyclic imports :)
   
   The problem is that sometimes we have too many grouped classes in one module where it is really not supposed to be groupped. For example the baseoperator->helpers cyclic import is caused by "chain" method in helpers. BaseOperator uses helpers module and helpers.chain() method uses BaseOperator. This all can be resolved by careful untangling of modules. Sometimes what helps is putting together things that are actually coupled by historically are in separate modules. I have some experience with that and will try to add a few commits on top of yours to solve 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


With regards,
Apache Git Services

[GitHub] [airflow] potiuk commented on issue #6929: [AIRFLOW-6373] Make airflow/utils pylint compatible

Posted by GitBox <gi...@apache.org>.

potiuk commented on issue #6929: [AIRFLOW-6373] Make airflow/utils pylint compatible
URL: https://github.com/apache/airflow/pull/6929#issuecomment-569418084
 
 
   And merge_conn method should be moved to Connection.

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [airflow] ashb commented on issue #6929: [AIRFLOW-6373][WIP] Make airflow/utils pylint compatible

Posted by GitBox <gi...@apache.org>.

ashb commented on issue #6929: [AIRFLOW-6373][WIP] Make airflow/utils pylint compatible
URL: https://github.com/apache/airflow/pull/6929#issuecomment-569429897
 
 
   Can you try to keep PRs to a single thing please? This is at least three? Or five! prs - white space changes, lots of doc comments, unrelated pylint changes, touching a file that was otherwise not changed etc.

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [airflow] nuclearpinguin commented on issue #6929: [AIRFLOW-6373] Make airflow/utils pylint compatible

Posted by GitBox <gi...@apache.org>.

nuclearpinguin commented on issue #6929: [AIRFLOW-6373] Make airflow/utils pylint compatible
URL: https://github.com/apache/airflow/pull/6929#issuecomment-569418494
 
 
   I just removed inner imports in `airflow.db` result: no cyclic imports.  There is a difference between real cyclic import which will be reported by Python and those "probably a cyclic import" raised by pylint.
   
   However, I agree that it's a good idea to remove session stuff to some `session` module.
   
   

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [airflow] potiuk commented on issue #6929: [AIRFLOW-6373] Make airflow/utils pylint compatible

Posted by GitBox <gi...@apache.org>.

potiuk commented on issue #6929: [AIRFLOW-6373] Make airflow/utils pylint compatible
URL: https://github.com/apache/airflow/pull/6929#issuecomment-569407567
 
 
   @nuclearpinguin ^^ Here you can see  how it helped to add type annotation in the BaseOperator-related functions - now BaseOperator can be top-level and can be used in method signature.
   
   The next thing will be to move those two BaseOperator methods to BaseOperator class. They actually belong there  - helpers are methods that can be imported in many places and should be fairly "standalone" so actually I think making helper dependent on BaseOperator is bad idea.

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [airflow] codecov-io edited a comment on issue #6929: [AIRFLOW-6373] Make airflow/utils pylint compatible

Posted by GitBox <gi...@apache.org>.

codecov-io edited a comment on issue #6929: [AIRFLOW-6373] Make airflow/utils pylint compatible
URL: https://github.com/apache/airflow/pull/6929#issuecomment-570194787
 
 
   # [Codecov](https://codecov.io/gh/apache/airflow/pull/6929?src=pr&el=h1) Report
   > Merging [#6929](https://codecov.io/gh/apache/airflow/pull/6929?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/be812bd660fac4621a25f3269fded8d4e03b0023?src=pr&el=desc) will **decrease** coverage by `0.28%`.
   > The diff coverage is `78.9%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/airflow/pull/6929/graphs/tree.svg?width=650&token=WdLKlKHOAU&height=150&src=pr)](https://codecov.io/gh/apache/airflow/pull/6929?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #6929      +/-   ##
   ==========================================
   - Coverage   84.85%   84.56%   -0.29%     
   ==========================================
     Files         679      679              
     Lines       38536    38542       +6     
   ==========================================
   - Hits        32698    32593     -105     
   - Misses       5838     5949     +111
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/airflow/pull/6929?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [airflow/utils/weight\_rule.py](https://codecov.io/gh/apache/airflow/pull/6929/diff?src=pr&el=tree#diff-YWlyZmxvdy91dGlscy93ZWlnaHRfcnVsZS5weQ==) | `100% <ø> (ø)` | :arrow_up: |
   | [airflow/utils/state.py](https://codecov.io/gh/apache/airflow/pull/6929/diff?src=pr&el=tree#diff-YWlyZmxvdy91dGlscy9zdGF0ZS5weQ==) | `96.29% <ø> (ø)` | :arrow_up: |
   | [airflow/utils/operator\_resources.py](https://codecov.io/gh/apache/airflow/pull/6929/diff?src=pr&el=tree#diff-YWlyZmxvdy91dGlscy9vcGVyYXRvcl9yZXNvdXJjZXMucHk=) | `84.78% <ø> (ø)` | :arrow_up: |
   | [airflow/utils/log/gcs\_task\_handler.py](https://codecov.io/gh/apache/airflow/pull/6929/diff?src=pr&el=tree#diff-YWlyZmxvdy91dGlscy9sb2cvZ2NzX3Rhc2tfaGFuZGxlci5weQ==) | `0% <0%> (ø)` | :arrow_up: |
   | [airflow/utils/log/wasb\_task\_handler.py](https://codecov.io/gh/apache/airflow/pull/6929/diff?src=pr&el=tree#diff-YWlyZmxvdy91dGlscy9sb2cvd2FzYl90YXNrX2hhbmRsZXIucHk=) | `42.46% <0%> (ø)` | :arrow_up: |
   | [airflow/utils/log/file\_processor\_handler.py](https://codecov.io/gh/apache/airflow/pull/6929/diff?src=pr&el=tree#diff-YWlyZmxvdy91dGlscy9sb2cvZmlsZV9wcm9jZXNzb3JfaGFuZGxlci5weQ==) | `85.33% <100%> (ø)` | :arrow_up: |
   | [airflow/utils/sqlalchemy.py](https://codecov.io/gh/apache/airflow/pull/6929/diff?src=pr&el=tree#diff-YWlyZmxvdy91dGlscy9zcWxhbGNoZW15LnB5) | `96.66% <100%> (+0.05%)` | :arrow_up: |
   | [airflow/utils/log/logging\_mixin.py](https://codecov.io/gh/apache/airflow/pull/6929/diff?src=pr&el=tree#diff-YWlyZmxvdy91dGlscy9sb2cvbG9nZ2luZ19taXhpbi5weQ==) | `95.38% <100%> (ø)` | :arrow_up: |
   | [airflow/utils/net.py](https://codecov.io/gh/apache/airflow/pull/6929/diff?src=pr&el=tree#diff-YWlyZmxvdy91dGlscy9uZXQucHk=) | `81.25% <100%> (ø)` | :arrow_up: |
   | [airflow/utils/email.py](https://codecov.io/gh/apache/airflow/pull/6929/diff?src=pr&el=tree#diff-YWlyZmxvdy91dGlscy9lbWFpbC5weQ==) | `100% <100%> (ø)` | :arrow_up: |
   | ... and [19 more](https://codecov.io/gh/apache/airflow/pull/6929/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/6929?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/6929?src=pr&el=footer). Last update [be812bd...dbe0cfa](https://codecov.io/gh/apache/airflow/pull/6929?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [airflow] nuclearpinguin edited a comment on issue #6929: [AIRFLOW-6373][WIP] Make airflow/utils pylint compatible

Posted by GitBox <gi...@apache.org>.

nuclearpinguin edited a comment on issue #6929: [AIRFLOW-6373][WIP] Make airflow/utils pylint compatible
URL: https://github.com/apache/airflow/pull/6929#issuecomment-569427011
 
 
   After a discussion I extracted session related stuff to a new `session` module #6938 . I will rebase after the merge.
   
   Thanks @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


With regards,
Apache Git Services

[GitHub] [airflow] ashb commented on issue #6929: [AIRFLOW-6373][WIP] Make airflow/utils pylint compatible

Posted by GitBox <gi...@apache.org>.

ashb commented on issue #6929: [AIRFLOW-6373][WIP] Make airflow/utils pylint compatible
URL: https://github.com/apache/airflow/pull/6929#issuecomment-569430452
 
 
   Wait did this get renamed? When I first opened it it was about just moving something around I thought

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [airflow] potiuk commented on issue #6929: [AIRFLOW-6373] Make airflow/utils pylint compatible

Posted by GitBox <gi...@apache.org>.

potiuk commented on issue #6929: [AIRFLOW-6373] Make airflow/utils pylint compatible
URL: https://github.com/apache/airflow/pull/6929#issuecomment-569406466
 
 
   In fact the baseoperator -> helpers root cause is that baseoperator uses helpers.validate_key as the only method from helpers. There are other methods in helpers that use BaseOperator and it makes sense to import BaseOperator there at top level and move validate_key somewhere else

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [airflow] nuclearpinguin edited a comment on issue #6929: [AIRFLOW-6373][WIP] Make airflow/utils pylint compatible

Posted by GitBox <gi...@apache.org>.

nuclearpinguin edited a comment on issue #6929: [AIRFLOW-6373][WIP] Make airflow/utils pylint compatible
URL: https://github.com/apache/airflow/pull/6929#issuecomment-569431651
 
 
   After merging #6938 I propose to include in this PR only the first commit. I agree with @ashb that all additional changes should be done in separate one, before or after this PR.
   

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [airflow] codecov-io edited a comment on issue #6929: [AIRFLOW-6373] Make airflow/utils pylint compatible

Posted by GitBox <gi...@apache.org>.

codecov-io edited a comment on issue #6929: [AIRFLOW-6373] Make airflow/utils pylint compatible
URL: https://github.com/apache/airflow/pull/6929#issuecomment-570194787
 
 
   # [Codecov](https://codecov.io/gh/apache/airflow/pull/6929?src=pr&el=h1) Report
   > Merging [#6929](https://codecov.io/gh/apache/airflow/pull/6929?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/be812bd660fac4621a25f3269fded8d4e03b0023?src=pr&el=desc) will **decrease** coverage by `0.28%`.
   > The diff coverage is `78.9%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/airflow/pull/6929/graphs/tree.svg?width=650&token=WdLKlKHOAU&height=150&src=pr)](https://codecov.io/gh/apache/airflow/pull/6929?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #6929      +/-   ##
   ==========================================
   - Coverage   84.85%   84.56%   -0.29%     
   ==========================================
     Files         679      679              
     Lines       38536    38542       +6     
   ==========================================
   - Hits        32698    32593     -105     
   - Misses       5838     5949     +111
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/airflow/pull/6929?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [airflow/utils/weight\_rule.py](https://codecov.io/gh/apache/airflow/pull/6929/diff?src=pr&el=tree#diff-YWlyZmxvdy91dGlscy93ZWlnaHRfcnVsZS5weQ==) | `100% <ø> (ø)` | :arrow_up: |
   | [airflow/utils/state.py](https://codecov.io/gh/apache/airflow/pull/6929/diff?src=pr&el=tree#diff-YWlyZmxvdy91dGlscy9zdGF0ZS5weQ==) | `96.29% <ø> (ø)` | :arrow_up: |
   | [airflow/utils/operator\_resources.py](https://codecov.io/gh/apache/airflow/pull/6929/diff?src=pr&el=tree#diff-YWlyZmxvdy91dGlscy9vcGVyYXRvcl9yZXNvdXJjZXMucHk=) | `84.78% <ø> (ø)` | :arrow_up: |
   | [airflow/utils/log/gcs\_task\_handler.py](https://codecov.io/gh/apache/airflow/pull/6929/diff?src=pr&el=tree#diff-YWlyZmxvdy91dGlscy9sb2cvZ2NzX3Rhc2tfaGFuZGxlci5weQ==) | `0% <0%> (ø)` | :arrow_up: |
   | [airflow/utils/log/wasb\_task\_handler.py](https://codecov.io/gh/apache/airflow/pull/6929/diff?src=pr&el=tree#diff-YWlyZmxvdy91dGlscy9sb2cvd2FzYl90YXNrX2hhbmRsZXIucHk=) | `42.46% <0%> (ø)` | :arrow_up: |
   | [airflow/utils/log/file\_processor\_handler.py](https://codecov.io/gh/apache/airflow/pull/6929/diff?src=pr&el=tree#diff-YWlyZmxvdy91dGlscy9sb2cvZmlsZV9wcm9jZXNzb3JfaGFuZGxlci5weQ==) | `85.33% <100%> (ø)` | :arrow_up: |
   | [airflow/utils/sqlalchemy.py](https://codecov.io/gh/apache/airflow/pull/6929/diff?src=pr&el=tree#diff-YWlyZmxvdy91dGlscy9zcWxhbGNoZW15LnB5) | `96.66% <100%> (+0.05%)` | :arrow_up: |
   | [airflow/utils/log/logging\_mixin.py](https://codecov.io/gh/apache/airflow/pull/6929/diff?src=pr&el=tree#diff-YWlyZmxvdy91dGlscy9sb2cvbG9nZ2luZ19taXhpbi5weQ==) | `95.38% <100%> (ø)` | :arrow_up: |
   | [airflow/utils/net.py](https://codecov.io/gh/apache/airflow/pull/6929/diff?src=pr&el=tree#diff-YWlyZmxvdy91dGlscy9uZXQucHk=) | `81.25% <100%> (ø)` | :arrow_up: |
   | [airflow/utils/email.py](https://codecov.io/gh/apache/airflow/pull/6929/diff?src=pr&el=tree#diff-YWlyZmxvdy91dGlscy9lbWFpbC5weQ==) | `100% <100%> (ø)` | :arrow_up: |
   | ... and [19 more](https://codecov.io/gh/apache/airflow/pull/6929/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/6929?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/6929?src=pr&el=footer). Last update [be812bd...dbe0cfa](https://codecov.io/gh/apache/airflow/pull/6929?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [airflow] potiuk commented on issue #6929: [AIRFLOW-6373] Make airflow/utils pylint compatible

Posted by GitBox <gi...@apache.org>.

potiuk commented on issue #6929: [AIRFLOW-6373] Make airflow/utils pylint compatible
URL: https://github.com/apache/airflow/pull/6929#issuecomment-570005034
 
 
   Unit tests failing  :(

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [airflow] nuclearpinguin commented on issue #6929: [AIRFLOW-6373][WIP] Make airflow/utils pylint compatible

Posted by GitBox <gi...@apache.org>.

nuclearpinguin commented on issue #6929: [AIRFLOW-6373][WIP] Make airflow/utils pylint compatible
URL: https://github.com/apache/airflow/pull/6929#issuecomment-569431651
 
 
   After merging #6938 I propose to keep include in this PR only the first commit. I agree with @ashb that all additional changes should be done in separate one, before or after this PR.
   

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [airflow] potiuk commented on issue #6929: [AIRFLOW-6373] Make airflow/utils pylint compatible

Posted by GitBox <gi...@apache.org>.

potiuk commented on issue #6929: [AIRFLOW-6373] Make airflow/utils pylint compatible
URL: https://github.com/apache/airflow/pull/6929#issuecomment-569406960
 
 
   @nuclearpinguin See ^^ here I solved the baseoperator -> helpers import. That actually will enable to add a few more BaseOperators (typehints) in helpers and move BaseOperator import to top level import (next commit that follows)

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [airflow] nuclearpinguin commented on issue #6929: [AIRFLOW-6373] Make airflow/utils pylint compatible

Posted by GitBox <gi...@apache.org>.

nuclearpinguin commented on issue #6929: [AIRFLOW-6373] Make airflow/utils pylint compatible
URL: https://github.com/apache/airflow/pull/6929#issuecomment-569334221
 
 
   Or even more cyclic imports?
   ```
   ************* Module airflow.cli.commands.config_command
   1652airflow/cli/commands/config_command.py:1:0: R0401: Cyclic import (airflow.models -> airflow.models.taskreschedule -> airflow.utils.db) (cyclic-import)
   1653airflow/cli/commands/config_command.py:1:0: R0401: Cyclic import (airflow.models.baseoperator -> airflow.utils.db -> airflow.models.serialized_dag -> airflow.serialization.serialized_objects) (cyclic-import)
   1654airflow/cli/commands/config_command.py:1:0: R0401: Cyclic import (airflow.models.baseoperator -> airflow.utils.helpers) (cyclic-import)
   1655airflow/cli/commands/config_command.py:1:0: R0401: Cyclic import (airflow.models -> airflow.models.baseoperator -> airflow.utils.db) (cyclic-import)
   1656airflow/cli/commands/config_command.py:1:0: R0401: Cyclic import (airflow.models -> airflow.models.taskreschedule -> airflow.utils.db -> airflow.models.serialized_dag -> airflow.serialization.serialized_objects) (cyclic-import)
   1657airflow/cli/commands/config_command.py:1:0: R0401: Cyclic import (airflow.models.serialized_dag -> airflow.utils.db) (cyclic-import)
   ```

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [airflow] potiuk commented on issue #6929: [AIRFLOW-6373] Make airflow/utils pylint compatible

Posted by GitBox <gi...@apache.org>.

potiuk commented on issue #6929: [AIRFLOW-6373] Make airflow/utils pylint compatible
URL: https://github.com/apache/airflow/pull/6929#issuecomment-569409695
 
 
   And now even better ^^ the two methods from helpers which are tightly coupled with BaseOperator are now static methods in BaseOperator itself.

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [airflow] potiuk commented on issue #6929: [AIRFLOW-6373][WIP] Make airflow/utils pylint compatible

Posted by GitBox <gi...@apache.org>.

potiuk commented on issue #6929: [AIRFLOW-6373][WIP] Make airflow/utils pylint compatible
URL: https://github.com/apache/airflow/pull/6929#issuecomment-569441267
 
 
   Yep. I will move my commits as next step after #6938 . I will do it shortly.

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [airflow] nuclearpinguin commented on a change in pull request #6929: [AIRFLOW-6373] Make airflow/utils pylint compatible

Posted by GitBox <gi...@apache.org>.

nuclearpinguin commented on a change in pull request #6929: [AIRFLOW-6373] Make airflow/utils pylint compatible
URL: https://github.com/apache/airflow/pull/6929#discussion_r362338257
 
 

 ##########
 File path: airflow/task/task_runner/base_task_runner.py
 ##########
 @@ -63,9 +63,7 @@ def __init__(self, local_task_job):
             # want to have to specify them in the sudo call - they would show
             # up in `ps` that way! And run commands now, as the other user
             # might not be able to run the cmds to get credentials
-            cfg_path = tmp_configuration_copy(chmod=0o600,
-                                              include_env=True,
-                                              include_cmds=True)
 
 Review comment:
   Those args were unused.

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [airflow] nuclearpinguin commented on issue #6929: [AIRFLOW-6373] Make airflow/utils pylint compatible

Posted by GitBox <gi...@apache.org>.

nuclearpinguin commented on issue #6929: [AIRFLOW-6373] Make airflow/utils pylint compatible
URL: https://github.com/apache/airflow/pull/6929#issuecomment-569415661
 
 
   There are still cyclic imports and I start to doubt in pylint messages, for example:
   ```
   airflow/utils/log/es_task_handler.py:1:0: R0401: Cyclic import (airflow.models -> airflow.models.taskreschedule -> airflow.utils.db) (cyclic-import)
   ```
   and I don't see how it is possible as `airflow.utils.db` do not import anything from `airflow.models` unless you count the import in functions... more info
   https://github.com/PyCQA/pylint/issues/850

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [airflow] mik-laj commented on a change in pull request #6929: [AIRFLOW-6373][WIP] Make airflow/utils pylint compatible

Posted by GitBox <gi...@apache.org>.

mik-laj commented on a change in pull request #6929: [AIRFLOW-6373][WIP] Make airflow/utils pylint compatible
URL: https://github.com/apache/airflow/pull/6929#discussion_r361815733
 
 

 ##########
 File path: airflow/utils/configuration.py
 ##########
 @@ -24,7 +24,11 @@
 from airflow.configuration import conf
 
 
-def tmp_configuration_copy(chmod=0o600, include_env=True, include_cmds=True):
+def tmp_configuration_copy(  # pylint: disable=unused-argument
+    chmod=0o600,
+    include_env=True,
 
 Review comment:
   Should we remove unused argument?

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [airflow] potiuk commented on issue #6929: [AIRFLOW-6373] Make airflow/utils pylint compatible

Posted by GitBox <gi...@apache.org>.

potiuk commented on issue #6929: [AIRFLOW-6373] Make airflow/utils pylint compatible
URL: https://github.com/apache/airflow/pull/6929#issuecomment-569419734
 
 
   That's the point. Those ARE real (not probable) cyclic imports. And of course Pylint reports more/different kinds cyclic imports than python parser - otherwise we would not even get there at the first place because pylint would fail parsing the code. Pylint deliberately reports the cycles that developers have hidden by using local imports. But they are only hidden - they are logical cycles. 
   
   They are still there but in order to stop python parser from failing at import time developer deferred resolving imports to later phase when method is executed. But real coupling between the MODULE containing the method and imported class remains. It's the logical cycle that is there: resetdb() (from db)  method uses 'models' which are using provide_session() (again from db). This is a real cycle. And it does not need to be ther

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [airflow] codecov-io commented on issue #6929: [AIRFLOW-6373] Make airflow/utils pylint compatible

Posted by GitBox <gi...@apache.org>.

codecov-io commented on issue #6929: [AIRFLOW-6373] Make airflow/utils pylint compatible
URL: https://github.com/apache/airflow/pull/6929#issuecomment-570194787
 
 
   # [Codecov](https://codecov.io/gh/apache/airflow/pull/6929?src=pr&el=h1) Report
   > Merging [#6929](https://codecov.io/gh/apache/airflow/pull/6929?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/be812bd660fac4621a25f3269fded8d4e03b0023?src=pr&el=desc) will **decrease** coverage by `0.28%`.
   > The diff coverage is `78.9%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/airflow/pull/6929/graphs/tree.svg?width=650&token=WdLKlKHOAU&height=150&src=pr)](https://codecov.io/gh/apache/airflow/pull/6929?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #6929      +/-   ##
   ==========================================
   - Coverage   84.85%   84.56%   -0.29%     
   ==========================================
     Files         679      679              
     Lines       38536    38542       +6     
   ==========================================
   - Hits        32698    32593     -105     
   - Misses       5838     5949     +111
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/airflow/pull/6929?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [airflow/utils/weight\_rule.py](https://codecov.io/gh/apache/airflow/pull/6929/diff?src=pr&el=tree#diff-YWlyZmxvdy91dGlscy93ZWlnaHRfcnVsZS5weQ==) | `100% <ø> (ø)` | :arrow_up: |
   | [airflow/utils/state.py](https://codecov.io/gh/apache/airflow/pull/6929/diff?src=pr&el=tree#diff-YWlyZmxvdy91dGlscy9zdGF0ZS5weQ==) | `96.29% <ø> (ø)` | :arrow_up: |
   | [airflow/utils/operator\_resources.py](https://codecov.io/gh/apache/airflow/pull/6929/diff?src=pr&el=tree#diff-YWlyZmxvdy91dGlscy9vcGVyYXRvcl9yZXNvdXJjZXMucHk=) | `84.78% <ø> (ø)` | :arrow_up: |
   | [airflow/utils/log/gcs\_task\_handler.py](https://codecov.io/gh/apache/airflow/pull/6929/diff?src=pr&el=tree#diff-YWlyZmxvdy91dGlscy9sb2cvZ2NzX3Rhc2tfaGFuZGxlci5weQ==) | `0% <0%> (ø)` | :arrow_up: |
   | [airflow/utils/log/wasb\_task\_handler.py](https://codecov.io/gh/apache/airflow/pull/6929/diff?src=pr&el=tree#diff-YWlyZmxvdy91dGlscy9sb2cvd2FzYl90YXNrX2hhbmRsZXIucHk=) | `42.46% <0%> (ø)` | :arrow_up: |
   | [airflow/utils/log/file\_processor\_handler.py](https://codecov.io/gh/apache/airflow/pull/6929/diff?src=pr&el=tree#diff-YWlyZmxvdy91dGlscy9sb2cvZmlsZV9wcm9jZXNzb3JfaGFuZGxlci5weQ==) | `85.33% <100%> (ø)` | :arrow_up: |
   | [airflow/utils/sqlalchemy.py](https://codecov.io/gh/apache/airflow/pull/6929/diff?src=pr&el=tree#diff-YWlyZmxvdy91dGlscy9zcWxhbGNoZW15LnB5) | `96.66% <100%> (+0.05%)` | :arrow_up: |
   | [airflow/utils/log/logging\_mixin.py](https://codecov.io/gh/apache/airflow/pull/6929/diff?src=pr&el=tree#diff-YWlyZmxvdy91dGlscy9sb2cvbG9nZ2luZ19taXhpbi5weQ==) | `95.38% <100%> (ø)` | :arrow_up: |
   | [airflow/utils/net.py](https://codecov.io/gh/apache/airflow/pull/6929/diff?src=pr&el=tree#diff-YWlyZmxvdy91dGlscy9uZXQucHk=) | `81.25% <100%> (ø)` | :arrow_up: |
   | [airflow/utils/email.py](https://codecov.io/gh/apache/airflow/pull/6929/diff?src=pr&el=tree#diff-YWlyZmxvdy91dGlscy9lbWFpbC5weQ==) | `100% <100%> (ø)` | :arrow_up: |
   | ... and [19 more](https://codecov.io/gh/apache/airflow/pull/6929/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/6929?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/6929?src=pr&el=footer). Last update [be812bd...dbe0cfa](https://codecov.io/gh/apache/airflow/pull/6929?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [airflow] nuclearpinguin commented on a change in pull request #6929: [AIRFLOW-6373] Make airflow/utils pylint compatible

Posted by GitBox <gi...@apache.org>.

nuclearpinguin commented on a change in pull request #6929: [AIRFLOW-6373] Make airflow/utils pylint compatible
URL: https://github.com/apache/airflow/pull/6929#discussion_r361725384
 
 

 ##########
 File path: scripts/ci/pylint_todo.txt
 ##########
 @@ -198,34 +198,7 @@
 ./airflow/ti_deps/deps/task_concurrency_dep.py
 ./airflow/ti_deps/deps/trigger_rule_dep.py
 ./airflow/ti_deps/deps/valid_state_dep.py
-./airflow/utils/asciiart.py
-./airflow/utils/cli_action_loggers.py
-./airflow/utils/compression.py
-./airflow/utils/configuration.py
-./airflow/utils/dates.py
-./airflow/utils/db.py
-./airflow/utils/decorators.py
-./airflow/utils/email.py
-./airflow/utils/file.py
-./airflow/utils/helpers.py
-./airflow/utils/json.py
-./airflow/utils/log/es_task_handler.py
-./airflow/utils/log/file_processor_handler.py
-./airflow/utils/log/gcs_task_handler.py
-./airflow/utils/log/logging_mixin.py
-./airflow/utils/log/s3_task_handler.py
-./airflow/utils/log/wasb_task_handler.py
-./airflow/utils/module_loading.py
-./airflow/utils/net.py
-./airflow/utils/operator_helpers.py
-./airflow/utils/operator_resources.py
-./airflow/utils/sqlalchemy.py
-./airflow/utils/state.py
-./airflow/utils/tests.py
-./airflow/utils/timeout.py
-./airflow/utils/timezone.py
-./airflow/utils/trigger_rule.py
-./airflow/utils/weight_rule.py
+./airflow/utils/log/colored_log.py
 
 Review comment:
   I have no idea why but we have cyclic import here:
   ```
   ************* Module airflow.utils.log.colored_log
   airflow/utils/log/colored_log.py:1:0: R0401: Cyclic import (airflow.models -> airflow.models.baseoperator -> airflow.utils.db) (cyclic-import)
   airflow/utils/log/colored_log.py:1:0: R0401: Cyclic import (airflow.models.baseoperator -> airflow.utils.helpers) (cyclic-import)
   airflow/utils/log/colored_log.py:1:0: R0401: Cyclic import (airflow.models.baseoperator -> airflow.utils.db -> airflow.models.serialized_dag -> airflow.serialization.serialized_objects) (cyclic-import)
   airflow/utils/log/colored_log.py:1:0: R0401: Cyclic import (airflow.models.serialized_dag -> airflow.utils.db) (cyclic-import)
   airflow/utils/log/colored_log.py:1:0: R0401: Cyclic import (airflow.models -> airflow.models.taskreschedule -> airflow.utils.db) (cyclic-import)
   ```

----------------------------------------------------------------
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


With regards,
Apache Git Services