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

Only iterate over self.compression if it's a dict #232

Merged
merged 1 commit into from
Mar 27, 2023
Merged

Conversation

ljgray
Copy link
Contributor

@ljgray ljgray commented Mar 23, 2023

This changes the default compression behaviour to use the container compression args. Compression will be disabled if compression: false is set, and compression args will be overwritten if a dict is provided in the config.

Previously, the default behaviour would disable compression if no arguments were provided

draco/core/task.py Outdated Show resolved Hide resolved
@ljgray ljgray force-pushed the compression-args branch 2 times, most recently from 0916689 to 5594643 Compare March 27, 2023 21:37
Copy link
Contributor

@jrs65 jrs65 left a comment

Choose a reason for hiding this comment

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

Looks good. A quick documentation comment, but just merge when you've fixed it up.

Otherwise, the default parameters set in the dataset spec are used.
Note that this will modify these parameters on the container itself, such that
if it is written out again downstream in the pipeline these will be used.
If set to `False` (or anything that evaluates to `False`, such as an empty dict),
Copy link
Contributor

Choose a reason for hiding this comment

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

You might want to update the docstring to reflect your last change.

The expected behaviour of the compression argument was to overwrite
compression args if a dict with those args was provided, to disable
compression if something which evaluates to False was provided, and use
the default parameters for the container otherwise. The behaviour
previously would instead disable compression if no args were provided.
@ljgray ljgray merged commit 83d4a55 into master Mar 27, 2023
@ljgray ljgray deleted the compression-args branch March 27, 2023 22:10
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