msrest-for-python icon indicating copy to clipboard operation
msrest-for-python copied to clipboard

Serialize np.float64 as float

Open mzat-msft opened this issue 3 years ago • 2 comments

On my machine np.float64 are the same as float, so I would expect that they are serialized in the same way. However, this is not the case as the logic in https://github.com/Azure/msrest-for-python/blob/af41991f17445e4cb26d39c247f091ddfd7027b9/msrest/serialization.py#L944 infer the type of the object's attribute using type rather than isinstance.

Do you think it makes sense to treat classes that are subclasses of basic types as basic types in the serialization process?

Here is a minimal working example of what I'm referring to. Note that np-float is cast to a str instead of a float.

import msrest.serialization
import numpy as np


class ModelWithNpFloat(msrest.serialization.Model):
    _attribute_map = {
        "state": {"key": "state", "type": "object"},
    }

    def __init__(self, state):
        self.state = state


model = ModelWithNpFloat(state={'np-float': np.float64(100)})
print(model.serialize())  # {'state': {'np-float': '100.0'}}

mzat-msft avatar Nov 16 '22 09:11 mzat-msft

This is a good point, I'd love to see our serialization supports numpy by default. We won't do it in msrest though as we are deprecating this lib to a new system, recent SDKs don't even install msrest anymore. I'll keep this issue open until we update the new system with it.

lmazuel avatar Nov 16 '22 16:11 lmazuel

Thanks for your feedback! I noticed that the package was deprecated only after opening the issue, but decided to keep the issue open for our reference as we have a package that depends on this issue.

Actually, I think that my suggestion is more connected to python's duck-typing. The issue with numpy is just one manifestation. What I mean is that for cases where a class is behaving as a built-in (i.e. being a subclass of a basic type) it might make sense to serialize them in the same way. In light of this I understand that the title of my issue is misleading.

mzat-msft avatar Nov 17 '22 09:11 mzat-msft