Skip to content

AttrDict.to_dict() should be recursive #291

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

Closed
marcinn opened this issue Nov 9, 2015 · 11 comments
Closed

AttrDict.to_dict() should be recursive #291

marcinn opened this issue Nov 9, 2015 · 11 comments

Comments

@marcinn
Copy link

marcinn commented Nov 9, 2015

 d = AttrDict({'a': 1, 'b': AttrDict({'c': 3})})

converting to dict:

In [10]: d.to_dict()
Out[10]: {'a': 1, 'b': {'c': 3}}
In [12]: type(d.to_dict()['b'])
Out[12]: elasticsearch_dsl.utils.AttrDict

Expecting dict as type of b.

@kputland
Copy link

kputland commented Nov 9, 2015

Possibly better is that AttrDict, AttrList are unwrapped before storing in
the internal dict.
This is the approach I had to take for another project with similar objects.

--Karl

_Karl Putland | _Voice Engineer III
Business Solutions Group
Anywhere (303) 242-8608 <3032428608> | [email protected]
9100 E. Panorama Dr., Ste 175 Centennial, CO 80112

[image: cid:[email protected]]
http://t.sidekickopen05.com/e1t/c/5/f18dQhb0S7lC8dDMPbW2n0x6l2B9nMJW7t5XYg1q0FV-W7dL2Pv7fcxLHW3Mqkws56dy6Nf1jsX_g02?t=http%3A%2F%2Fwww.vonagebusiness.com%2F%3Futm_source%3Dvonage%26utm_medium%3De-signature%26utm_campaign%3Dvonage-email-redirect%26utm_content%3Dupgrade-business&si=6412430733213696&pi=653f3e27-b33e-4b16-a3d0-71ac0fa165e3

We transform how people connect. Click here to find out how we’re doing it
for businesses.
http://t.sidekickopen05.com/e1t/c/5/f18dQhb0S7lC8dDMPbW2n0x6l2B9nMJW7t5XYg1q0FV-W7dL2Pv7fcxLHW3Mqkws56dy6Nf1jsX_g02?t=http%3A%2F%2Fbusiness.vonage.com%2F&si=6412430733213696&pi=653f3e27-b33e-4b16-a3d0-71ac0fa165e3

On Mon, Nov 9, 2015 at 12:06 PM, Marcin Nowak [email protected]
wrote:

d = AttrDict({'a': 1, 'b': AttrDict({'c': 3})})

converting to dict:

In [10]: d.to_dict()
Out[10]: {'a': 1, 'b': {'c': 3}}

In [12]: type(d.to_dict()['b'])
Out[12]: elasticsearch_dsl.utils.AttrDict

Expecting dict as type of b.


Reply to this email directly or view it on GitHub
#291.

@marcinn
Copy link
Author

marcinn commented Nov 9, 2015

@kputland , I've got complex and nested AttrDict directly from ES result. I'm using nested properties in my doctype. I'm trying to get plain dict, I must have plain dict, but to_dict() has an issue mentioned above.

@honzakral
Copy link
Contributor

@marcinn if you get a nested document from ES it is still just a dict, only when accessing it via the top level AttrDict is it wrapped in AttrDict. So this should never happen when talking about dicts from elasticsearch.

Also, if you create a connection via the recommended connections.create_connection method, it will instantiate Elasticsearch with a serializer that knows how to deal with AttrDict and AttrList.

I know this is not ideal, but it makes everything much faster when we don't have to recursively walk the datastructures to serialize everything.

What do you think would be the best solution?

@marcinn
Copy link
Author

marcinn commented Nov 9, 2015

I'm calling Search methods and finally execute(). I'm getting <class 'elasticsearch_dsl.result.Response'>. Response contains Result items. Both Result and Response classes inherits directly from AttrDict, so they're an AttrDicts.

How can I get the plain dict instead of AttrDict from Response?

What do you think would be the best solution?

Avoid these wrappers, you know it already. :(

@honzakral
Copy link
Contributor

If you want to avoid these wrappers just call to_dict on the Response and you will get the dict itselt, no AttrDict anywhere. Or you can call it on every Result and also get just the raw dict with no wrapper. It's really that easy.

Can you show me where this is not the case in real world?

@marcinn
Copy link
Author

marcinn commented Nov 9, 2015

I have something like that:

response = Search()....execute()
for item in response:
     data = item.to_dict()

When I'm getting dict from response I must go additionally through response['hits']['hits'], but yes - it works as you described - no AttrDicts anymore.

This is not as simple as iterating over response. That's why getting complete (plain) dict from any AttrDict node would be great and very handy. Maybe to_dict() should have optional "recursive" argument? When I'm thinking about conversion from one type to another, I'm expecting complete process, not only at the top level. Please reconsider this.

And thanks for the answer!

@honzakral
Copy link
Contributor

I don't understand - if you don't change the data, calling to_dict on the Response returns a raw dictionary, no AttrDicts anywhere:

from elasticsearch_dsl import Search, utils

def check(d):
    for key, value in d.items():
        assert not isinstance(value, (utils.AttrDict, utils.AttrList))

        if isinstance(value, dict):
            check(value)
        if isinstance(value, list):
            for v in value:
                if isinstance(v, dict):
                    check(v)
check(Search().execute().to_dict())

@marcinn
Copy link
Author

marcinn commented Nov 9, 2015

Confirmed - to_dict() on response returns a raw dict.

But raw result differs a little, so I would like to convert only a part of AttrDict`s tree. It looks simpler than raw output. That's why I think that converting part of AttrDict would be handy.

@honzakral
Copy link
Contributor

What do you mean by "converting part of AttrDict"? You can do that, just get that part and call to_dict on it. Only part where you can't do it is with AttrList, there you have to use for example: hits = response.hits._l_ - that will give you a raw list of raw dicts. We could add to_list method to AttrList to make it more explicit, but I don't see much value in that.

Adding recursive argument to AttrDict.to_dict won't help your use case here, the issue, as demonstrated in #291 (comment) is not with AttrDict.

@marcinn
Copy link
Author

marcinn commented Nov 9, 2015

I've got an error after joining with inner_hits manually. Next I was confused by "partial" covert of AttrDict to plain dict, as I wrote in description of the issue.

So it looks that was my big fault in my nested objects transform code. Sorry for that. My bad. Bić kijami po piętach i walić po łbie...

Maybe recursive to_dict() is not necessary as I thought before, or maybe there are some rare and unknown cases... Thanks again. If I will find something I'll post you complete example, but currently I have no issues with result.to_dict().

@honzakral
Copy link
Contributor

thanks, closing the issue

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

No branches or pull requests

3 participants