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

use ParamSpec to enable intellisense with distributed_trace decorators #22891

Merged
merged 7 commits into from Mar 1, 2022

Conversation

kristapratico
Copy link
Member

@kristapratico kristapratico commented Feb 3, 2022

This PR is related to this discussion on Teams where we noticed that our distributed_trace decorators were blocking the intellisense type help. Fortunately there is a solution to this called typing.ParamSpec which forwards the parameter types of one callable to another callable and restores our type hinting!

ParamSpec: https://docs.python.org/3/library/typing.html#typing.ParamSpec

This only works for py3.10+, but does enable intellisense as expected: This works for all supported Python versions.

VSCode/pylance:

image

Pycharm:

image

@kristapratico kristapratico self-assigned this Feb 3, 2022
@ghost ghost added the Azure.Core label Feb 3, 2022
@azure-sdk
Copy link
Collaborator

API changes have been detected in azure-cosmos. You can review API changes here

@azure-sdk
Copy link
Collaborator

API changes have been detected in azure-core. You can review API changes here

API changes

- def azure.core.tracing.decorator.distributed_trace(__func = None, **kwargs) -> Callable[, T]
+ def azure.core.tracing.decorator.distributed_trace(__func: Callable[P, T] = None, **kwargs: Any) -> Callable[P, T]
- def azure.core.tracing.decorator_async.distributed_trace_async(__func: Callable[Ellipsis, Awaitable[T]] = None, **kwargs: Any) -> Callable[, Awaitable[T]]
+ def azure.core.tracing.decorator_async.distributed_trace_async(__func: Callable[P, Awaitable[T]] = None, **kwargs: Any) -> Callable[P, Awaitable[T]]

@azure-sdk
Copy link
Collaborator

API changes have been detected in azure-core. You can review API changes here

@azure-sdk
Copy link
Collaborator

API changes have been detected in azure-identity. You can review API changes here

API changes

+     def __str__(self)
+ 
+ 
+     def __str__(self)

@azure-sdk
Copy link
Collaborator

API changes have been detected in azure-cosmos. You can review API changes here

@azure-sdk
Copy link
Collaborator

API changes have been detected in azure-data-tables. You can review API changes here

API changes

+     def __str__(self)
+ 
+ 
+     def __str__(self)

@azure-sdk
Copy link
Collaborator

API changes have been detected in azure-appconfiguration. You can review API changes here

API changes

+     def __str__(self)
+ 

@azure-sdk
Copy link
Collaborator

API changes have been detected in azure-communication-networktraversal. You can review API changes here

@kristapratico
Copy link
Member Author

kristapratico commented Feb 4, 2022

We run mypy==0.782 in the CI and it erroneously calls out these errors in this PR:

azure/core/tracing/decorator_async.py:31: error: Module 'typing_extensions' has no attribute 'ParamSpec'
azure/core/tracing/decorator_async.py:41: error: The first argument to Callable must be a list of types or "..."
azure/core/tracing/decorator_async.py:42: error: The first argument to Callable must be a list of types or "..."
azure/core/tracing/decorator_async.py:49: error: The first argument to Callable must be a list of types or "..."
azure/core/tracing/decorator_async.py:54: error: The first argument to Callable must be a list of types or "..."
azure/core/tracing/decorator_async.py:66: error: The first argument to Callable must be a list of types or "..."
azure/core/tracing/decorator.py:31: error: Module 'typing_extensions' has no attribute 'ParamSpec'
azure/core/tracing/decorator.py:41: error: The first argument to Callable must be a list of types or "..."
azure/core/tracing/decorator.py:48: error: The first argument to Callable must be a list of types or "..."
azure/core/tracing/decorator.py:53: error: The first argument to Callable must be a list of types or "..."
azure/core/tracing/decorator.py:65: error: The first argument to Callable must be a list of types or "..."

Running with the latest release (mypy==0.931) does not raise these errors. Options I see...

  1. bump mypy version across the repo. Every package opted-in to mypy checks might start seeing some new errors which will need resolve: https://github.com/Azure/azure-sdk-for-python/blob/main/eng/tox/mypy_hard_failure_packages.py#L8

2) type: ignore these specific lines.

3) Add a mypy.ini to azure-core to exclude these files from mypy checks for now.

4) Is there a way to run a different version of mypy only on azure-core? Other options?

@kristapratico
Copy link
Member Author

kristapratico commented Feb 4, 2022

Output on mypy opted-in packages using mypy==0.931 and Python 3.10

