-
Notifications
You must be signed in to change notification settings - Fork 9
AIRFLOW-2193- Added r-base and rpy2 for ROperator #5
base: master
Are you sure you want to change the base?
Conversation
zhongjiajie
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more question, did you test with docker build, because I try several times months ago but failed to build docker images
Dockerfile
Outdated
| sudo -H pip install wheel tox && \ | ||
| sudo -H pip3 install --upgrade pip && \ | ||
| sudo -H pip3 install wheel tox && \ | ||
| sudo -H pip3 install rpy2 && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't know to install rpy2 here, should install by Airflow , not Airflow-CI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have deleted this line.
…, should install by Airflow , not Airflow-CI" deleting installing of rpy2.
|
Just for your information - I am working on making this image obsolete and replace it with the docker image from the main airflow repository (within days I hope). I will make sure to add R to the image in Airflow or you can make similar pull request there @matkalinowski - as you prefer. |
|
good news thanks @potiuk |
|
Is this PR? apache/airflow#4938 |
I think it would be faster if you could do this. Please remember to also add this line: This setups R repositories source site. |
|
My preference would be to switch to @potiuk his image first. You could even create a PR onto his PR :-) |
|
I'd really rather we didn't run R in the airflow unit test suite - that isn't our job. |
|
I second @ash on this. Having to debug R tests would add an extra barrier for contributors since it's not a common language for system developers. |
|
I agree with @ashb and @dimberman. And I also have a concrete proposal how to address such needs as @matkalinowski (if I understand it correctly). What I plan to take a look after AIP-10 (docker) and AI-7 hopefully (Breeze) is to come back to propoal of AIP-4 (Support for system tests for external systems). There I wanted to add a way to run system tests that could be run via CI (but not necessarily running in default CI of Airflow) to communicate with external systems and maybe we could extend that so that those R-related tests are treated as "external systems" (even though R is not an external system but it can be treated as such) The Gist of AIP-4 will be to let people add "System Tests" testing external systems (like GCP) which should be very easy to run automatically (so likely using the same pytest approach and TestCase base class) but to skip them by default. This way we could have some custom CI setup per team that would run the tests automatically (if they choose to configure the external systems). So the idea that I vaguely have in mind is to be able to a) add extra requirements to be added to image (in this case R) before tests are run and b) setup environment so that tests for external system can be run easily. This way we could keep R-tests or GCP tests or AWS tests in the code but without running them all the time (but each team working on such tests can easily build CI setup that will run tests for their work). Would that be something that could address your needs @matkalinowski (and address your concerns @ashb @dimberman ) ? |
Added r-base and rpy2 for open ROperator pull request in: apache/airflow#3115.
Docker image is building successfully.