Azure / azure-sdk-for-python

This repository is for active development of the Azure SDK for Python. For consumers of the SDK we recommend visiting our public developer docs at https://learn.microsoft.com/python/azure/ or our versioned developer docs at https://azure.github.io/azure-sdk-for-python.
MIT License
4.64k stars 2.84k forks source link

`azure.ai.ml.entities._inputs_outputs._get_param_with_standard_annotation` doesn't support stringizized annotations enabled by `from __future__ import annotations`. #35538

Open mchikyt3 opened 6 months ago

mchikyt3 commented 6 months ago

Describe the bug I'm using mldesigner.command_component to convert a function into a command component that is used later in a pipeline. The inputs and outputs of the command components are determined by the type annotations of the parameters of the function. However, stringized annotations, which is enabled by from __future__ import annotations, can't be parsed.

After debugging, the problem can be traced down to #L334, where getattr(cls_or_func, "__annotations__", {}) returns a dictionary of type annotation strings. It can be replaced by a helper function typing.get_type_hints(cls_or_func, include_extras=True) to correctly evaluate the type back from its string form. The flag include_extras is set to True to correctly deal with the Annotated type.

To Reproduce Steps to reproduce the behavior:

  1. Run the following script:
    
    from __future__ import annotations

from azure.ai.ml.entities._inputs_outputs import _get_param_with_standard_annotation

def test_annotation(param: int): return

_get_param_with_standard_annotation(test_annotation, is_func=True)

2. The exception trace:

Exception has occurred: UserErrorException (note: full exception trace is shown but execution is paused at: _run_module_as_main) Unsupported annotation type 'int' for parameter 'param'. File "C:\Users\someone\Documents\VSCode.venvazureml\Lib\site-packages\azure\ai\ml\entities_inputs_outputs\utils.py", line 142, in _get_fields raise UserErrorException(msg) File "C:\Users\someone\Documents\VSCode.venvazureml\Lib\site-packages\azure\ai\ml\entities_inputs_outputs\utils.py", line 337, in _get_param_with_standard_annotation annotation_fields = _get_fields(annotations) ^^^^^^^^^^^^^^^^^^^^^^^^ File "C:\Users\someone\Documents\VSCode\test_annotation.py", line 10, in _get_param_with_standard_annotation(test_annotation, is_func=True) File "C:\Program Files\Python312\Lib\runpy.py", line 88, in _run_code exec(code, run_globals) File "C:\Program Files\Python312\Lib\runpy.py", line 198, in _run_module_as_main (Current frame) return _run_code(code, main_globals, None, ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ azure.ai.ml.exceptions.UserErrorException: Unsupported annotation type 'int' for parameter 'param'.



**Expected behavior**
The type should be correctly recognized.
mchikyt3 commented 6 months ago

This issue is related to stringizing annotations as per PEP 563 and PEP 649. The problem can be solved by test_annotation.__annotations__.update(typing.get_type_hints(test_annotation)) though, but it's not recommended in the Annotations Best Practices.

github-actions[bot] commented 6 months ago

Thanks for the feedback! We are routing this to the appropriate team for follow-up. cc @Azure/azure-ml-sdk @azureml-github.

diondrapeck commented 6 months ago

Hi @mchikyt3 - thank you for bringing this to our attention. I see your PR to address it. Please let me know if you need help getting the tests green. Once it is ready for review, I can review it and help you get it merged.

Thank you for contributing.

mchikyt3 commented 6 months ago

Hi, @diondrapeck. Thank you for your reply. Regarding the test for the PR, I've found that the original code doesn't pass (see commit 261e93ec75c0ce449d5e64dc7d7027b246ddb1ce). Please let me know if this is a known issue, or I've missed something. Thanks.

diondrapeck commented 6 months ago

Can you link to the specific test/error log where you're seeing that? I don't see it in the errors for your PR.

mchikyt3 commented 6 months ago

I have reverted my changes, only leaving an empty comment to trigger testing. You may check the errors now #35599. Thanks.

diondrapeck commented 6 months ago

Yes, I've confirmed this is a failure going on in main, unrelated to your PR. Please add your changes back and I'll review them. Once the main issue is fixed, we can get your PR tested objectively and merged. Thank you!

mchikyt3 commented 6 months ago

Thanks @diondrapeck. My changes are back now.