Issue 538 - fixing wrong grib-filter-expression flag.#539
Issue 538 - fixing wrong grib-filter-expression flag.#539shubhamkumar-bot wants to merge 3 commits into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
| grib_get_cmd = shutil.which('grib_get') | ||
| uniq_cmd = shutil.which('uniq') | ||
| for cmd, name in [(grib_get_cmd, 'grib_copy'), (grib_get_cmd, 'grib_get'), (uniq_cmd, 'uniq')]: | ||
| for cmd, name in [(grib_copy_cmd, 'grib_copy'), (grib_get_cmd, 'grib_get'), (uniq_cmd, 'uniq')]: |
There was a problem hiding this comment.
Let's do this change in a separate PR please!
| self.logger.info('Finished uploading %r', self.input_path) | ||
| except Exception as e: | ||
| log_msg = ( | ||
| f"GRIB tool failed for {self.input_path!r}. This means the requested " |
There was a problem hiding this comment.
nits:
GribSplitterV2 failed for ...- We can omit
This means the requested filter expression {self.grib_filter_expression!r} does not exist in this file.and just log the error instead.
| f"Error: {e}" | ||
| ) | ||
| if self.skip_on_invalid_grib_filter_expression: | ||
| self.logger.warning(f"{log_msg} | Flag 'skip_on_invalid_grib_filter_expression' is True. Skipping file.") |
There was a problem hiding this comment.
Let's keep the log level as error.
nit: Can omit | Flag 'skip_on_invalid_grib_filter_expression' is True. Skipping file.
| logger.info('Using splitter: GribSplitterV2') | ||
| return GribSplitterV2(file_path, output_info, force_split, | ||
| logging_level, grib_filter_expression) | ||
| logging_level, grib_filter_expression, skip_on_invalid_grib_filter_expression) |
There was a problem hiding this comment.
Please run a linter over the files.
| def __init__(self, input_path: str, output_info: OutFileInfo, | ||
| force_split: bool = False, logging_level: int = logging.INFO, | ||
| grib_filter_expression: t.Optional[str] = None): | ||
| grib_filter_expression: t.Optional[str] = None, skip_on_invalid_grib_filter_expression:bool = False): |
There was a problem hiding this comment.
Please run a linter over the files.
| 'specifically supported by the GribSplitterV2' | ||
| 'implementation.' | ||
| 'Example: typeOfLevel=isobaricInhPa,level=1000') | ||
| parser.add_argument('--skip-on-invalid-grib-filter-expression', action='store_true', default=False, |
There was a problem hiding this comment.
I think this flag's name is a bit ambiguous. Rather than validating whether the GRIB filter expression itself is invalid, it seems we are just toggling how the pipeline handles a failure (crashing vs. logging and continuing).
Suggestion: A better name would be suppress_splitter_pipeline_failures.
3 participants
Added a flag that allows the user to choose whether to log invalid files (those missing the key-value pairs specified in the where flag) and continue execution, or to throw an error immediately upon encountering the first invalid file.