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

Serialization from_dict modifies input dictionary #2799

Open
pvaneck opened this issue Aug 27, 2024 · 4 comments · May be fixed by #2430
Open

Serialization from_dict modifies input dictionary #2799

pvaneck opened this issue Aug 27, 2024 · 4 comments · May be fixed by #2430
Assignees

Comments

@pvaneck
Copy link
Member

pvaneck commented Aug 27, 2024

In the generated serialization.py file, if a user tries to call from_dict on a model that has _subtype_map values, those values are popped from the original dictionary. See here.

Here is a sample issue showcasing this behavior: Azure/azure-sdk-for-python#37024

Here is also a simpler recreation:

from azure.search.documents.indexes._generated.models import VectorSearchAlgorithmConfiguration


data = {
    "name": "hnsw",
    "kind": "hnsw",
}
config = VectorSearchAlgorithmConfiguration.from_dict(algo)
print(data) # "kind" key will now be missing.

VectorSearchAlgorithmConfiguration has:

_subtype_map = {
    "kind": {"exhaustiveKnn": "ExhaustiveKnnAlgorithmConfiguration", "hnsw": "HnswAlgorithmConfiguration"}
}

I feel like the input dictionary should remain unmodified. Is mutating the input dictionary intentional?

@iscai-msft
Copy link
Contributor

@pvaneck this might have been accidentally introduced bc we're optimizing for our new models, and with our new models we don't have anything like subtype_map. Let me take a look at this today

@tadelesh
Copy link
Member

it is a same issue to this. but in that issue, we just fix the impact for lro operation, not fix the root problem. @msyyc do you remember why we need to pop the discriminator from the sub type map?

@msyyc
Copy link
Member

msyyc commented Aug 30, 2024

We ever had discussion with Laurent #2430 (comment) that it might bring risks if change the deserialization logic of msrest. Let me make up some complicated test to check it, if passed, I think we had better fix it directly like https://github.com/Azure/autorest.python/pull/2430/files

@msyyc msyyc linked a pull request Aug 30, 2024 that will close this issue
@msyyc msyyc self-assigned this Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants