No compression for CLI test ouputs #924
Conversation
bayliffe
requested review from
benfitzpatrick,
arh89 and
TomekTrzeciak
and removed request for
arh89
| @@ -142,6 +144,10 @@ def save_netcdf(cubelist, filename): | |||
| Raises: | |||
| warning if cubelist contains cubes of varying dimensions. | |||
| """ | |||
| # To avoid compression SAVE_COMPRESSED=False can be exported to the | |||
| # environment. Defaults True if not set. | |||
| save_compressed = ast.literal_eval(os.getenv("SAVE_COMPRESSED", "True")) | |||
There was a problem hiding this comment.
I think I'd prefer a CLI flag really. If we are going to go for an environment variable, it probably should be IMPROVER_SAVE_UNCOMPRESSED and just test it's non-empty. We should also document it in all the CLI help... so it probably doesn't save any effort vs a CLI flag.
There was a problem hiding this comment.
To me it also feels like reading environment within the save routine doesn't really belong there. Unfortunately, there doesn't seem any other easy way to implement that. CLI flag will force us for us add this to each and every CLI (because we have no way to handle common options across all CLIs).
A nice way to implement this could be with a context manager:
with improver.settings(netcdf_compression=False):
...Unfortunately, we have no common point of entry into CLIs where we could add something like this.
CC @arh89, maybe you have some thoughts on this.
There was a problem hiding this comment.
(Agreed with @bayliffe about not doing this until after @neilCrosswaite's PR has gone in.)
Adding to all CLIs should be trivial, if we add to the COMPULSORY_ARGUMENTS in https://github.com/metoppv/improver/blob/master/lib/improver/argparser.py#L87, then all CLIs should pick them up. (NB: We'll still have to update all the BATS test outputs though, e.g help/nullargs)
There was a problem hiding this comment.
Also, if we wait until after Neil's work has gone in, we can trivially centralise the loading and saving, then we're golden 👍
|
Following discussion with @benfitzpatrick we will take an approach in which the call to save_netcdf is modified in each CLI to include the flag determining whether or not to use compression. This will require changes to all the CLIs and so will not be undertaken until @neilCrosswaite has completed his ongoing modifications to the CLIs. |
This PR proposes a simple method for turning off compression in the save wrapper using an imported environment variable. This will enable us to run the CLI tests without compression, which speeds them up by a factor of more than 2 (essentially reclaiming lost speed).
Note that a particular pylint check has been disabled for the test_save.py file as there is currently a bug in pylint which does not seem to be being fixed very rapidly: pylint-dev/pylint#1498
Testing: