-
Notifications
You must be signed in to change notification settings - Fork 16.3k
Run Dataflow for ML Engine summary in venv #7809
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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 install these libraries in an isolated environment outside of user code. In this environment, we only run Airflow code. At this point, Airflow is not a framework, but an application. We do not need an extension point here, i.e. change versions. Version pinning is a good solution here to maintain stability.
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 am not sure if this is possible for example in Composer environment. But I like it.
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.
The Cloud Composer team must think of a way to access pip if they want this operator to work. This team is following changes in MLEngine and is aware of new requirements.
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.
They can also prepare this virtual environment in a Docker image and provide it with the installation. In the next step, they can modify this call to use a pre-baked environment.
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.
Dataflow does not print anything on the screen by default. Good practice says to configure the logger to be able to track the progress. This code is run in a separate process, so it's safe.
ceb3b22 to
97f38cc
Compare
|
@potiuk Can I have your review again? Travis is happy. |
potiuk
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.
If Travis is happy, I am happy as well!
Issue link: WILL BE INSERTED BY boring-cyborg
Make sure to mark the boxes below before creating PR: [x]
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.
Read the Pull Request Guidelines for more information.