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.

Sign up for GitHub

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

Jump to bottom

Create CustomJob and Datasets operators for Vertex AI service #20077

Merged
merged 20 commits into from
Jan 28, 2022
Merged

Create CustomJob and Datasets operators for Vertex AI service #20077

merged 20 commits into from
Jan 28, 2022

Conversation

MaksYermak
Copy link
Contributor

@MaksYermak MaksYermak commented Dec 6, 2021 •
edited
Loading

Create operators for working with Custom Job and Datasets for Vertex AI service. Includes operators, hooks, example dags, tests and docs.

Co-authored-by: Wojciech Januszek [email protected]
Co-authored-by: Lukasz Wyszomirski [email protected]
Co-authored-by: Maksim Yermakou [email protected]

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
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.

@boring-cyborg boring-cyborg bot added area:providers kind:documentation provider:google Google (including GCP) related issues labels Dec 6, 2021
@MaksYermak MaksYermak force-pushed the vertex-ai-operators branch 2 times, most recently from 100781f to 3787260 Compare December 9, 2021 10:45
@potiuk
Copy link
Member

potiuk commented Dec 13, 2021

Tests are failing - please fix and rebase.

@MaksYermak MaksYermak force-pushed the vertex-ai-operators branch 5 times, most recently from 4303c1e to 3c1b4ca Compare December 15, 2021 16:24
@MaksYermak
Copy link
Contributor Author

@potiuk could we merge this PR ?

@MaksYermak
Copy link
Contributor Author

@josh-fell @mik-laj @turbaszek @vikramkoka @ashb @potiuk hi guys what about this PR, could we merge it?

josh-fell
josh-fell reviewed Jan 13, 2022
View reviewed changes
Copy link
Contributor

@josh-fell josh-fell left a comment

Choose a reason for hiding this comment

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

It would be nice if the Custom Job operators had docstrings for params/types so they are exposed in the Airflow documentation.

airflow/providers/google/cloud/operators/vertex_ai/custom_job.py Outdated Show resolved Hide resolved
airflow/providers/google/cloud/example_dags/example_vertex_ai.py Show resolved Hide resolved
@MaksYermak MaksYermak force-pushed the vertex-ai-operators branch 3 times, most recently from 11b1d45 to abc7475 Compare January 18, 2022 09:40
@MaksYermak
Copy link
Contributor Author

@potiuk @josh-fell hi guys could you look on my PR one more time and approve/merge if all looks good?

@josh-fell
Copy link
Contributor

@potiuk @josh-fell hi guys could you look on my PR one more time and approve/merge if all looks good?

Just a small comment on a possible clarification but LGTM.

@josh-fell
Copy link
Contributor

@ashb Should the :type: directives in the docstrings here be removed preemptively because of #20951?

@ashb
Copy link
Member

ashb commented Jan 20, 2022

Yes please!

@MaksYermak
Copy link
Contributor Author

MaksYermak commented Jan 20, 2022 •
edited
Loading

@ashb Should the :type: directives in the docstrings here be removed preemptively because of #20951?

@josh-fell I have deleted :type in last commit

@ashb
Copy link
Member

ashb commented Jan 20, 2022

@MaksYermak You might have to add some local spelling dictionaries to your hook file too -- check out 1149e63

Sorry about that.

@MaksYermak
Copy link
Contributor Author

@MaksYermak You might have to add some local spelling dictionaries to your hook file too -- check out 1149e63

Sorry about that.

@ashb I have run this command for check ./breeze build-docs -- --spellcheck-only --package-filter apache-airflow-providers-google and I haven´t seen any spell errors in this code.

@MaksYermak
Copy link
Contributor Author

@ashb @josh-fell @potiuk Hi all, could we merge this PR?

potiuk
potiuk approved these changes Jan 28, 2022
View reviewed changes
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.

Big. But looks like really all things are there. Was it - in big parts auto-generated? It certainly looks like (and we used to have airflow-munchkin for that :)), so I wonder if this is some kind of successor.

@github-actions
Copy link

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 main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Jan 28, 2022
@potiuk potiuk merged commit 640c0b6 into apache:main Jan 28, 2022
@jedcunningham jedcunningham mentioned this pull request Jan 28, 2022
Merged
jedcunningham added a commit to astronomer/airflow that referenced this pull request Jan 28, 2022
@jedcunningham
Revert "Create CustomJob and Datasets operators for Vertex AI service (…
50aec8b 
…apache#20077)"

This reverts commit 640c0b6.
jedcunningham added a commit that referenced this pull request Jan 29, 2022
@jedcunningham
Revert "Create CustomJob and Datasets operators for Vertex AI service (…
1d4b709 
…#20077)" (#21203)

This reverts commit 640c0b6.
@jedcunningham
Copy link
Member

This had a bunch of failing mypy issues once it landed in main. #21203 reverted it.

@MaksYermak, can you rebase your changes on current main, fix the mypy issues, and open a new PR? Thanks!

@potiuk
Copy link
Member

potiuk commented Jan 29, 2022 •
edited
Loading

Yeah. So the up-to-date checker did not prevent it because it was really last run 8 days ago :( . Up-to-date was pretty useless actually (or maybe not :) - this was the ONLY change with this problem. I wonder how many we prevented )

@potiuk potiuk mentioned this pull request Jan 29, 2022
Merged
potiuk added a commit to potiuk/airflow that referenced this pull request Jan 29, 2022
@potiuk
Remove up-to-date checks for Python
de3418a 
The up-to-date check for Python run for ~week so all the PRs
raised in the last week will need to be rebased to account for
MyPy changes. Hopefully there will be no more PRs from before,
that have not been rebased (we had one serious MyPy problem for
the apache#20077 change that was approved before the up-to-date checker
was enabled in apache#21016
potiuk added a commit that referenced this pull request Jan 29, 2022
@potiuk
Remove up-to-date checks for Python (#21208)
c93cb9e 
The up-to-date check for Python run for ~week so all the PRs
raised in the last week will need to be rebased to account for
MyPy changes. Hopefully there will be no more PRs from before,
that have not been rebased (we had one serious MyPy problem for
the #20077 change that was approved before the up-to-date checker
was enabled in #21016
@MaksYermak MaksYermak mentioned this pull request Feb 1, 2022
Merged
@MaksYermak
Copy link
Contributor Author

@potiuk @jedcunningham I have created a new PR #21253

@jedcunningham jedcunningham added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@josh-fell josh-fell josh-fell left review comments

@potiuk potiuk potiuk approved these changes

@mik-laj mik-laj Awaiting requested review from mik-laj

@turbaszek turbaszek Awaiting requested review from turbaszek

@vikramkoka vikramkoka Awaiting requested review from vikramkoka

@ashb ashb Awaiting requested review from ashb ashb is a code owner

Assignees
No one assigned
Labels
area:providers changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) full tests needed We need to run full set of tests for this PR to merge kind:documentation provider:google Google (including GCP) related issues
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@MaksYermak @potiuk @josh-fell @ashb @jedcunningham