-
Notifications
You must be signed in to change notification settings - Fork 14.8k
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
Adding params. to create_auto_ml_forecasting_training_job in AutoMl hook #39767
Conversation
Added window_stride_length & window_max_count
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 Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
|
Hi, I am really really new to open source community. I may not be fully aware lot of things here. Please help me grow here. Thanks, |
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.
Welcome to Apache Airflow, and thank you for your first contribution!
You may want to read the contributing docs to familiarize yourself with what you need to contribute here.
As for this specific PR, adding these parameters is a great idea - but there are a few things that you need to take care of before we'll be able to merge it:
- Fix docstrings and parameter assignment—To help you, I created suggestions that you can commit via GitHub UI or manually.
- Update the unit test in
tests/providers/google/cloud/operators/test_vertex_ai.py
in lines 1280-1343). It's a matter of adding the parameters to the operator call and the hook assertions to ensure it's passed correctly. If you're not familiar with the concept of unit tests, I suggest that you read some material about it (for example, in Real Python), as well as about the testing framework that we use, which is called pytest. - Change the PR's title to something more indicative (for example,
Adding params. to create_auto_ml_forecasting_training_job in AutoMl hook
).
If you're not sure of what or how to do the above, feel free to ask :)
Good luck!
Co-authored-by: Shahar Epstein <60007259+shahar1@users.noreply.github.com>
Co-authored-by: Shahar Epstein <60007259+shahar1@users.noreply.github.com>
Co-authored-by: Shahar Epstein <60007259+shahar1@users.noreply.github.com>
Co-authored-by: Shahar Epstein <60007259+shahar1@users.noreply.github.com>
Co-authored-by: Shahar Epstein <60007259+shahar1@users.noreply.github.com>
Co-authored-by: Shahar Epstein <60007259+shahar1@users.noreply.github.com>
Thank you Shahar, I have tried to implement things that you have mentioned. Could you please let me know if they look good. Thanks, |
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.
LGTM. Congratulations on your first PR.
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.
Looks good!
The tests need some improvement, but it could wait for other time IMO
Thank you Shahar, Could you please let me if there any action item on my side? Thanks, |
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.
removed spaces
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.
removed spaces
It looks good to me, but we need approval from a committer so it can be merged. |
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.
Small nit on docstring format, but LGTM!
Co-authored-by: Josh Fell <48934154+josh-fell@users.noreply.github.com>
Co-authored-by: Andrey Anshin <Andrey.Anshin@taragol.is>
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
Congrats @vinay2242g 🎉 |
…ook (apache#39767) * Update auto_ml.py Added window_stride_length & window_max_count * Update auto_ml.py * Update airflow/providers/google/cloud/hooks/vertex_ai/auto_ml.py Co-authored-by: Shahar Epstein <60007259+shahar1@users.noreply.github.com> * Update airflow/providers/google/cloud/hooks/vertex_ai/auto_ml.py Co-authored-by: Shahar Epstein <60007259+shahar1@users.noreply.github.com> * Update airflow/providers/google/cloud/hooks/vertex_ai/auto_ml.py Co-authored-by: Shahar Epstein <60007259+shahar1@users.noreply.github.com> * Update airflow/providers/google/cloud/operators/vertex_ai/auto_ml.py Co-authored-by: Shahar Epstein <60007259+shahar1@users.noreply.github.com> * Update airflow/providers/google/cloud/operators/vertex_ai/auto_ml.py Co-authored-by: Shahar Epstein <60007259+shahar1@users.noreply.github.com> * Update airflow/providers/google/cloud/operators/vertex_ai/auto_ml.py Co-authored-by: Shahar Epstein <60007259+shahar1@users.noreply.github.com> * Update test_vertex_ai.py * Update test_vertex_ai.py * Update auto_ml.py * Update airflow/providers/google/cloud/hooks/vertex_ai/auto_ml.py Co-authored-by: Josh Fell <48934154+josh-fell@users.noreply.github.com> * Update airflow/providers/google/cloud/operators/vertex_ai/auto_ml.py Co-authored-by: Andrey Anshin <Andrey.Anshin@taragol.is> * Update auto_ml.py * Update test_vertex_ai.py --------- Co-authored-by: Shahar Epstein <60007259+shahar1@users.noreply.github.com> Co-authored-by: Josh Fell <48934154+josh-fell@users.noreply.github.com> Co-authored-by: Andrey Anshin <Andrey.Anshin@taragol.is>
Added window_stride_length & window_max_count
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named
{pr_number}.significant.rst
or{issue_number}.significant.rst
, in newsfragments.