Skip to content

Conversation

@mik-laj
Copy link
Member

@mik-laj mik-laj commented Mar 23, 2020


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.

@boring-cyborg boring-cyborg bot added the provider:google Google (including GCP) related issues label Mar 23, 2020
Copy link
Member Author

@mik-laj mik-laj Mar 23, 2020

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.

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Member Author

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.

@mik-laj mik-laj force-pushed the mlengine-backport-5 branch from ceb3b22 to 97f38cc Compare March 23, 2020 06:45
@mik-laj
Copy link
Member Author

mik-laj commented Mar 24, 2020

@potiuk Can I have your review again? Travis is happy.

Copy link
Member

@potiuk potiuk left a 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!

@mik-laj mik-laj merged commit 1982c3f into apache:master Mar 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

provider:google Google (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants