-
Notifications
You must be signed in to change notification settings - Fork 26
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
Deprecate SageMakerTrainingOperatorAsync #1463
Conversation
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.
Also leaving a comment to remember updating the provider version once it's released.
534951a
to
aa734a8
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1463 +/- ##
==========================================
- Coverage 98.27% 98.24% -0.04%
==========================================
Files 91 91
Lines 4648 4609 -39
==========================================
- Hits 4568 4528 -40
- Misses 80 81 +1 ☔ View full report in Codecov by Sentry. |
aa734a8
to
10a3c8f
Compare
if event and event["status"] == "error": | ||
raise AirflowException(event["message"]) | ||
raise AirflowException("No event received in trigger callback") | ||
super().__init__(deferrable=True, **kwargs) |
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.
shall we pass args too?
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.
this is not needed, the upstream operator has no positional args. It only has named/keyword args.
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
setup.cfg
Outdated
# Update version when the below RC is released | ||
apache-airflow-providers-amazon>=8.18.0rc1 |
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.
# Update version when the below RC is released | |
apache-airflow-providers-amazon>=8.18.0rc1 | |
apache-airflow-providers-amazon>=8.18.0 |
setup.cfg
Outdated
# Update version when the below RC is released | ||
apache-airflow-providers-amazon>=8.18.0rc1 |
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.
# Update version when the below RC is released | |
apache-airflow-providers-amazon>=8.18.0rc1 | |
apache-airflow-providers-amazon>=8.18.0 |
35f1b4c
to
2f23555
Compare
eff7b31
to
605f33d
Compare
I will merge this so that we can test our regressions DAGs together on all RCs and create a separate PR later to update the versions in |
Deprecate SageMakerTrainingOperatorAsync and proxy them to their Airflow OSS provider's counterpart.
this needs to wait for the release of apache/airflow#36685