Standardize AWS Batch naming #20369
Standardize AWS Batch naming #20369
Conversation
| """ | ||
| A structured Protocol for ``boto3.client('batch') -> botocore.client.Batch``. | ||
| This is used for type hints on :py:meth:`.AwsBatchClient.client`; it covers | ||
| This is used for type hints on :py:meth:`.BatchClient.client`; it covers | ||
| only the subset of client methods required. |
There was a problem hiding this comment.
I'm not 100% sure if this one should have been left along. Please double check me here.
| class AwsBatchProtocol(BatchProtocol, Protocol): | ||
| """ | ||
| This class is deprecated. | ||
| Please use :class:`airflow.providers.amazon.aws.hooks.batch.BatchProtocol`. | ||
| """ | ||
|
|
||
| def __init__(self, *args, **kwargs): | ||
| warnings.warn( | ||
| "This class is deprecated. " | ||
| "Please use :class:`airflow.providers.amazon.aws.hooks.batch.BatchProtocol`.", | ||
| DeprecationWarning, | ||
| stacklevel=2, | ||
| ) | ||
| super().__init__(*args, **kwargs) |
There was a problem hiding this comment.
Please see this discussion before approving: #20332 (comment)
There was a problem hiding this comment.
probably not needed any more...
There was a problem hiding this comment.
That is correct, based on the other one now working I'll be doing this next. I'm not sure why that was giving me an error before, but it seems clear now. /shrug
There was a problem hiding this comment.
You were right. We need the protocol.
see a fix we had to deploy #20669
|
Now we are failing isort because of #20369 (comment) and removing the Protocol inheritance in 05abb3b causes the error we saw before about "only a Protocol can import a Protocol" I'm not sure what to do here. |
This reverts commit 05abb3b.
There was a problem hiding this comment.
The test failures related to files that that are missing in the PR:
https://github.com/apache/airflow/blob/main/airflow/contrib/operators/awsbatch_operator.py
airflow/tests/deprecated_classes.py
Lines 979 to 980 in cf5c311
You need to make changes on one of them or both depends on the needed fix. I think it's identical case to the Ecs so probably best to look at how the code looks there on Main barch and see if applying same fix here will solve the issue.
|
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. |
Part of #20296
In addition:
batchtoBatchwhere applicable.execute(None)andpoke(None)toexecute({})andpoke({})because my IDE was throwing a type error (expected a dict, got NoneType).^ 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.