Add ChoiceParameter to restrict parameter options to those specified. #1800
Conversation
|
@brcopeland, thanks for your PR! By analyzing the annotation information on this pull request, we identified @dlstadther, @Tarrasch and @mikekap to be potential reviewers |
|
lgtm although would be cool if this works with |
|
@erikbern, can you give me an example of what you mean? I'd be happy to make the change(s). When I create a Task using a ChoiceParameter, the parameter and its description show up when I run |
|
would be cool if the |
| if var in self.choices: | ||
| return var | ||
| else: | ||
| raise ValueError("{s} is not a valid choice from {choices}".format( |
There was a problem hiding this comment.
this line is broken – can you fix
There was a problem hiding this comment.
Yes, fixed. Don't know how I didn't copy that last line.
|
Ah, yeah. I'm trying to figure out a way to do that - updating self.description in init does not appear to be sufficient. |
|
Ok, got it working. My system has a luigi install I was importing instead of the one I was editing. |
|
awesome! flake8 is complaining: https://travis-ci.org/spotify/luigi/jobs/149050059 |
|
Can you also add test cases please? |
| """ | ||
| def __init__(self, choices, var_type=str, *args, **kwargs): | ||
| super(ChoiceParameter, self).__init__(*args, **kwargs) | ||
| self.choices = set(var_type(choice) for choice in choices) |
There was a problem hiding this comment.
Can you make this into an assertion? I imagine people creating new Parameters know enough luigi+python that they would rather be explicit in types over than luigi magically converting stuff behind their back.
|
This class is somewhat similar to |
|
You could be entirely right here. I am a pretty new user of luigi and may have overlooked this class before coming up with my own version. I will clean it up if requested, but maybe at this point it just makes more sense to not include this. |
|
I still think this makes sense. Not everyone likes to create a new Enum type for every input-restriction and add all the conversion overhead if they want to eventually go back to a string type. I would be happy to include this, as long as the docs comes with recommended good practices. Like we've previously done here for example. :) |
brcopeland
force-pushed
the
ChoiceParameter
branch
from
909009a
to
c438adc
Compare
|
I changed the docstring to I believe the proper format and added a test suite to the test/ directory. Please let me know if any further changes would be welcome. |
| def __init__(self, var_type=str, *args, **kwargs): | ||
| if "choices" not in kwargs: | ||
| raise ParameterException("A choices iterable must be specified") | ||
| self._choices = set(var_type(choice) for choice in kwargs.pop("choices")) |
There was a problem hiding this comment.
As I pointed out earlier, I probably would prefer if we could somehow would assert instead of convert. But I'm not sure really what's best :)
There was a problem hiding this comment.
Oh, ok sure I think that's perfectly reasonable.
|
Apart from the small comment about assert/convert. This looks very good. I must say I'm very impressed how you managed to fix all the shortcomings in a single review turnaround! :) |
|
Made a couple changes to assert the choices are the proper type as you suggest. And sure, glad it's helpful. |
…s and var_type made private.
…an silently coercing them.
… given in choices.
brcopeland
force-pushed
the
ChoiceParameter
branch
from
40d68cd
to
baa6078
Compare
|
I rebased, updated docstrings, and added unit tests for the class. |
|
Awesomeness! :) |
|
A post-merge note. Some users might want to use the "Functional API" of enums. Instead of Some users might prefer |
Description
As in description.
Motivation and Context
This allows the user to define the permissible values for a parameter and the value type, and reject any parameter that does not meet these criteria.
Have you tested this? If so, how?
I've been using the class for several weeks without issue.