Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve optional parameters #3079

Merged
merged 3 commits into from
Oct 26, 2021
Merged

Conversation

adrien-berchet
Copy link
Contributor

Description

Update OptionalParameter and add several other parameter classes for other types.

Motivation and Context

Parameters raise warnings when their default value is None. As far as I know, it is only possible to use OptionalParameter for string type so I added optional classes for other types.

Have you tested this? If so, how?

I have included unit tests.

@adrien-berchet
Copy link
Contributor Author

Do you think that the fail in CI is due to this PR? I don't think so but I am not sure.

@dlstadther
Copy link
Collaborator

dlstadther commented May 10, 2021

Do you think that the fail in CI is due to this PR? I don't think so but I am not sure.

The errors listed don't look related to these changes. @ckiosidis do you have insights? This was the first time i've looked at PR-specific unittests for Luigi since the move to Github Actions. I assume master is passing?

@adrien-berchet
Copy link
Contributor Author

Hello
I come back to this since it seems the CI is working now. Can you try to trigger it please?

class OptionalListParameter(OptionalParameterMixin, ListParameter):
"""Class to parse optional list parameters."""

expected_type = tuple
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is OptionalListParameter intentionally a tuple? As opposed to a list?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is intentional because the _warn_on_wrong_param_type is called after normalize which calls recursively_freeze on the value, thus the list is transformed into a tuple before the type is checked. That's why we expect a tuple here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right. I vaguely recall that confusion. I believe I added List Parameter and Tuple Parameter separately and later wished I hadn't added both. But alas, we have them both and treat them nearly the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed it would be simpler to have only one of these 2.

luigi/parameter.py Outdated Show resolved Hide resolved
test/optional_parameter_test.py Outdated Show resolved Hide resolved
class OptionalListParameter(OptionalParameterMixin, ListParameter):
"""Class to parse optional list parameters."""

expected_type = tuple
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right. I vaguely recall that confusion. I believe I added List Parameter and Tuple Parameter separately and later wished I hadn't added both. But alas, we have them both and treat them nearly the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants