WIP: Detect parameter collisions in the @inherits decorator. See #2566 for more details. #2565
Conversation
KenyaDonDraper
requested review from
dlstadther,
Tarrasch and
ulzha
as code owners
KenyaDonDraper
changed the title
KenyaDonDraper
changed the title
KenyaDonDraper
changed the title
KenyaDonDraper
changed the title
There was a problem hiding this comment.
Thanks for submitting this PR!
While i'm not a user of this feature, I'm happy to see improvements upon it and i'll continue to provide general code and luigi reviews.
I realize this is a WIP, but thought i'd go ahead and submit a quick, superficial review :)
| super(inherits, self).__init__() | ||
| if not tasks_to_inherit: | ||
| raise TypeError("tasks_to_inherit cannot be empty") | ||
|
|
||
| self.tasks_to_inherit = tasks_to_inherit | ||
| self.ignore_collisions = ignore_collisions |
There was a problem hiding this comment.
highly recommend
def __init__(self, *tasks_to_inherit, ignore_collisions=None):
...
self.ignore_collisions = ignore_collisions or []There was a problem hiding this comment.
I agree that was a bad practice. Assuming the mistake was using the mutable default argument, I actually prefer replacing the empty list with a tuple,
def __init__(self, *tasks_to_inherit, ignore_collisions=()):
...
self.ignore_collisions = ignore_collisionsThis way we remove the mutable default argument and at the same time allow the argument to communicate to a developer that the expected input is a sequence. The fewer lines the developer has to read before inferring the expected input the better.
There was a problem hiding this comment.
No complaint from me :)
luigi/util.py
Outdated
| self.check_for_parameter_collisions(task_that_inherits) | ||
|
|
||
| # Get all parameter objects from each of the underlying tasks and | ||
| # them to the inheriting task. |
There was a problem hiding this comment.
maybe a typo... not sure what you are trying to say and them to the inheriting task
There was a problem hiding this comment.
yes typo, thanks. Was supposed to read: and add them to the inheriting task. Will make the change.
luigi/util.py
Outdated
| error_msg = ( | ||
| "Parameter name collision detected in tasks_to_inherit. " | ||
| "Parameter '{param}' in '{task}' duplicates " | ||
| "parameter '{param}' in {another_task}. " |
There was a problem hiding this comment.
could probably be simplified to Parameter {param} in {task} is duplicated in {another_task}. No need to continually output the name of the parameter if there is a collision due to the names being the same.
There was a problem hiding this comment.
Agreed. Will make the change.
luigi/util.py
Outdated
|
|
||
| # Check that the parameters from an inherited task don't mask the | ||
| # parameters from another inherited task. | ||
| for task_to_inherit in self.tasks_to_inherit: |
There was a problem hiding this comment.
As written, this loop is ran 3 times. 2 within this method and once afterward. Is there anyway to combine the looping of these tasks_to_inherit to avoid repetitive looping?
| super(requires, self).__init__() | ||
| if not tasks_to_require: | ||
| raise TypeError("tasks_to_require cannot be empty") | ||
|
|
||
| self.tasks_to_require = tasks_to_require | ||
| self.ignore_collisions = ignore_collisions |
There was a problem hiding this comment.
same here as mentioned above
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If closed, you may revisit when your time allows and reopen! Thank you for your contributions. |
|
FWIW, we found the name Let me know if I should open a PR for it. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If closed, you may revisit when your time allows and reopen! Thank you for your contributions. |
Detect parameter collisions while using the @inherits and the associated
@requires decorator. Collision detection for a particular parameter can
be opted out by adding it to the 'ignore_collisions=[]' list argument
of the decorator.