Skip to content

Conversation

@Amichelangelo
Copy link
Member

Description

Please add an informative description that covers that changes made by the pull request and link all relevant issues.

If an SDK is being regenerated based on a new API spec, a link to the pull request containing these API spec changes should be included above.

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.

@sarkar-rajarshi
Copy link
Member

/azp run python - cognitivelanguage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sarkar-rajarshi
Copy link
Member

After the move to TypeSpec, Authoring and Inference API would be in separate artifacts. There shouldn't be any authoring folder in this package if this being generated from typespec correctly. When generating the code, could you please locally delete the existing folder of azure-ai-language-questionanswering and run the codegen after that

@github-actions
Copy link

github-actions bot commented Sep 9, 2025

API Change Check

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

azure-ai-language-questionanswering

@Amichelangelo Amichelangelo changed the title (Python) CQA inference 2025-05-15-preview [SDK Release] (Python) CQA inference 2025-05-15-preview Sep 10, 2025
@Amichelangelo
Copy link
Member Author

@microsoft-github-policy-service agree company="Microsoft"

@sarkar-rajarshi
Copy link
Member

/azp run python - cognitivelanguage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Amichelangelo
Copy link
Member Author

/azp run python - cognitivelanguage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

…ytypes score

The verifytypes pipeline failed because type completeness for azure-ai-language-questionanswering dropped from 100% (main) to 87.9%. Pyright reported missing / ambiguous types for:

QuestionAnsweringClient.question_answering (sync + async clients)
QuestionAnsweringOperations.init (sync + async)
Derived patch classes were then flagged as having partially unknown bases.
Changes:

Added explicit return/attribute annotation for question_answering in _client.py and _client.py.
Added *args: Any, **kwargs: Any plus explicit internal attribute types in _operations.py and _operations.py.
Result:

Eliminates Pyright errors about missing annotations and partially unknown base classes.
Restores type completeness score to parity with main (expected 100%).
No runtime or public API behavior changes; typing-only improvement.
Follow-up (optional):

Consider templating these annotations in the code generator to avoid future regeneration loss
Copy link
Member

@sarkar-rajarshi sarkar-rajarshi left a comment

Choose a reason for hiding this comment

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

Lets check how we are generating code. The generated client does not look correct

@Amichelangelo
Copy link
Member Author

/azp run python - cognitivelanguage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

* Authoring functionality (project creation, knowledge source management, deployment operations) has been removed from this package and moved to a separate dedicated authoring package / namespace. All references to `AuthoringClient`, and related authoring operations have been eliminated from the runtime client distribution.
* Dropped support for Python versions earlier than 3.9 (aligns with Azure SDK Python support policy going forward).
* Model base class change: all public model types now inherit from `MutableMapping[str, Any]` (dict-like) instead of the previous `Model` base class. As a result they now support standard mutable mapping behavior (key iteration, item assignment, etc.) and any code depending on methods/properties inherited from the old base class should be reviewed/updated.

Copy link
Member

Choose a reason for hiding this comment

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

Can we add that default_language has been removed, and give a link to how to customers can change the service default instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

sure

Copy link
Member

Choose a reason for hiding this comment

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

Is there an actual guide/documentation that we can link to about how to configure this at the project level? Or in the portal?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just check the service side, the default_language is hardcoded as "en",can not be configured at the project level, remove related line in changelog

super().__init__(
question=question, text_documents=cast(List[TextDocument], text_documents), language=language, **kwargs
)
self.string_index_type = "UnicodeCodePoint"
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason we're no longer providing this default?

Copy link
Member Author

Choose a reason for hiding this comment

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

I remember it is not needed, but I'm not sure now. Add this back

Copy link
Member

Choose a reason for hiding this comment

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

Let's make a note to confirm this before GA.

Copy link
Member Author

Choose a reason for hiding this comment

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

this is useful, add it back

class MetadataFilter(_GeneratedMetadataFilter):
"""Backward compatible MetadataFilter supporting legacy tuple/dict inputs."""

def __init__(self, *args: Any, **kwargs: Any) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

We're missing the overloads for this model.
I'm also not sure if all the documentation will be inherited - so we should check how it renders.

Copy link
Member Author

Choose a reason for hiding this comment

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

add the overloads and update the documentation

Copy link
Member

@annatisch annatisch Nov 4, 2025

Choose a reason for hiding this comment

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

I think we're still missing the base overload that just supports a raw mapping? (You can find an example of this on all the generated models) - something like:

    @overload
    def __init__(self, mapping: Mapping[str, Any]) -> None:
        ...

Copy link
Member Author

@Amichelangelo Amichelangelo Nov 5, 2025

Choose a reason for hiding this comment

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

delete this patch file, it's false added

Copy link
Contributor

Copilot AI commented Nov 4, 2025

@Amichelangelo I've opened a new pull request, #43778, to work on those changes. Once the pull request is ready, I'll request review from you.

Copy link
Contributor

Copilot AI commented Nov 4, 2025

@Amichelangelo I've opened a new pull request, #43779, to work on those changes. Once the pull request is ready, I'll request review from you.

from __future__ import annotations

from collections.abc import Iterable, Mapping, MutableMapping
from typing import Any, Optional, Sequence, Tuple, Union, Mapping as TypingMapping, List, overload
Copy link
Member

Choose a reason for hiding this comment

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

You don't need separate imports of Mapping from typing vs collections.abc - they are aliases for the same object:
https://docs.python.org/3/library/typing.html#typing.Mapping

So you only need to import once from collections.abc

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

example, use "en" for English; "es" for Spanish etc. If not set, use "en" for English as
default.
:vartype language: str
metadata: Sequence[Union[Tuple[str, str], MetadataRecord, TypingMapping[str, str]]],
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't support a union sequence like this - I don't think it's too much to ask that users provide a homogenous list. Keeps things a bit more simple.

Copy link
Member Author

Choose a reason for hiding this comment

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

you're right, fix it

class MetadataFilter(_GeneratedMetadataFilter):
"""Backward compatible MetadataFilter supporting legacy tuple/dict inputs."""

def __init__(self, *args: Any, **kwargs: Any) -> None:
Copy link
Member

@annatisch annatisch Nov 4, 2025

Choose a reason for hiding this comment

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

I think we're still missing the base overload that just supports a raw mapping? (You can find an example of this on all the generated models) - something like:

    @overload
    def __init__(self, mapping: Mapping[str, Any]) -> None:
        ...

super().__init__(
question=question, text_documents=cast(List[TextDocument], text_documents), language=language, **kwargs
)
self.string_index_type = "UnicodeCodePoint"
Copy link
Member

Choose a reason for hiding this comment

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

Let's make a note to confirm this before GA.

self.string_index_type = "UnicodeCodePoint"
# _attribute_map may not exist if underlying generator changed; guard accordingly.
try: # pragma: no cover - defensive
self._attribute_map.update({"string_index_type": {"key": "stringIndexType", "type": "str"}})
Copy link
Member

Choose a reason for hiding this comment

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

Rather than do this, shouldn't we do something like:

    def __init__(
        self,
        *,
        question: str,
        text_documents: Sequence[Union[str, TextDocument]],
        language: Optional[str] = None,
        **kwargs: Any,
    ) -> None:
        string_index_type = kwargs.pop("string_index_type", "UnicodeCodePoint")
        super().__init__(question=question, text_documents=normalized, language=language, string_index_type=string_index_type)

Copy link
Member Author

@Amichelangelo Amichelangelo Nov 5, 2025

Choose a reason for hiding this comment

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

from the changelog, the string_index_type is hardcoded to Utf16CodeUnit

Copy link
Member Author

Choose a reason for hiding this comment

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

we don't need to use kwargs.pop("string_index_type", "UnicodeCodePoint"), it's designed to use hardcode UnicodeCodePoint

@Amichelangelo Amichelangelo enabled auto-merge (squash) November 5, 2025 12:40
@Amichelangelo
Copy link
Member Author

/azp run python - cognitivelanguage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Amichelangelo
Copy link
Member Author

/azp run python - cognitivelanguage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

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.

5 participants