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 2021/05/12 23:47:43 UTC
[GitHub] [airflow] dstandish opened a new pull request #15812: Move plyvel to google provider extra
dstandish opened a new pull request #15812:
URL: https://github.com/apache/airflow/pull/15812
Not sure if I did this correctly, or how to test installation of providers....
--
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] dstandish commented on pull request #15812: Move plyvel to google provider extra
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #15812:
URL: https://github.com/apache/airflow/pull/15812#issuecomment-843531891
> Yep, leveldb is indeed very separate from the rest. I think master is OK, but we have another major outage of Github Actions in a full swing: https://www.githubstatus.com/incidents/m16jzl31gnqt
>
> BTW. you need to rebase anyway.
Yeah i know i need to rebase but when i did locally at pre-commit i found this error:
```python
Run mypy.................................................................................Failed
- hook id: mypy
- exit code: 2
+ export PYTHONPATH=/opt/airflow
+ PYTHONPATH=/opt/airflow
+ mypy_args=()
+ for filename in "$@"
+ [[ setup.py == docs/* ]]
+ mypy_args+=("-m" "$(filename_to_python_module "$filename")")
++ filename_to_python_module setup.py
++ file=setup.py
++ no_leading_dotslash=setup.py
++ no_py=setup
++ no_init=setup
++ echo setup
+ mypy --namespace-packages -m setup
Unable to load the config, contains a configuration error.
setup.cfg:193: error: Error importing plugin 'airflow.mypy.plugin.decorators':
Unable to configure handler 'task': Cannot resolve
'airflow.utils.log.file_task_handler.FileTaskHandler': No module named 'httpx'
plugins =
^
Found 1 error in 1 file (checked 1 source file)
+ in_container_script_end
+ EXIT_CODE=2
+ [[ 2 != 0 ]]
+ [[ false == \t\r\u\e ]]
+ [[ false == \t\r\u\e ]]
+ in_container_clear_tmp
+ rm -rf /tmp/core-js-banners /tmp/v8-compile-cache-0 /tmp/yarn--1620688528816-0.4993168394804959 /tmp/yarn--1620688528816-0.8367507637222573 /tmp/yarn--1620688607348-0.5560835416794161
+ in_container_fix_ownership
+ [[ Darwin == \L\i\n\u\x ]]
```
--
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] dstandish commented on pull request #15812: Move plyvel to google provider extra
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #15812:
URL: https://github.com/apache/airflow/pull/15812#issuecomment-843448652
> For now I think the 'extra' is quick and pretty much backwards-compatible solution (with the right dependencies installed). Splitting it to independent providers will be much more laborious and can have some unforeseen cosequences (like the need to extract a "common" part to even separate provider.
For leveldb, there isn't any common part, i think. It does not use GCP base hooks, because it actually has nothing to do with GCP.
So it might actually be a simple job to create a distinct provider for this one. I'm not sure we really even need google in the name. Why couldn't it just be a `leveldb` provider? It came out of google but it's an open source project.
Just exploring options here. Already pushed the change with the approach you suggested. Will let the build go before rebasing with those conflicts (cus it seems there's something in master broken)
--
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 #15812: Move plyvel to google provider extra
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #15812:
URL: https://github.com/apache/airflow/pull/15812#issuecomment-843454478
Yep, leveldb is indeed very separate from the rest. I think master is OK, but we have another major outage of Github Actions in a full swing: https://www.githubstatus.com/incidents/m16jzl31gnqt
BTW. you need to rebase anyway.
--
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 #15812: Move plyvel to google provider extra
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #15812:
URL: https://github.com/apache/airflow/pull/15812#issuecomment-843445055
> @potiuk it seems that it may be more elegant to create a `google.leveldb` provider and pull leveld out of google, similar to what we see with apache. then there's no need to modify core extra requirements. (since leveldb isn't very "core")
>
> what do you think?
I think this would not play very well with the current hierarchy. I think We have not foreseen that we can have booth "providerX" and "providerX.something". I am afraid it might lead to some problems. I think - looking at the current "google" provider structure, splittiing it similarly to "microsoft" could have been better.
Maybe (but that's somethig that likely needs a bit more thought) we should split it to "google.cloud" , "google.firebase", etc. eventaualy.
For now I think the 'extra' is quick and pretty much backwards-compatible solution (with the right dependencies installed). Splitting it to independent providers will be much more laborious and can have some unforeseen cosequences (like the need to extract a "common" part to even separate provider.
--
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 #15812: Move plyvel to google provider extra
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #15812:
URL: https://github.com/apache/airflow/pull/15812#issuecomment-840598284
Just make sure that you run pre-commits as well. After you add 'plyvel' extra, you will also have to update "extra documentation" and pre-commit will fail if you did not :)
--
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] dstandish edited a comment on pull request #15812: Move plyvel to google provider extra
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on pull request #15812:
URL: https://github.com/apache/airflow/pull/15812#issuecomment-843428709
@potiuk it seems that it may be more elegant to create a `google.leveldb` provider and pull leveld out of google, similar to what we see with apache. then there's no need to modify core extra requirements. (since leveldb isn't very "core")
what do you think?
doing so also means we don't have an airflow level db extra _and_ a google leveldb extra, and avoids the decision of whether the airflow leveldb extra should also install the google provider 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
[GitHub] [airflow] potiuk commented on pull request #15812: Move plyvel to google provider extra
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #15812:
URL: https://github.com/apache/airflow/pull/15812#issuecomment-840596906
I think we need a little more :)
The providers are prepared and tested during CI build (We have a special job "Build and test povider packages).
And In fact the job did fail: https://github.com/apache/airflow/pull/15812/checks?check_run_id=2571762641
The job failed because our CI does not install plyvel any more since this is an optional dependency now.
But for CI image we run all the tests and check if all providers are fully importable, so we should also add plyvel as depenedncy installed during CI jib. This can be done simply as yet another extra for "airflow" itself - similalrly as we have virtualenv, statsd, rabbitmq etc. Simply adding it as one more extra in setup.py and adding it to the "CORE_EXTRAS_REQUIREMENTS" in setup.py extra should do the job.
I believe this is also the reason why level_db tests failed as well and doc build failed (because neither can import plyvel).
--
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 #15812: Move plyvel to google provider extra
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #15812:
URL: https://github.com/apache/airflow/pull/15812#issuecomment-840596906
--
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 #15812: Move plyvel to google provider extra
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #15812:
URL: https://github.com/apache/airflow/pull/15812#issuecomment-843445055
> @potiuk it seems that it may be more elegant to create a `google.leveldb` provider and pull leveld out of google, similar to what we see with apache. then there's no need to modify core extra requirements. (since leveldb isn't very "core")
>
> what do you think?
I think this would not play very well with the current hierarchy. I think We have not foreseen that we can have booth "providerX" and "providerX.something". I am afraid it might lead to some problems. I think - looking at the current "google" provider structure, splittiing it similarly to "microsoft" could have been better.
Maybe (but that's somethig that likely needs a bit more thought) we should split it to "google.cloud" , "google.firebase", etc. eventually.
For now I think the 'extra' is quick and pretty much backwards-compatible solution (with the right dependencies installed). Splitting it to independent providers will be much more laborious and can have some unforeseen cosequences (like the need to extract a "common" part to even separate provider.
--
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 #15812: Move plyvel to google provider extra
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #15812:
URL: https://github.com/apache/airflow/pull/15812#issuecomment-840596906
I think we need a little more :)
The providers are prepared and tested during CI build (We have a special job "Build and test povider packages").
And In fact the job did fail: https://github.com/apache/airflow/pull/15812/checks?check_run_id=2571762641
The job failed because our CI does not install plyvel any more since this is an optional dependency now.
But for CI image we run all the tests and check if all providers are fully importable, so we should also add plyvel as depenedncy installed during CI jib. This can be done simply as yet another extra for "airflow" itself - similalrly as we have virtualenv, statsd, rabbitmq etc. Simply adding it as one more extra in setup.py and adding it to the "CORE_EXTRAS_REQUIREMENTS" in setup.py extra should do the job.
I believe this is also the reason why level_db tests failed as well and doc build failed (because neither can import plyvel).
--
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] dstandish edited a comment on pull request #15812: Move plyvel to google provider extra
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on pull request #15812:
URL: https://github.com/apache/airflow/pull/15812#issuecomment-843428709
@potiuk it seems that it may be more elegant to create a `google.leveldb` provider and pull leveld out of google, similar to what we see with apache. then there's no need to modify core extra requirements. (since leveldb isn't very "core")
what do you think?
doing so also means we don't have an airflow level db extra _and_ a google provider leveldb extra, and avoids the decision of whether the airflow leveldb extra should also install the google provider 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
[GitHub] [airflow] dstandish edited a comment on pull request #15812: Move plyvel to google provider extra
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on pull request #15812:
URL: https://github.com/apache/airflow/pull/15812#issuecomment-843428709
@potiuk it seems that it may be more elegant to create a `google.leveldb` provider and pull leveld out of google, similar to what we see with apache. then there's no need to modify core extra requirements. (since leveldb isn't very "core")
what do you think?
--
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 #15812: Move plyvel to google provider extra
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15812:
URL: https://github.com/apache/airflow/pull/15812#issuecomment-843768102
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest master at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
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] dstandish commented on pull request #15812: Move plyvel to google provider extra
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #15812:
URL: https://github.com/apache/airflow/pull/15812#issuecomment-840601280
i will look into this thank you :)
--
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] dstandish commented on pull request #15812: Move plyvel to google provider extra
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #15812:
URL: https://github.com/apache/airflow/pull/15812#issuecomment-843428709
@potiuk it seems that it may be more elegant to create a `google.leveldb` provider and pull leveld out of google, as has been done with apache. then there's no need to modify core extra requirements. (since leveldb isn't very "core")
what do you think?
--
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 #15812: Move plyvel to google provider extra
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #15812:
URL: https://github.com/apache/airflow/pull/15812
--
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 #15812: Move plyvel to google provider extra
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #15812:
URL: https://github.com/apache/airflow/pull/15812#issuecomment-840596906
I think we need a little more :)
The providers are prepared and tested during CI build (We have a special job "Build and test povider packages").
And In fact the job did fail: https://github.com/apache/airflow/pull/15812/checks?check_run_id=2571762641
The job failed because our CI does not install plyvel any more since this is an optional dependency now.
But for CI image we run all the tests and check if all providers are fully importable, so we should also add plyvel as depenedncy installed during CI image build. This can be done simply as yet another extra for "airflow" itself - similarly as we have virtualenv, statsd, rabbitmq etc. Simply adding it as one more extra in setup.py and adding it to the "CORE_EXTRAS_REQUIREMENTS" in setup.py should do the job.
I believe this is also the reason why level_db tests failed as well and doc build failed (because neither can import plyvel).
--
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 #15812: Move plyvel to google provider extra
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15812:
URL: https://github.com/apache/airflow/pull/15812#issuecomment-843558947
[The Workflow run](https://github.com/apache/airflow/actions/runs/854696314) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^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 #15812: Move plyvel to google provider extra
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #15812:
URL: https://github.com/apache/airflow/pull/15812#issuecomment-843770468
I think this looks cool for now. I've opened https://github.com/apache/airflow/issues/15933 to discuss what we do with Google Provider.
--
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] dstandish edited a comment on pull request #15812: Move plyvel to google provider extra
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on pull request #15812:
URL: https://github.com/apache/airflow/pull/15812#issuecomment-843531891
> Yep, leveldb is indeed very separate from the rest. I think master is OK, but we have another major outage of Github Actions in a full swing: https://www.githubstatus.com/incidents/m16jzl31gnqt
>
> BTW. you need to rebase anyway.
Yeah i know i need to rebase but when i did locally at pre-commit i found this error:
```
Run mypy.................................................................................Failed
- hook id: mypy
- exit code: 2
+ export PYTHONPATH=/opt/airflow
+ PYTHONPATH=/opt/airflow
+ mypy_args=()
+ for filename in "$@"
+ [[ setup.py == docs/* ]]
+ mypy_args+=("-m" "$(filename_to_python_module "$filename")")
++ filename_to_python_module setup.py
++ file=setup.py
++ no_leading_dotslash=setup.py
++ no_py=setup
++ no_init=setup
++ echo setup
+ mypy --namespace-packages -m setup
Unable to load the config, contains a configuration error.
setup.cfg:193: error: Error importing plugin 'airflow.mypy.plugin.decorators':
Unable to configure handler 'task': Cannot resolve
'airflow.utils.log.file_task_handler.FileTaskHandler': No module named 'httpx'
plugins =
^
Found 1 error in 1 file (checked 1 source file)
+ in_container_script_end
+ EXIT_CODE=2
+ [[ 2 != 0 ]]
+ [[ false == \t\r\u\e ]]
+ [[ false == \t\r\u\e ]]
+ in_container_clear_tmp
+ rm -rf /tmp/core-js-banners /tmp/v8-compile-cache-0 /tmp/yarn--1620688528816-0.4993168394804959 /tmp/yarn--1620688528816-0.8367507637222573 /tmp/yarn--1620688607348-0.5560835416794161
+ in_container_fix_ownership
+ [[ Darwin == \L\i\n\u\x ]]
```
--
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 #15812: Move plyvel to google provider extra
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15812:
URL: https://github.com/apache/airflow/pull/15812#issuecomment-840233384
[The Workflow run](https://github.com/apache/airflow/actions/runs/837461299) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^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 #15812: Move plyvel to google provider extra
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #15812:
URL: https://github.com/apache/airflow/pull/15812#issuecomment-843765355
```
Unable to configure handler 'task': Cannot resolve
'airflow.utils.log.file_task_handler.FileTaskHandler': No module named 'httpx'
plugins =
```
Yeah. You need to rebuild the image when asked :)
--
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