Make getpaths type-aware, add getpaths test. #2113
Conversation
|
Seems like the |
|
Could you give me a hint how to check what went wrong? |
luigi/task.py
Outdated
| @@ -817,19 +817,19 @@ def getpaths(struct): | |||
| if isinstance(struct, Task): | |||
| return struct.output() | |||
| elif isinstance(struct, dict): | |||
| r = {} | |||
| r = struct.__class__() | |||
There was a problem hiding this comment.
What's this? The constructor for the dict subtype? Will this work if it's a immutable type?
There was a problem hiding this comment.
Yep. If it's a subtype of dict it should always be mutable.
I just needed to rerun travis on that. Sorry never your fault. |
Should be covered with the I can add more tests if you want. |
test/task_test.py
Outdated
| self.assertIsInstance(struct["OrderedDict"], collections.OrderedDict) | ||
| self.assertIsInstance(struct["list"], list) | ||
| self.assertIsInstance(struct["tuple"], tuple) | ||
| self.assertIsInstance(struct["generator"], list) |
There was a problem hiding this comment.
You should probably only check for being iterable here.
| for k, v in six.iteritems(struct): | ||
| r[k] = getpaths(v) | ||
| return r | ||
| return struct.__class__((k, getpaths(v)) for k, v in six.iteritems(struct)) |
There was a problem hiding this comment.
Here, I assumed that even immutable mappings (such as a "frozendict"-like mapping) shares the functionality of a standard dict to take a list/generator of tuples in the constructor.
AFAIK there isn't a builtin immutable mapping, but people could use implementations such as frozendict or this implementation, which should work here.
There was a problem hiding this comment.
I believe this would crash if you use a collections.Counter. But if you use that for this purpose, you totally deserve it. ;p
To be clear. This code is better, there's no need for a fix. :)
There was a problem hiding this comment.
This code is better, there's no need for a fix. :)
You mean
for k, v in six.iteritems(struct):
r[k] = getpaths(v)
return r? Sure, I can revert that commit.
There was a problem hiding this comment.
No, I think the current code is better. If I have to choose between supporting custom frozen dicts or collections.Counter. I would pick the former any day (for this instance)
|
@riga, I merged this, but took the liberty to greatly reword the commit message. I hope you don't mind. Thanks for this patch! :) |
Description
This PR slightly updates
luigi.task.getpathswhich is used to replace tasks in the structured object returned byTask.requireswith their respective outputs. The change makes that procedure type-aware, so e.g. when the struct's class is a subclass of dict, the returned struct has the same class.Motivation and Context
We often rely on the order in which we define our task requirements, e.g. via an
OrderedDict. However, when looping overself.input(), that order might not be preserved at the moment, because the returned object type is a plaindict.Tests
I added
task_test.TaskTest.test_getpathsand checked for several types.