Drop redundant project id null checks. #8145

jmcarp · 2020-04-04T22:08:23Z

The fallback_to_default_project_id method already raises an exception
if both self.project_id and kwargs["project_id"] are missing, so
skip redundant null checks in hook methods.

Make sure to mark the boxes below before creating PR: [x]

Description above provides context of the change
Unit tests coverage for changes (not needed for documentation changes)
Commits follow "How to write a good git commit message"
Relevant documentation is updated including usage instructions.
I will engage committers as explained in Contribution Workflow Example.

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.
Read the Pull Request Guidelines for more information.

turbaszek · 2020-04-05T10:24:00Z

@jmcarp those checks are only to please mypy. Initially, we used assert project_id but since we don't use assert in production code we have if clause...

potiuk · 2020-04-05T10:28:05Z

Just thinking - maybe we can do it differently and find a way to both - keep mypy happy and remove the ifs... Let me think about it a bit :)

turbaszek · 2020-04-05T10:29:20Z

@potiuk I think @mik-laj already gave it a lot of thought

potiuk · 2020-04-05T10:31:51Z

Some more would not hurt :)

turbaszek · 2020-04-05T10:49:26Z

Sure, would be awesome to remove it. But typehints with decorators are tricky :)

potiuk · 2020-04-05T10:52:06Z

Hold my 🍺 and 🐱

turbaszek · 2020-04-05T11:31:27Z

Hold my 🍺 and 🐱

Are you going to propose PEP to make Python statically typed language? 😅

potiuk · 2020-04-05T12:01:06Z

Yep. That's my secret plan. Just started to write it :)

jmcarp · 2020-04-05T17:31:15Z

I added a simple mypy plugin to preserve types for decorated functions, and to update types when decorators change function signatures. For example, fallback_to_default_project_id effectively changes the type of the project_id argument from str to Optional[str]. This also fixes the problem of decorators changing the types of all decorated callables to Callable[..., Any]. Still a WIP, but what do you think?

By the way, mypy might support this kind of behavior natively at some point, but this issue has been open for a long time: python/mypy#3157.

turbaszek · 2020-04-05T18:45:06Z

I have mixed feelings... First, I am not a big fan of adding more and more custom plugins (especially to solve problems that should be solved on language level #static_types_fan) . Especially to solve problems that I am not even sure are real problems. Since we have the if clause I even started to like the "explicit" information about project_id ("at this point, it will be provided by decorator").

On the other hand, this change does no harm. It makes our codebase bigger and adds next moving part to it. So, I am not sure.

I am curious about what others think? @mik-laj @kaxil @ashb @potiuk

ashb · 2020-04-05T18:54:40Z

Quick thought: The same issue about project_id also applies to session/provide_session

jmcarp · 2020-04-05T19:08:56Z

I was also hesitant about writing a mypy plugin, even a simple one, but as far as I can tell, it's the only way to write decorators that don't produce callables with type Callable[..., Any]. I also think it's not ideal that all decorated functions in the airflow codebase have that type. For example, users might see that PubSubHook.publish has type annotations and expect that mypy will catch type errors. But that's not the case, since the fallback_to_default_project_id decorator doesn't preserve function types. Adding the mypy plugin also caused the type checker to find some type errors in the codebase that it couldn't catch previously, again because decorated functions are effectively untyped.

jmcarp · 2020-04-05T22:40:57Z

One more thing: one of the mypy developers suggests using mypy plugins to preserve types for decorated functions until mypy supports this kind of thing: python/mypy#1927 (comment).

turbaszek · 2020-04-05T23:01:09Z

Quick thought: The same issue about project_id also applies to session/provide_session

As well as any other decorator that will inject arguments. @jmcarp will we be able to somehow check if all decorators that require it use this plugin? project_id is only GCP stuff an probably not so much problematic but provide_session is common and its case can be worth considering.

jmcarp · 2020-04-06T19:01:56Z

I don't know if we can programmatically audit use of the plugin, but the number of decorators is small and shouldn't change often, and it's simple enough to check by hand by grepping for wraps. I can add more decorators to the plugin here, but this patch is already pretty big, so I was thinking of adding provide_session and other decorators in a follow-up if we accept this change.

turbaszek · 2020-04-08T06:45:27Z

airflow/providers/google/cloud/example_dags/example_automl_tables.py

@@ -139,7 +139,6 @@ def get_target_column_spec(columns_specs: List[Dict], column_name: str) -> str:
        task_id="update_dataset_task",
        dataset=update,
        location=GCP_AUTOML_LOCATION,
-        project_id=GCP_PROJECT_ID,


Is this change necessary?

Yeah. I think we should leave that in.

This parameter was unused in CloudAutoMLHook.update_dataset, so I dropped the parameter from that method, as well as AutoMLTablesUpdateDatasetOperator and the example code here.

Yeah. I thik it's good :)

turbaszek · 2020-04-08T06:48:31Z

airflow/providers/google/cloud/hooks/bigtable.py

@@ -54,7 +54,7 @@ def _get_client(self, project_id: str):
        return self._client

    @GoogleBaseHook.fallback_to_default_project_id
-    def get_instance(self, instance_id: str, project_id: Optional[str] = None) -> Instance:
+    def get_instance(self, instance_id: str, project_id: str) -> Instance:


Let's decide what we want to use: str or Optional[str] = None and be cosistent

Yeah. Agree. project_id: str is better IMHO. it is semantically correct - we expect project_id to be set. And it is used most of the time below.

@jmcarp I checked the changes and in some places where we use the decorator we are using project_id: str and in others project_id: Optional[str] = None. I think we should be consistent :)

