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/04/02 15:39:19 UTC
[GitHub] [airflow] j-y-matsubara opened a new pull request #8068: Make
models/pool.py pylint compatible
j-y-matsubara opened a new pull request #8068: Make models/pool.py pylint compatible
URL: https://github.com/apache/airflow/pull/8068
---
This PR fixes some pylint errors and divide the airflow/ti_dep/dependencies.py to avoid cyclic import.
Issue link: WILL BE INSERTED BY [boring-cyborg](https://github.com/kaxil/boring-cyborg)
Make sure to mark the boxes below before creating PR: [x]
- [x] Description above provides context of the change
- [x] Unit tests coverage for changes (not needed for documentation changes)
- [x] Commits follow "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)"
- [x] Relevant documentation is updated including usage instructions.
- [x] I will engage committers as explained in [Contribution Workflow Example](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#contribution-workflow-example).
---
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).
Read the [Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines) for more information.
----------------------------------------------------------------
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] j-y-matsubara commented on issue #8068: Make
models/pool.py pylint compatible
Posted by GitBox <gi...@apache.org>.
j-y-matsubara commented on issue #8068: Make models/pool.py pylint compatible
URL: https://github.com/apache/airflow/pull/8068#issuecomment-608574983
I don't know why this error is occurred in travis
```
tests/runtime/kubernetes/test_kubernetes_executor.py:203:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
tests/runtime/kubernetes/test_kubernetes_executor.py:140: in ensure_dag_expected_state
self.assertEqual(state, expected_final_state)
E AssertionError: 'failed' != 'success'
E - failed
E + success
```
----------------------------------------------------------------
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] turbaszek commented on issue #8068: Make models/pool.py
pylint compatible
Posted by GitBox <gi...@apache.org>.
turbaszek commented on issue #8068: Make models/pool.py pylint compatible
URL: https://github.com/apache/airflow/pull/8068#issuecomment-608509510
> Thank you for your comments.
> I excluded scripts/ci/pylint_todo.txt from this commit. ( not removing the file from the todo list. )
> Is this alright with you, @turbaszek @kaxil @potiuk ?
Yeah! Can you fix the imports? It seems that isort is sad :<
----------------------------------------------------------------
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] j-y-matsubara edited a comment on issue #8068: Make
models/pool.py pylint compatible
Posted by GitBox <gi...@apache.org>.
j-y-matsubara edited a comment on issue #8068: Make models/pool.py pylint compatible
URL: https://github.com/apache/airflow/pull/8068#issuecomment-607924106
Cyclic import message:
```
・Cyclic import (airflow.models -> airflow.models.pool -> airflow.ti_deps.deps.pool_slots_available_dep) (cyclic-import)
・Cyclic import (airflow.models -> airflow.models.baseoperator -> airflow.models.pool -> airflow.ti_deps.deps.pool_slots_available_dep) (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] j-y-matsubara commented on issue #8068: Make
models/pool.py pylint compatible
Posted by GitBox <gi...@apache.org>.
j-y-matsubara commented on issue #8068: Make models/pool.py pylint compatible
URL: https://github.com/apache/airflow/pull/8068#issuecomment-607924106
Cyclic import message:
・Cyclic import (airflow.models -> airflow.models.pool -> airflow.ti_deps.deps.pool_slots_available_dep) (cyclic-import)
・Cyclic import (airflow.models -> airflow.models.baseoperator -> airflow.models.pool -> airflow.ti_deps.deps.pool_slots_available_dep) (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] j-y-matsubara edited a comment on issue #8068: Make
models/pool.py pylint compatible
Posted by GitBox <gi...@apache.org>.
j-y-matsubara edited a comment on issue #8068: Make models/pool.py pylint compatible
URL: https://github.com/apache/airflow/pull/8068#issuecomment-607924106
These Cyclic imports were fixed:
```
・Cyclic import (airflow.models -> airflow.models.pool -> airflow.ti_deps.deps.pool_slots_available_dep) (cyclic-import)
・Cyclic import (airflow.models -> airflow.models.baseoperator -> airflow.models.pool -> airflow.ti_deps.deps.pool_slots_available_dep) (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] codecov-io commented on issue #8068: Make models/pool.py
pylint compatible
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #8068: Make models/pool.py pylint compatible
URL: https://github.com/apache/airflow/pull/8068#issuecomment-608597992
# [Codecov](https://codecov.io/gh/apache/airflow/pull/8068?src=pr&el=h1) Report
> Merging [#8068](https://codecov.io/gh/apache/airflow/pull/8068?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/10542e0ab85397edbb38949097876ba5c1839bd0&el=desc) will **decrease** coverage by `1.42%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/airflow/pull/8068?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #8068 +/- ##
==========================================
- Coverage 87.27% 85.85% -1.43%
==========================================
Files 937 938 +1
Lines 45556 45969 +413
==========================================
- Hits 39760 39466 -294
- Misses 5796 6503 +707
```
| [Impacted Files](https://codecov.io/gh/apache/airflow/pull/8068?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [airflow/cli/commands/task\_command.py](https://codecov.io/gh/apache/airflow/pull/8068/diff?src=pr&el=tree#diff-YWlyZmxvdy9jbGkvY29tbWFuZHMvdGFza19jb21tYW5kLnB5) | `73.56% <100.00%> (ø)` | |
| [airflow/jobs/backfill\_job.py](https://codecov.io/gh/apache/airflow/pull/8068/diff?src=pr&el=tree#diff-YWlyZmxvdy9qb2JzL2JhY2tmaWxsX2pvYi5weQ==) | `91.64% <100.00%> (ø)` | |
| [airflow/jobs/scheduler\_job.py](https://codecov.io/gh/apache/airflow/pull/8068/diff?src=pr&el=tree#diff-YWlyZmxvdy9qb2JzL3NjaGVkdWxlcl9qb2IucHk=) | `90.67% <100.00%> (-0.15%)` | :arrow_down: |
| [airflow/models/dagrun.py](https://codecov.io/gh/apache/airflow/pull/8068/diff?src=pr&el=tree#diff-YWlyZmxvdy9tb2RlbHMvZGFncnVuLnB5) | `95.68% <100.00%> (ø)` | |
| [airflow/models/pool.py](https://codecov.io/gh/apache/airflow/pull/8068/diff?src=pr&el=tree#diff-YWlyZmxvdy9tb2RlbHMvcG9vbC5weQ==) | `97.29% <100.00%> (-0.08%)` | :arrow_down: |
| [airflow/models/taskinstance.py](https://codecov.io/gh/apache/airflow/pull/8068/diff?src=pr&el=tree#diff-YWlyZmxvdy9tb2RlbHMvdGFza2luc3RhbmNlLnB5) | `94.70% <100.00%> (ø)` | |
| [airflow/ti\_deps/dependencies\_deps.py](https://codecov.io/gh/apache/airflow/pull/8068/diff?src=pr&el=tree#diff-YWlyZmxvdy90aV9kZXBzL2RlcGVuZGVuY2llc19kZXBzLnB5) | `100.00% <100.00%> (ø)` | |
| [airflow/ti\_deps/dependencies\_states.py](https://codecov.io/gh/apache/airflow/pull/8068/diff?src=pr&el=tree#diff-YWlyZmxvdy90aV9kZXBzL2RlcGVuZGVuY2llc19zdGF0ZXMucHk=) | `100.00% <100.00%> (ø)` | |
| [airflow/ti\_deps/deps/pool\_slots\_available\_dep.py](https://codecov.io/gh/apache/airflow/pull/8068/diff?src=pr&el=tree#diff-YWlyZmxvdy90aV9kZXBzL2RlcHMvcG9vbF9zbG90c19hdmFpbGFibGVfZGVwLnB5) | `100.00% <100.00%> (ø)` | |
| [airflow/www/views.py](https://codecov.io/gh/apache/airflow/pull/8068/diff?src=pr&el=tree#diff-YWlyZmxvdy93d3cvdmlld3MucHk=) | `76.53% <100.00%> (ø)` | |
| ... and [25 more](https://codecov.io/gh/apache/airflow/pull/8068/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/8068?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/8068?src=pr&el=footer). Last update [10542e0...070a20a](https://codecov.io/gh/apache/airflow/pull/8068?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] j-y-matsubara edited a comment on issue #8068: Make
models/pool.py pylint compatible
Posted by GitBox <gi...@apache.org>.
j-y-matsubara edited a comment on issue #8068: Make models/pool.py pylint compatible
URL: https://github.com/apache/airflow/pull/8068#issuecomment-608547517
> > Thank you for your comments.
> > I excluded scripts/ci/pylint_todo.txt from this commit. ( not removing the file from the todo list. )
> > Is this alright with you, @turbaszek @kaxil @potiuk ?
>
> Yeah! Can you fix the imports? It seems that isort is sad :<
Thank you for your comments, @turbaszek.
I fixed the imports for isort.
----------------------------------------------------------------
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] j-y-matsubara commented on issue #8068: Make
models/pool.py pylint compatible
Posted by GitBox <gi...@apache.org>.
j-y-matsubara commented on issue #8068: Make models/pool.py pylint compatible
URL: https://github.com/apache/airflow/pull/8068#issuecomment-608547517
> > Thank you for your comments.
> > I excluded scripts/ci/pylint_todo.txt from this commit. ( not removing the file from the todo list. )
> > Is this alright with you, @turbaszek @kaxil @potiuk ?
>
> Yeah! Can you fix the imports? It seems that isort is sad :<
Thank you for your comments, @turbaszek.
I fixed the imports.
----------------------------------------------------------------
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] j-y-matsubara removed a comment on issue #8068: Make
models/pool.py pylint compatible
Posted by GitBox <gi...@apache.org>.
j-y-matsubara removed a comment on issue #8068: Make models/pool.py pylint compatible
URL: https://github.com/apache/airflow/pull/8068#issuecomment-608574983
I don't know why this error is occurred in travis
```
tests/runtime/kubernetes/test_kubernetes_executor.py:203:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
tests/runtime/kubernetes/test_kubernetes_executor.py:140: in ensure_dag_expected_state
self.assertEqual(state, expected_final_state)
E AssertionError: 'failed' != 'success'
E - failed
E + success
```
----------------------------------------------------------------
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] boring-cyborg[bot] commented on issue #8068: Make
models/pool.py pylint compatible
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on issue #8068: Make models/pool.py pylint compatible
URL: https://github.com/apache/airflow/pull/8068#issuecomment-608993061
Awesome work, congrats on your first merged pull request!
----------------------------------------------------------------
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] kaxil commented on issue #8068: Make models/pool.py
pylint compatible
Posted by GitBox <gi...@apache.org>.
kaxil commented on issue #8068: Make models/pool.py pylint compatible
URL: https://github.com/apache/airflow/pull/8068#issuecomment-608421140
> Hi @j-y-matsubara ! Those changes look good but you've found the reason why those files are still on the todo list: cyclic imports. However, I think we should merge this change and not remove the file from the todo list. In this way, we will have less work in the future. WDYT @potiuk @kaxil ?
Yea that is fine 👍
----------------------------------------------------------------
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] j-y-matsubara commented on issue #8068: Make
models/pool.py pylint compatible
Posted by GitBox <gi...@apache.org>.
j-y-matsubara commented on issue #8068: Make models/pool.py pylint compatible
URL: https://github.com/apache/airflow/pull/8068#issuecomment-608505782
Thank you for your comments.
I excluded scripts/ci/pylint_todo.txt from commit. ( not removing the file from the todo list. )
Is this alright with you, @turbaszek @kaxil @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] j-y-matsubara edited a comment on issue #8068: Make
models/pool.py pylint compatible
Posted by GitBox <gi...@apache.org>.
j-y-matsubara edited a comment on issue #8068: Make models/pool.py pylint compatible
URL: https://github.com/apache/airflow/pull/8068#issuecomment-608505782
Thank you for your comments.
I excluded scripts/ci/pylint_todo.txt from this commit. ( not removing the file from the todo list. )
Is this alright with you, @turbaszek @kaxil @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] mik-laj commented on a change in pull request #8068: Make
models/pool.py pylint compatible
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #8068: Make models/pool.py pylint compatible
URL: https://github.com/apache/airflow/pull/8068#discussion_r402563370
##########
File path: airflow/ti_deps/deps/pool_slots_available_dep.py
##########
@@ -20,9 +20,7 @@
from airflow.ti_deps.deps.base_ti_dep import BaseTIDep
from airflow.utils.session import provide_session
-from airflow.utils.state import State
-
-STATES_TO_COUNT_AS_RUNNING = [State.RUNNING, State.QUEUED]
Review comment:
In my opinion, it could be in 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] j-y-matsubara edited a comment on issue #8068: Make
models/pool.py pylint compatible
Posted by GitBox <gi...@apache.org>.
j-y-matsubara edited a comment on issue #8068: Make models/pool.py pylint compatible
URL: https://github.com/apache/airflow/pull/8068#issuecomment-608190343
Thank you for your comments, @turbaszek.
I will continue to look forward to comments, everyone.
----------------------------------------------------------------
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] j-y-matsubara edited a comment on issue #8068: Make
models/pool.py pylint compatible
Posted by GitBox <gi...@apache.org>.
j-y-matsubara edited a comment on issue #8068: Make models/pool.py pylint compatible
URL: https://github.com/apache/airflow/pull/8068#issuecomment-608190343
Thank you for your comments, @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
With regards,
Apache Git Services
[GitHub] [airflow] turbaszek merged pull request #8068: Make models/pool.py
pylint compatible
Posted by GitBox <gi...@apache.org>.
turbaszek merged pull request #8068: Make models/pool.py pylint compatible
URL: https://github.com/apache/airflow/pull/8068
----------------------------------------------------------------
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] turbaszek commented on issue #8068: Make models/pool.py
pylint compatible
Posted by GitBox <gi...@apache.org>.
turbaszek commented on issue #8068: Make models/pool.py pylint compatible
URL: https://github.com/apache/airflow/pull/8068#issuecomment-608008454
Hi @j-y-matsubara ! Those changes look good but you've found the reason why those files are still on the todo list: cyclic imports. However, I think we should merge this change and not remove the file from the todo list. In this way, we will have less work in the future. WDYT @potiuk @kaxil ?
----------------------------------------------------------------
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] j-y-matsubara commented on issue #8068: Make
models/pool.py pylint compatible
Posted by GitBox <gi...@apache.org>.
j-y-matsubara commented on issue #8068: Make models/pool.py pylint compatible
URL: https://github.com/apache/airflow/pull/8068#issuecomment-608190343
Thank you for your comments, @turbaszek.
If should I cancel scripts/ci/pylint_todo.txt modifications, I will do it right away.
----------------------------------------------------------------
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] turbaszek commented on a change in pull request #8068:
Make models/pool.py pylint compatible
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #8068: Make models/pool.py pylint compatible
URL: https://github.com/apache/airflow/pull/8068#discussion_r402500613
##########
File path: airflow/ti_deps/deps/pool_slots_available_dep.py
##########
@@ -20,9 +20,7 @@
from airflow.ti_deps.deps.base_ti_dep import BaseTIDep
from airflow.utils.session import provide_session
-from airflow.utils.state import State
-
-STATES_TO_COUNT_AS_RUNNING = [State.RUNNING, State.QUEUED]
Review comment:
I like this change but it could be better to make this as separate PR, WDYT @mik-laj ?
----------------------------------------------------------------
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