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.

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

Google Cloud Providers - Fix _MethodDefault deepcopy failure #29518

Merged
merged 1 commit into from
Mar 4, 2023

Conversation

IKholopov
Copy link
Contributor

@IKholopov IKholopov commented Feb 13, 2023

This is the attempt to fix #28751 by setting a memo to the same instance of DEFAULT, which is the stub for a Literal value type introduced to support Python 3.7 with mypy in Google's API core Python client.

closes: #28751

@boring-cyborg boring-cyborg bot added provider:cncf-kubernetes Kubernetes provider related issues area:providers provider:google Google (including GCP) related issues labels Feb 13, 2023
@IKholopov IKholopov marked this pull request as draft February 13, 2023 23:07
@IKholopov IKholopov force-pushed the ikholopov/deepcopy-google-operators branch from dec03c2 to bf92064 Compare February 14, 2023 00:20
Comment on lines 26 to 30
class GoogleBaseOperator(BaseOperator):
"""
Abstract base class that takes care of common specifics of the operators built
on top of Google API client libraries.
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Our goal is to split Google provider to smaller providers #15933
This class seems to be GCP specific so lets keep it in the cloud directory and not in common as this directory will not stay after the split.

Possibly also rename the class toGcpBaseOperator ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, especially since AFAIU only Cloud providers are affected. GCP is a bit of a deprecated term, but "Google Cloud Base" should do that.

@IKholopov IKholopov force-pushed the ikholopov/deepcopy-google-operators branch 6 times, most recently from 67d97d5 to 67bd5be Compare February 19, 2023 11:50
@IKholopov IKholopov changed the title [DRAFT] Google Providers - Fix _MethodDefault deepcopy failure Google Providers - Fix _MethodDefault deepcopy failure Feb 19, 2023
@IKholopov IKholopov marked this pull request as ready for review February 19, 2023 12:09
"""

def __deepcopy__(self, memo):
memo[id(DEFAULT)] = DEFAULT
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to add explanation why we are doing it.

Copy link
Member

Choose a reason for hiding this comment

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

(Link to some description/issue explaining it because otherwise it is pretty magical.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@potiuk
Copy link
Member

potiuk commented Feb 20, 2023

@IKholopov - can you please split this one in two PRs:

  1. introduce base class
  2. adding deepcopy

That will be much better when it comes to historical look at the changes. It's quite hard to find the deepcopy fix among all the changed base operators.

@IKholopov IKholopov force-pushed the ikholopov/deepcopy-google-operators branch 2 times, most recently from b1a5080 to 5cc71b1 Compare February 21, 2023 23:17
@IKholopov IKholopov changed the title Google Providers - Fix _MethodDefault deepcopy failure Google Cloud Providers - Fix _MethodDefault deepcopy failure Feb 21, 2023
@IKholopov
Copy link
Contributor Author

Sounds good, created #29680

@IKholopov IKholopov force-pushed the ikholopov/deepcopy-google-operators branch 2 times, most recently from a6bdccc to 5ac4d75 Compare February 24, 2023 23:40
@eladkal
Copy link
Contributor

eladkal commented Feb 25, 2023

Sounds good, created #29680

merged.

This is the attempt to fix apache#28751 by setting a memo to the same instance of DEFAULT,
which is the stub for a Literal value type introduced to support Python 3.7 with mypy
in Google's API core Python client.

Without this any operator that has the parameter set to DEFAULT constant
(which is often the default value for retry parameters) will throw the
error in mini-scheduler after execution as the attemt to deepcopy this
project would fail.
@IKholopov IKholopov force-pushed the ikholopov/deepcopy-google-operators branch from 5ac4d75 to 9c53f94 Compare February 25, 2023 16:26
@ikholopov-omni
Copy link
Contributor

@potiuk, any other feedback that you would like me to address or are we good to merge?

@potiuk potiuk merged commit 5a632f7 into apache:main Mar 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers provider:cncf-kubernetes Kubernetes provider related issues provider:google Google (including GCP) related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

KubernetesExecutor leaves failed pods due to deepcopy issue with Google providers
4 participants