tests/providers/google/cloud/hooks/test_cloud_sql.py

potiuk

Few small thigs. I like it :)

potiuk · 2020-04-09T17:57:49Z

airflow/providers/google/cloud/example_dags/example_automl_tables.py

@@ -139,7 +139,6 @@ def get_target_column_spec(columns_specs: List[Dict], column_name: str) -> str:
        task_id="update_dataset_task",
        dataset=update,
        location=GCP_AUTOML_LOCATION,
-        project_id=GCP_PROJECT_ID,


Yeah. I think we should leave that in.

airflow/mypy/plugin/decorators.py

airflow/providers/google/cloud/hooks/automl.py

potiuk · 2020-04-09T18:02:49Z

airflow/providers/google/cloud/hooks/bigtable.py

@@ -54,7 +54,7 @@ def _get_client(self, project_id: str):
        return self._client

    @GoogleBaseHook.fallback_to_default_project_id
-    def get_instance(self, instance_id: str, project_id: Optional[str] = None) -> Instance:
+    def get_instance(self, instance_id: str, project_id: str) -> Instance:


Yeah. Agree. project_id: str is better IMHO. it is semantically correct - we expect project_id to be set. And it is used most of the time below.

airflow/providers/google/cloud/hooks/datacatalog.py

airflow/providers/google/cloud/hooks/stackdriver.py

airflow/providers/google/cloud/operators/automl.py

airflow/providers/google/cloud/operators/dataflow.py

tests/providers/google/cloud/hooks/test_cloud_sql.py

mik-laj · 2020-04-11T02:11:45Z