azure-core -
azure\core\settings.py:164: error: Incompatible return value type (got "Union[str, Type[AbstractSpan]]", expected "Optional[Type[AbstractSpan]]")
azure\core_pipeline_client.py:31: error: Module "collections" has no attribute "Iterable"
azure\core_pipeline_client.py:31: error: Name "Iterable" already defined (possibly by an import)

azure-eventhub -
azure\eventhub_client_base.py:49: error: First argument to namedtuple() should be "_Address", not "Address"

azure-keyvault-certificates -
azure\keyvault\certificates_shared_polling.py:50: error: Argument 1 of "wait" is incompatible with supertype "LROPoller"; supertype defines the argument type as "Optional[float]"
azure\keyvault\certificates_shared_polling.py:50: note: This violates the Liskov substitution principle
azure\keyvault\certificates_shared_polling.py:50: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides

azure-keyvault-keys -
azure\keyvault\keys_shared_polling.py:50: error: Argument 1 of "wait" is incompatible with supertype "LROPoller"; supertype defines the argument type as "Optional[float]"
azure\keyvault\keys_shared_polling.py:50: note: This violates the Liskov substitution principle
azure\keyvault\keys_shared_polling.py:50: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides

azure-keyvault-secrets -
azure\keyvault\secrets_shared_polling.py:50: error: Argument 1 of "wait" is incompatible with supertype "LROPoller"; supertype defines the argument type as "Optional[float]"
azure\keyvault\secrets_shared_polling.py:50: note: This violates the Liskov substitution principle
azure\keyvault\secrets_shared_polling.py:50: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides

azure-servicebus -
azure\servicebus_servicebus_receiver.py:695: error: Argument 1 to "len" has incompatible type "Union[int, List[int]]"; expected "Sized"
azure\servicebus_servicebus_receiver.py:705: error: Item "int" of "Union[int, List[int]]" has no attribute "iter" (not iterable)
azure\servicebus\aio_servicebus_receiver_async.py:681: error: Argument 1 to "len" has incompatible type "Union[int, List[int]]"; expected "Sized"
azure\servicebus\aio_servicebus_receiver_async.py:691: error: Item "int" of "Union[int, List[int]]" has no attribute "iter" (not iterable)

azure-ai-metricsadvisor -
azure\ai\metricsadvisor_metrics_advisor_administration_client.py:642: error: syntax error in type comment "(*str, Any) -> None"
azure\ai\metricsadvisor_metrics_advisor_administration_client.py:668: error: syntax error in type comment "(*str, Any) -> None"
azure\ai\metricsadvisor_metrics_advisor_administration_client.py:694: error: syntax error in type comment "(*str, Any) -> None"
azure\ai\metricsadvisor_metrics_advisor_administration_client.py:718: error: syntax error in type comment "(*str, Any) -> None"
azure\ai\metricsadvisor_metrics_advisor_administration_client.py:1472: error: syntax error in type comment "(*str, Any) -> None"

azure-identity - No changes needed
azure-keyvault-administration - No changes needed
azure-ai-textanalytics - No changes needed
azure-ai-formrecognizer - No changes needed
azure-ai-translation-document - No changes needed
azure-eventgrid - No changes needed
azure-appconfiguration - No changes needed
azure-data-tables - No changes needed
azure-mixedreality-remoterendering - No changes needed

@azure-sdk
Copy link
Collaborator

API changes have been detected in azure-cosmos. You can review API changes here

@azure-sdk
Copy link
Collaborator

API changes have been detected in azure-core. You can review API changes here

@kristapratico kristapratico changed the title PoC - use ParamSpec to enable intellisense with distributed_trace decorators use ParamSpec to enable intellisense with distributed_trace decorators Feb 24, 2022
@kristapratico kristapratico marked this pull request as ready for review February 24, 2022 21:19
@lmazuel lmazuel merged commit 1912da9 into main Mar 1, 2022
@lmazuel lmazuel deleted the fix-intellisense-typing branch March 1, 2022 23:49
rakshith91 pushed a commit to rakshith91/azure-sdk-for-python that referenced this pull request Apr 7, 2022
Azure#22891)

* use ParamSpec in distributed_trace decorators to enable intellisense in py3.10+

* use backported ParamSpec from typings

* add dependency on typing_extensions

* fix name, remove from dev_reqs

* fix typing-extensions version

* update changelog
rakshith91 pushed a commit to rakshith91/azure-sdk-for-python that referenced this pull request Apr 10, 2022
Azure#22891)

* use ParamSpec in distributed_trace decorators to enable intellisense in py3.10+

* use backported ParamSpec from typings

* add dependency on typing_extensions

* fix name, remove from dev_reqs

* fix typing-extensions version

* update changelog
@xiaoyongzhu
Copy link

