Skip to content

Proposal: set default value of OptionalParameter to None#3411

Merged
dlstadther merged 2 commits intospotify:masterfrom
kitagry:set-default-none
Apr 4, 2026
Merged

Proposal: set default value of OptionalParameter to None#3411
dlstadther merged 2 commits intospotify:masterfrom
kitagry:set-default-none

Conversation

@kitagry
Copy link
Copy Markdown
Contributor

@kitagry kitagry commented Mar 25, 2026

Description

Currently, when using OptionalParameter (and its variants like OptionalStrParameter), users must explicitly set default=None. If omitted, Luigi throws a MissingParameterException.

Since the prefix "Optional" strongly implies that the parameter can be omitted and will default to a null state, the current behavior is slightly counter-intuitive. This PR proposes changing the default value of optional parameters from _no_value to None.

class MyTask(luigi.Task):
    param1: luigi.Parameter[str | None] = luigi.OptionalStrParameter()

    def output(self):
        return luigi.LocalTarget(f"output_{self.param1}.txt")

    def run(self):
        with self.output().open("w") as f:
            f.write(f"Parameter 1: {self.param1}\n")


luigi.build([MyTask()], local_scheduler=True)

expected behavior

===== Luigi Execution Summary =====

Scheduled 1 tasks of which:
* 1 ran successfully:
    - 1 MyTask(param1=)

actual behavior

luigi.parameter.MissingParameterException: MyTask[args=(), kwargs={}]: requires the 'param1' parameter to be set

Motivation and Context

To improve the developer experience by aligning the behavior of OptionalParameter with standard Python conventions and user expectations. This change reduces boilerplate code (removing the need to write default=None everywhere) and prevents unexpected errors when an optional parameter is simply omitted.

Have you tested this? If so, how?

Verified that the provided reproduction code now runs successfully and outputs as expected.

@kitagry
Copy link
Copy Markdown
Contributor Author

kitagry commented Apr 3, 2026

@dlstadther Sorry. Could you review this?

dlstadther
dlstadther previously approved these changes Apr 3, 2026
Copy link
Copy Markdown
Collaborator

@dlstadther dlstadther left a comment

Choose a reason for hiding this comment

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

I'm good here. Are there any test cases that need to be added/updated?

@kitagry kitagry requested a review from a team as a code owner April 3, 2026 23:33
@kitagry
Copy link
Copy Markdown
Contributor Author

kitagry commented Apr 3, 2026

@dlstadther Thank you for the review! I added a test to verify that a Task with OptionalParameter() (no explicit default) can be instantiated without a value and defaults to None.

Copy link
Copy Markdown
Collaborator

@dlstadther dlstadther left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for adding a test case!

@dlstadther dlstadther merged commit c3c89ca into spotify:master Apr 4, 2026
42 checks passed
@kitagry kitagry deleted the set-default-none branch April 4, 2026 01:17
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