search

spotify / luigi

Luigi is a Python module that helps you build complex pipelines of batch jobs. It handles dependency resolution, workflow management, visualization etc. It also comes with Hadoop support built in.
Apache License 2.0
17.89k stars 2.4k forks source link

Why assert default is not True for BoolParameter? #1268

Closed bencharb closed 9 years ago

bencharb commented 9 years ago

Why assert default is not True for BoolParameter? This breaks tasks with some_parameter = BoolParameter(default=True)

Code: https://github.com/spotify/luigi/commit/bba1ef2e93747d14db449bd94c49a2c026282f7a#diff-6d22afaf0832ec4dde7f9ad3477e8b4aR142

erikbern commented 9 years ago

This is because command line integration breaks if you have True as default. But maybe that's a bad argument against it and we should allow it

Tarrasch commented 9 years ago

You mean that it's a valid use case to specify [my_section] my_bool_param: False? I didn't think about that. I'm happy to merge a removal of that assertion if there's an accompanying test case, showing that it can be set from config.

bencharb commented 9 years ago

Here's the task where I'd like to set a default=True

class BigqueryExtractTask(luigi.task.Task):
    ''' Extracts a Bigquery table to Google Cloud Storage '''
    extraction_format = Parameter(default='CSV')  # AVRO, JSON, CSV
    extract_compression = BoolParameter(default=True)
    extract_print_header = BoolParameter(default=True)
    field_delimiter = Parameter(default=',')
    table_id = Parameter()
    project_id = Parameter()
    dataset_id = Parameter()
    create_target_flag = BoolParameter(default=True)
    destination_uri_pattern = Parameter()
    # snip ...

But maybe theres's better way to architect for this need and avoid this assertion altogether?

Tarrasch commented 9 years ago

I would probably go for no_extract_compression = BoolParameter(default=False), so I can set it from the command line as well.

dhruvg commented 9 years ago

I am facing this issue too. @bencharb are you working on the removal of the assertion?

bencharb commented 9 years ago

@Tarrasch Funny, I was going to suggest that as a joke. :+1:, but it does work fine for me now. @dhruvg I'm not removing the assertion because I'm not familiar enough with Luigi to know what else it affects.

Tarrasch commented 9 years ago

@bencharb, don't worry. We'll (a) code review it and (b) there are tons of tests that you can easily run locally and will be checked by Travis. :)

Tarrasch commented 9 years ago

Fixed in #1271, thanks for the fix @dhruvg and thanks for reporting this @bencharb!