Skip to content
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

Move Airflow to TFXA #186

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Move Airflow to TFXA #186

wants to merge 9 commits into from

Conversation

murthyvs
Copy link

Fixes #<issue_number_goes_here>

It's a good idea to open an issue first for discussion.

  • Tests pass
  • Appropriate changes to README are included in PR

@google-cla
Copy link

google-cla bot commented Sep 20, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@github-actions
Copy link
Contributor

Thanks for the PR! 🚀

Instructions: Approve using /lgtm and mark for automatic merge by using /merge.

**Project name:** Airflow Orchestration

## Project Description
Moving Airflow to TFX add-ons due to decreasing native support.
Copy link
Collaborator

@rcrowe-google rcrowe-google Sep 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please describe what the results of the project will be, and a short introduction to what Airflow is and how it's used. You can also discuss the transition from having Airflow support included in TFX directly, versus having it as a separate install.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can we installed and used as a separate package under tfxa/.

Orchestrator

## Project Use-Case(s)
We are moving Airflow from tfx/orchestration to tfx-addons. Native support for Airflow won't be provided in the near future.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A use-case describes how the project will be used by developers. In this case it should be something more like:

Apache Airflow is a widely used orchestrator for ML workflows. Developers can use Airflow with TFX to orchestrate TFX pipelines. This project continues support for Airflow, which includes the web console and CLI tooling which developers can use to monitor and control their TFX pipelines.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imagine that you're a developer who is new to TFX. What do you need to know about Airflow, and how it fits with TFX? Why should you consider installing this module?

We are moving Airflow from tfx/orchestration to tfx-addons. Native support for Airflow won't be provided in the near future.

## Project Implementation
Move Airflow from tfx/orchestration to tfx-addons.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs some more detail about how the move will be accomplished.

Move Airflow from tfx/orchestration to tfx-addons.

## Project Dependencies
Same as tfx/orchestration/airflow.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These need to be listed out.

Same as tfx/orchestration/airflow.

## Project Team
Google Core ML TFX Team (MTV + Seoul)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The project team members need to be listed, including Github IDs and email addresses. The team members themselves will be responsible for maintaining the project, so they need to be aware of their responsibilities.

@murthyvs
Copy link
Author

Please have a look at this d9a2374

@@ -19,13 +19,29 @@ Orchestrator
We are moving Airflow from tfx/orchestration to tfx-addons. Native support for Airflow won't be provided in the near future.

## Project Implementation
Move Airflow from tfx/orchestration to tfx-addons.

1. Copy tfx/orchestration/airflow to tfx-addons/airflow.
Copy link
Collaborator

@rcrowe-google rcrowe-google Sep 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Please describe how the install will work
  2. Please discuss any issues with testing in CI, given that this code will be independent of TFX
  3. Please describe any restructuring of the code, such as adding a setup script and tests

Copy link
Author

@murthyvs murthyvs Sep 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So ... this is where it gets a bit interesting because the tutorials on the TF website need to reflect the deprecation too. It's a multi-part effort in which we need to first move the orchestrator to TFXA and THEN update the documentation, delete/remove dependencies from tfx/. We can't provide an accurate, line-by-line description as of today ... but we need to create a project in TFXA to get started.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's ok, just describe the issues and some options for dealing with them, and note that we haven't settled on a particular option yet. At some point however, we will need a solid plan to do this so that we can be successful.

1. Copy tfx/orchestration/airflow to tfx-addons/airflow.
2. Mark Airflow as deprecated in tfx/ and indicate that support will be dropped in 1-2 releases.
3. Update TFX tutorials on www.tensorflow.org to indicate deprecated and moving to TFXA

## Project Dependencies
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


## Project Team
Google Core ML TFX Team (MTV + Seoul)
Varun Murthy ([email protected])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, and we also need your Github ID

Comment on lines +29 to +30
## Project Team
murthyvs: Varun Murthy ([email protected])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will Google continue to support it from TFX-Addons? Or is the idea it will be abandoned? We don't want to reach a place where Airflow is released in TFX-Addons and then abandoned by maintainers. We will need some garantees on maintainance from Google or whoever is maintaining this component of the project.

The wording on Native support for Airflow won't be provided in the near future. is not super great, maybe we can specify that it will continue to be supported on TFX-Addons.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, unless Google can provide with a clear path of support when moved to TFX-Addons for long term maintainability of this component, expect my -1 on this.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@1025KB can help with this.

@casassg
Copy link
Member

casassg commented Sep 28, 2022

Also pls make sure to check CLA bot failing :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants