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.

Sign up for GitHub

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

Jump to bottom

[evaluation] ci: Enable mypy #37615

Merged
merged 102 commits into from
Oct 8, 2024
Merged

[evaluation] ci: Enable mypy #37615

merged 102 commits into from
Oct 8, 2024

Conversation

kdestin
Copy link
Member

@kdestin kdestin commented Sep 27, 2024 •
edited
Loading

Description

This pull request enables mypy in CI for azure-ai-evaluation and resolves outstanding type errors

All SDK Contribution checklist:

  • The pull request does not introduce [breaking changes]
  • CHANGELOG is updated for new features, bug fixes or other significant changes.
  • I have read the contribution guidelines.

General Guidelines and Best Practices

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

@kdestin kdestin requested a review from a team as a code owner September 27, 2024 19:46
@github-actions github-actions bot added the Evaluation Issues related to the client library for Azure AI Evaluation label Sep 27, 2024
@kdestin kdestin changed the title [evaluation] ci: Enable mypy [DO NOT MERGE][evaluation] ci: Enable mypy Sep 27, 2024
@azure-sdk
Copy link
Collaborator

azure-sdk commented Sep 27, 2024 •
edited
Loading

API change check

APIView has identified API level changes in this PR and created following API reviews.

azure-ai-evaluation

@kdestin kdestin force-pushed the evaluation/mypy/enable branch 4 times, most recently from 2e979f9 to 5897e7d Compare October 3, 2024 17:08
@kdestin kdestin changed the title [DO NOT MERGE][evaluation] ci: Enable mypy [evaluation] ci: Enable mypy Oct 3, 2024
@kdestin
fix(typing): Resolve mypy violations in azure/ai/evaluation/_http_uti…
401ad2d 
…ls.py
@kdestin
fix(typing): Resolve uses of implicit Optional in type annotations
035c15d
@kdestin
fix(typing): Resolve type reassignment in http_utils.py
07be192
@kdestin
style: Run isort
3ce1127
@kdestin
fix(typing): Fix attempted type reassignment in _f1_score.py
4e47899
@kdestin
fix(typing): Use a TypeGuard to allow mypy to narrow types in _common…
afcf2b5 
…/utils.py
@kdestin
fix(typing): Correct return type of get_harm_severity_level
2e65f4b
@kdestin
fix(typing): Correct return type of _compute_f1_score
dfadf99
@kdestin
fix(typing): Ensure mypy knows that AsyncHttpPipeline.__enter__ retur…
b0fddf5 
…ns Self
@kdestin
fix(typing): Allow mypy to infer the types of the convenience request…
3a9ab36 
… methods

   _http_utils.py extensively uses decorators to implement the
   "convenience" request methods (get, post, put, etc...) for
    {Async}HttpPipeline, since they all share a common underlying
    implementation.

    However neither decorator annotated its return type (the type of the
    decorated function). Initially this was because the accurate type
    couldn't be spelled using a `Callable`, and pylance still did
    a fine job providing intellisense.

    It turns out that it currently isn't possible to spell it
    with a callable typing.Protocol. Our decorator applies to a method,
    and mypy struggles with the removal of the `self` attribute that
    occurs when a method binds to an object (see python/mypy issue
    Azure#16200).

    This commit resolves this by making the implementation of the http
    pipelines more verbose, removing the decorators and unrolling the
    implementation to each convenience method.

    Using `Unpack[TypeDict]` to annotate kwargs makes this substantially
    more readable, but this causes mypy to complain if unknown keys
    are passed as kwargs (useful for request-specific pipeline
    configuration).
@kdestin
ci: Enable mypy in CI
b121eb0
@kdestin
fix(typing): Fix extranous total=False on TypedDict
584bd5b
@kdestin
fix(typing): Propagate model config type hint upwards
3cd9e6f
@kdestin
fix(typing): Ensure that len is only called on objects that impleme…
73b8067 
…nt Sized in _tracing.py
@kdestin
fix(typing): Resolve implicit optional type for Turn
1bf93c5
@kdestin
fix(typing): Resolve missing/inaccurate return types in simulator_dat…
ae95c9f 
…a_classes
@kdestin
fix(typing): Refine the TExperimental type for experimental()
2125280
@kdestin
fix(typing): Ignore the method assign in experimental decorator
56bedd9
@kdestin
fix(typing): Remove unnecessary optional for _add_method_docstring
06b1f7e
@kdestin
fix(typing): Correct Simulator__call__ task parameter to be List[str]
ef85339
@kdestin
fix(typing): evaluators Dict[str, Type] -> Dict[str, Callable]
b0443c0
@kdestin
fix(typing): Make CodeClient.get_metrics always return a dict
4f2e1ec
@kdestin
fix(typing): Add type annotations to evaluate/utils.py
9e1d600
@kdestin
fix(typing): Clarify that CodeRun.get_aggregated_metrics returns Dict…
2cf3c85 
…[str, Any]
@kdestin
fix(typing): data is a required parameter for _evaluate
8685ab6
@kdestin
fix(typing): Add variable annotations in _evaluate
4093604
@kdestin
fix(typing),refactor: Prevent batch_run_client from being Union[Proxy…
ea0cdc4 
…Client,CodeClient]

    Despite having similar interfaces with compatible calling conventions,
    the fact that ProxyClient and CodeClient have different "run" types
    (ProxyRun and CodeRun) causes type errors when dealing with a
    client of type Union[ProxyClient,CodeRun]. Mypy must consider the case
    when the wrong run type is used for a given client, despite that
    not being possible in this function.

    Refactoring the relevant code into a function allows us to clarify
    to mypy that client and run types are used consistently.
@kdestin
fix: Remove unused imports
db71385
@kdestin
fix(pylint): Resolve R1711(useless-return)
91206cd
@kdestin
fix(pylint): Resolve W0707(raise-missing-from)
9de0b8c
@kdestin
fix(pylint): Add parameters/returns to http_utils docstrings
8074271
@kdestin
fix(pylint): Make EvaluationMetrics implement CaseInsentitiveEnumMeta
206ef2b
@kdestin
fix: Remove return type annotations for Evaluators
ed684b1 
         Promptflow does reflection on type annotations, and only
         accepts a dataclass, typeddict, or string as return type
         annotation.
@kdestin
fix(typing): Add runtime validation of model_config
5e01c7e
@kdestin
fix: Remove type annotations from evaluator/simulators credential param
fae1092 
    Promptflow does reflection on type annotations and only allows
    dict
@kdestin
fix: Remove type annotations from azure_ai_project param
5588c5b 
    Promptflow does reflection on param types and disallows TypedDicts
@kdestin
fix(typing): {Azure,}OpenAIModelConfiguration.type is NotRequired
22ff116
@kdestin
fix(typing): List[Dict] -> list for conversation param
96a81f8
@kdestin kdestin merged commit a852079 into Azure:main Oct 8, 2024
37 checks passed
@kdestin kdestin mentioned this pull request Oct 17, 2024
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@luigiw luigiw luigiw approved these changes

Assignees
No one assigned
Labels
Evaluation Issues related to the client library for Azure AI Evaluation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kdestin @azure-sdk @luigiw