Recursively normalize DictParameter #2115
Conversation
There was a problem hiding this comment.
I think we can merge this as I see it as "strict improvement" (not the least in documentation), but I believe I have spotted a bug. :)
I'm not sure, but perhaps you have to create a helper like _recursively_freeze() which doesn't use anything from Parameter or normalize(). That's probably how I would do it, but this is just a suggestion. :)
luigi/parameter.py
Outdated
| if isinstance(v, Mapping): | ||
| return self.normalize(v) | ||
| elif isinstance(v, list): | ||
| return ListParameter().normalize(v) |
There was a problem hiding this comment.
Will this work if a list contains a dict or a list?
|
By the way, it seems like the failed test weren't just flukes. I think either code or testcode needs to be updated/fixed. |
|
Yea, I'm trying to reproduce that behavior but 3.4 and 3.5 are behaving as expected for me. I'll break the method out to be `_recursively_freeze`.
… On May 19, 2017, at 8:12 AM, Arash Rouhani ***@***.***> wrote:
By the way, it seems like the failed test weren't just flukes. I think either code or testcode needs to be updated/fixed.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
There was a problem hiding this comment.
Yea, I'm trying to reproduce that behavior but 3.4 and 3.5 are behaving as expected for me.
It's probably because the traversal orders of dicts are random.
By the way, why don't you add a test case for the code that you fixed with your latest commit? TDD is a sane practice at least when fixing bugs. :)
| @@ -917,7 +928,7 @@ def normalize(self, x): | |||
| :param str x: the value to parse. | |||
| :return: the normalized (hashable/immutable) value. | |||
| """ | |||
| return tuple(x) | |||
| return _recursively_freeze(x) | |||
There was a problem hiding this comment.
this should still be tuple() I think, or do we say anywhere a "bigger" structure is allowed?
There was a problem hiding this comment.
The documentation says <JSON string> so I think that implies it can be a list containing additional lists or Mappings
There was a problem hiding this comment.
It is sort of overkill for a very specific use case though.
There was a problem hiding this comment.
Why on earth is it a JSON Object ... oh well then. Looks good to me.
|
Big thanks! Such an improvement. But as for issue #2094, I'm not 100% confident this solved that issue. Let's ping the issue author in that issue. |
Description
This recursively calls normalize on
Mappings andlists when callingDictParameter.normalize. Also, makesOrderedFrozenDictandDictParameter.DictParamEncoderprivate, as requested by @Tarrasch.Motivation and Context
Fixes issue #2094.
Have you tested this? If so, how?
Added a test, but am open to suggestions of better tests.