If you would like to add support for more decorators, the most popular decorator is `apply_defaullts. However, this will be best done in a separate PR. This is very problematic and has made it difficult for me to introduce metaclasses - #7450

potiuk · 2020-04-11T08:23:33Z

@turbaszek @mik-laj -> looks good to me. Can you take another look?

codecov-io · 2020-04-11T08:53:43Z

Codecov Report

Merging #8145 into master will decrease coverage by 0.40%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master    #8145      +/-   ##
==========================================
- Coverage   88.33%   87.93%   -0.41%     
==========================================
  Files         936      937       +1     
  Lines       45321    45197     -124     
==========================================
- Hits        40036    39743     -293     
- Misses       5285     5454     +169

Impacted Files	Coverage Δ
airflow/mypy/plugin/decorators.py	`0.00% <0.00%> (ø)`
...google/cloud/example_dags/example_automl_tables.py	`92.72% <ø> (ø)`
airflow/providers/google/cloud/hooks/automl.py	`98.94% <ø> (+10.42%)`	⬆️
...rflow/providers/google/cloud/hooks/bigquery_dts.py	`86.00% <ø> (+6.68%)`	⬆️
.../providers/google/cloud/hooks/cloud_memorystore.py	`74.07% <ø> (ø)`
...irflow/providers/google/cloud/hooks/datacatalog.py	`91.74% <ø> (ø)`
airflow/providers/google/cloud/hooks/datafusion.py	`84.53% <ø> (+3.22%)`	⬆️
airflow/providers/google/cloud/hooks/dataproc.py	`98.60% <ø> (ø)`
airflow/providers/google/cloud/hooks/dlp.py	`98.55% <ø> (ø)`
airflow/providers/google/cloud/hooks/mlengine.py	`82.74% <ø> (+1.27%)`	⬆️
... and 32 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 77412ac...f33e29e. Read the comment docs.

turbaszek · 2020-04-11T10:01:12Z

I still have concerns about type annotations consistency:
#8145 (comment)

turbaszek

LGTM

jmcarp · 2020-04-11T22:58:50Z

Thanks for reviewing. Is it all right to merge while TestCliTaskBackfill.test_run_ignores_all_dependencies is failing? It looks unrelated to this patch.

* Preserve types for decorators that don't change signatures * Augment types for decorators that change signatures * Drop redundant type checks

potiuk · 2020-04-13T07:50:28Z

Thanks @jmcarp! Great change!

zachliu · 2020-06-22T14:11:09Z

@jmcarp do you plan to work on apply_defaults? 😁

boring-cyborg bot added the provider:google Google (including GCP) related issues label Apr 4, 2020

jmcarp force-pushed the drop-redundant-project-check branch from 01499ea to 37c79f6 Compare April 4, 2020 22:46

jmcarp force-pushed the drop-redundant-project-check branch from 37c79f6 to 325f379 Compare April 5, 2020 17:28

jmcarp force-pushed the drop-redundant-project-check branch 2 times, most recently from 8b41a5a to 988afe8 Compare April 5, 2020 18:27

jmcarp force-pushed the drop-redundant-project-check branch 7 times, most recently from aae4fb9 to e5ece17 Compare April 5, 2020 22:24

jmcarp force-pushed the drop-redundant-project-check branch 4 times, most recently from 4eced5c to 1161ea6 Compare April 6, 2020 00:19

jmcarp force-pushed the drop-redundant-project-check branch 3 times, most recently from 6f29451 to c0983b4 Compare April 6, 2020 16:49

jmcarp force-pushed the drop-redundant-project-check branch from c0983b4 to 0f6020a Compare April 6, 2020 19:23

turbaszek reviewed Apr 8, 2020

View reviewed changes

tests/providers/google/cloud/hooks/test_cloud_sql.py Show resolved Hide resolved

potiuk requested changes Apr 9, 2020

View reviewed changes

jmcarp force-pushed the drop-redundant-project-check branch from 0f6020a to e0f61d7 Compare April 11, 2020 01:06

potiuk approved these changes Apr 11, 2020

View reviewed changes

jmcarp force-pushed the drop-redundant-project-check branch from e0f61d7 to f8d4c4b Compare April 11, 2020 14:51

boring-cyborg bot added the k8s label Apr 11, 2020

turbaszek approved these changes Apr 11, 2020

View reviewed changes

jmcarp force-pushed the drop-redundant-project-check branch 3 times, most recently from fde9de9 to 81e549a Compare April 11, 2020 20:07

jmcarp force-pushed the drop-redundant-project-check branch 2 times, most recently from e032024 to 535efb5 Compare April 12, 2020 21:16

Add mypy plugin for decorators.

f33e29e

* Preserve types for decorators that don't change signatures * Augment types for decorators that change signatures * Drop redundant type checks

jmcarp force-pushed the drop-redundant-project-check branch from 535efb5 to f33e29e Compare April 12, 2020 21:39

potiuk merged commit 1fd9ed3 into apache:master Apr 13, 2020

mik-laj mentioned this pull request Jul 8, 2020

Increase typing coverage for Apache and http provider package #9729

Merged

6 tasks

Drop redundant project id null checks. #8145

Drop redundant project id null checks. #8145

Uh oh!

Conversation

jmcarp commented Apr 4, 2020

Uh oh!

turbaszek commented Apr 5, 2020

Uh oh!

potiuk commented Apr 5, 2020

Uh oh!

turbaszek commented Apr 5, 2020

Uh oh!

potiuk commented Apr 5, 2020

Uh oh!

turbaszek commented Apr 5, 2020

Uh oh!

potiuk commented Apr 5, 2020

Uh oh!

turbaszek commented Apr 5, 2020

Uh oh!

potiuk commented Apr 5, 2020

Uh oh!

jmcarp commented Apr 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

turbaszek commented Apr 5, 2020

Uh oh!

ashb commented Apr 5, 2020

Uh oh!

jmcarp commented Apr 5, 2020

Uh oh!

jmcarp commented Apr 5, 2020

Uh oh!

turbaszek commented Apr 5, 2020

Uh oh!

jmcarp commented Apr 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

potiuk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mik-laj commented Apr 11, 2020

Uh oh!

potiuk commented Apr 11, 2020

Uh oh!

codecov-io commented Apr 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

turbaszek commented Apr 11, 2020

Uh oh!

turbaszek left a comment

Choose a reason for hiding this comment

Uh oh!

jmcarp commented Apr 5, 2020 •

edited

Loading

codecov-io commented Apr 11, 2020 •

edited

Loading