@kristapratico seems this PR introduces some import issue in databricks ML runtime (see this issue: feathr-ai/feathr#154). Also seems like ParamSpec is added in 3.10 (https://docs.python.org/3/library/typing.html#typing.ParamSpec) so maybe we should also have a way to relax this limitation?

@xiaoyongzhu
Copy link

And can be repoduced by this (in databricks ML 10.2):
!pip install azure-core==1.23.1
from azure.core.tracing.decorator import distributed_trace

and will throw out this error:


---------------------------------------------------------------------------
ImportError                               Traceback (most recent call last)
<command-3599966980702568> in <module>
----> 1 from azure.core.tracing.decorator import distributed_trace

/databricks/python_shell/dbruntime/PythonPackageImportsInstrumentation/__init__.py in import_patch(name, globals, locals, fromlist, level)
    160             # Import the desired module. If you’re seeing this while debugging a failed import,
    161             # look at preceding stack frames for relevant error information.
--> 162             original_result = python_builtin_import(name, globals, locals, fromlist, level)
    163 
    164             is_root_import = thread_local._nest_level == 1

/databricks/python/lib/python3.8/site-packages/azure/core/tracing/decorator.py in <module>
     29 
     30 from typing import Callable, Any, TypeVar, overload
---> 31 from typing_extensions import ParamSpec
     32 from .common import change_context, get_function_and_class_name
     33 from ..settings import settings

ImportError: cannot import name 'ParamSpec' from 'typing_extensions' (/databricks/python/lib/python3.8/site-packages/typing_extensions.py)

@kristapratico
Copy link
Member Author

@xiaoyongzhu moving this conversation to #23697

@glaubitz
Copy link

@kristapratico @lmazuel Is there a particular reason why you are depending on typing_extensions >= 4.0.1? Would version 3.10.0.0 work as well?

I am asking because I am trying to upgrade azure-storage-blob in SUSE SLE-15-SP1 to 12.13.1 which requires azure-core 1.23.1 which in turn introduced the dependency on typing_extensions.

We currently have typing_extensions 3.10.0.0 in SLE-15-SP1 and upgrading that to 4.0.1 would be rather difficult as that pulls in a number of new Python packages, so if we could stay with 3.10.0.0, it would make the upgrade for us much easier.

I did some tests with the Azure SDK and CLI with typing_extensions 3.10.0.0 and it seems to work but I am not 100% confident.

@kristapratico
Copy link
Member Author

kristapratico commented Oct 17, 2022

Hey @glaubitz, if I recall correctly ParamSpec (which azure-core==1.23.1 uses) was listed as an experimental feature in 3.10.0.0. I wouldn't expect any runtime errors, but I'm not sure about typing degradation due to the older version.

@glaubitz
Copy link

@kristapratico Thanks for the answer! Do you know whether is a way to reliably test the compatibility?

@kristapratico
Copy link
Member Author

@kristapratico Thanks for the answer! Do you know whether is a way to reliably test the compatibility?

@glaubitz here's what I did to test compatibility.

  • created a virtual environment with Python 3.7 and installed the following versions:

    • azure-core==1.23.1
    • azure-storage-blob==12.13.1
    • typing-extensions==3.10.0.0
    • mypy==0.982
  • created a simple sample with the azure-storage-blob BlobServiceClient where we pass in the wrong type for the parameter analytics_logging:

from azure.storage.blob import BlobServiceClient
client = BlobServiceClient(account_url="x")

client.set_service_properties(analytics_logging=True)
  • ran mypy on the above code and got an error: Argument "analytics_logging" to "set_service_properties" of "BlobServiceClient" has incompatible type "bool"; expected "Optional[BlobAnalyticsLogging]"

If you see the typing error, then typing is working correctly with the older version of typing-extensions (3.10.0.0) and using ParamSpec to forward the argument types to the client method. If you don't see the error (parameter gets typed as Any), then the typing experience is degraded with the older version. Note that I only tested this with Python 3.7 (although it appears to work on machine).

@glaubitz
Copy link

@kristapratico Wow, thanks a lot! This is incredibly helpful! I will perform these tests with the various Python versions.

@glaubitz
Copy link

glaubitz commented Nov 2, 2022

Apologies for the late reply. I performed the check with mypy on Python3.6 and typing_extensions == 3.10.0.0 and the type check fails as expected:

suse_gce@sle-15-jpag-test:~> mypy azure_blob_test.py 
azure_blob_test.py:4: error: Argument "analytics_logging" to "set_service_properties" of "BlobServiceClient" has incompatible type "bool"; expected "Optional[BlobAnalyticsLogging]"
Found 1 error in 1 file (checked 1 source file)
suse_gce@sle-15-jpag-test:~>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants