-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
Add support for Hyperparameter Tuning on Google Cloud AI Platform #15429
Add support for Hyperparameter Tuning on Google Cloud AI Platform #15429
Conversation
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst)
|
@@ -1189,6 +1189,7 @@ def __init__( | |||
mode: str = 'PRODUCTION', | |||
labels: Optional[Dict[str, str]] = None, | |||
impersonation_chain: Optional[Union[str, Sequence[str]]] = None, | |||
hyperparameters: Optional[Dict] = None, |
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.
Add parameter in class documentation and maybe a link to Google documentation can be helpful to users https://cloud.google.com/ai-platform/training/docs/using-hyperparameter-tuning and https://cloud.google.com/ai-platform/training/docs/reference/rest/v1/projects.jobs#HyperparameterSpec
:param hyperparameters Make a dictionary representing your HyperparameterSpec and add it to your training input.
:type Dict
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.
Updated and pushed the code
66d839e
to
10f29f8
Compare
:param hyperparameters: Optional HyperparameterSpec dictionary for hyperparameter tuning | ||
For further reference, check these: | ||
https://cloud.google.com/ai-platform/training/docs/using-hyperparameter-tuning | ||
https://cloud.google.com/ai-platform/training/docs/reference/rest/v1/projects.jobs#HyperparameterSpec | ||
:type hyperparameters: Dict |
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.
Take a look here
I think the way you wrote the documentation they wont be render as lists.
You can test locally using ./breeze build-docs -- --package-filter apache-airflow-providers-google
to generate documentation.
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'm not able to run this locally. It exists without any error message.
I tried pushing the code to PR but the build throws an error for the 2nd URL line - https://cloud.google.com/ai-platform/training/docs/reference/rest/v1/projects.jobs#HyperparameterSpec. The line has 112 characters and the max allowed is 110. Will it be okay if I modify the /airflow/pylintrc file and update the max-line-length value to 112? Or is there any other workaround?
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.
Did you enable pre-commit to validate pylint/flake etc?
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'm not able to run pre-commit on my system. It's throwing permission error. Been trying to find a solution for a while, but haven't figured it yet.
Meanwhile, I've updated and pushed the documentation as per the demo in the link you had shared. Please check it out.
c91463d
to
7c19baa
Compare
The Workflow run is cancelling this PR. Building images for the PR has failed. Follow the workflow link to check the reason. |
ddf814d
to
7c19baa
Compare
Thanks for the PR! Can you also add a test case to the file https://github.com/apache/airflow/blob/master/tests/providers/google/cloud/operators/test_mlengine.py ? |
+1 to the request for test case, and also consider adding an example to https://github.com/apache/airflow/blob/master/airflow/providers/google/cloud/example_dags/example_mlengine.py Thanks for your contribution! :) |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
@@ -1274,6 +1280,9 @@ def execute(self, context): | |||
if self._service_account: | |||
training_request['trainingInput']['serviceAccount'] = self._service_account | |||
|
|||
if self._hyperparameters: | |||
training_request['trainingInput']['hyperparameters'] = self._hyperparameters |
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.
Can you add tests to avoid regression?
The MLEngineStartTrainingJobOperator is modified in airflow/providers/google/cloud/operators/mlengine.py file.
The lines 1138-1142, 1192, 1214, 1279, and 1280 are added to the file.
These lines add support for Model training involving hyperparameter tuning on AI Platform.