search

m3dev / gokart

Gokart solves reproducibility, task dependencies, constraints of good code, and ease of use for Machine Learning Pipeline.
https://gokart.readthedocs.io/en/latest/
MIT License
318 stars 57 forks source link

Feature/ Runtime validation of TaskInstanceParameter() and ListTaskInstanceParameter() by subclass bound #305

Closed ujiuji1259 closed 1 year ago

ujiuji1259 commented 1 year ago

Issue: #268

I've implemented runtime validation of TaskInstanceParameter() and ListTaskInstanceParameter() by subclass bound.

@ganow I'd like to use this feature, so I've created a PR based on your idea. Thank you for your feature request and the concrete implementation idea!

ganow commented 1 year ago

@mski-iksm Thank you for creating the PR!

I forgot to make a PR after making this issue.

ujiuji1259 commented 1 year ago

@mski-iksm @hirosassa @Hi-king please review!

ujiuji1259 commented 1 year ago

Thank you for the reviews!

I found _warn_on_wrong_param_type, and think overriding _warn_on_wrong_param_type to raise unexpected param type is more suitable in this situation.

How about this implementation?

Sample

class TaskInstanceParameter(luigi.Parameter):
    ...

    def _warn_on_wrong_param_type(self, param_name, param_value):
        if self.__class__ != TaskInstanceParameter:
            return
        if not isinstance(param_value, self.expected_type):
            raise TypeError(f'{param_value} is not an instance of {self.expected_type}')
ujiuji1259 commented 1 year ago

I've updated implementation to use _warn_on_wrong_param_type.

Could you review again? @mski-iksm @hirosassa @Hi-king

Hi-king commented 1 year ago

@ganow Thx for suggesting nice feature for improving reliability :)

@ujiuji1259 Thank you for the reasonable implementation! :+1: