Feature/repacking config

[sol1105]

This PR aims to generalize the (re)packing checks, as they are not only a requirement for CMIP7 but also a recommendation for CORDEX-CMIP6, and potentially will be a requirement for CORDEX-CMIP7.

For CORDEX-CMIP6, cmip7repack has been forked and modified: https://github.com/WCRP-CORDEX/ncrepack-cordex
It allows to define fix chunk-length along the time dimension to allow kerchunking of the data files (i.e. creating virtual zarr datasets out of them), which is not possible when the chunk-length varies across the files of a dataset or within a file (which it does eg. due to leap years etc.) - there is ongoing work to allow such varying chunk lengths but that is too late for CORDEX-CMIP6.

The default behaviour is still the one originally implemented, but optional settings in the toml configuration make it possible to define allowed deviations for different frequencies and also different severities for the different kind of critieria.

Additionally, the functions were made more robust for exceptions of pyfive when reading and interpreting the datasets. Due to issues with parsing some AWI output, the minimum required pyfive version was increased to 1.1.2, which then allowed reading all test files.

@Ayoubnac1 @jesusff It would be great to get your feedback on this 😃

[jesusff]

Thank you, Martin. I was planning to give it a try, but in a much simpler way, just skipping the FILE004d check in CORDEX. E.g. by having a separate check_cordex_packing mimicking the check_cmip7_packing but without the FILE004d section.

Anyway, this is much more flexible and currently fits the defaults in ncrepack-cordex. I tested it and it works flawlessly, warning on non-repacked data and providing details if the chunk size does not reach 4MB and the time is not packed with at least the defaults in ncrepack.

I also tested the -t wcrp_cmip7 plugin and it still works as expected (complaining only about data variable chunk size after ncrepack-cordex and silent after cmip7repack)

[glevava]

Hi @sol1105

The changes you describe seem relevant to me. I just have one question regarding the severity_[a-d] settings you introduced.

1. Is it scientifically meaningful to allow a check to pass for the data chunking but not for the time chunking, for example?
2. This also seems to break somewhat with the QA/QC philosophy. I would have preferred to see fully independent checks, for example:

[file.internal_packing.metadata]
[file.internal_packing.time]
[file.internal_packing.bounds]
[file.internal_packing.data]

each with its own severity argument.

[sol1105]

@jesusff @glevava Thanks for your feedback 😃

Is it scientifically meaningful to allow a check to pass for the data chunking but not for the time chunking, for example?

I think each sub-check could get different priorities in other projects. In the CORDEX discussion about it, it was my impression that there was basically agreement that the repacked metadata as well as time+bounds make a lot of sense. And alone this already increases the performance quite a bit.

There was more discussion on the repacking of the data variable, since different use cases would require different packing. For example we have a usecase for our CORDEX-CMIP6 3hr-point data that requires that the data-chunks along the time dimension are only of size 1, so we can easily sub sample the 3hr-point data as 6hr-point data (in a virtual zarr dataset) without providing the data at both frequencies. This would however mostly conflict with the 4MiB rule.

This also seems to break somewhat with the QA/QC philosophy. I would have preferred to see fully independent checks, for example:
[file.internal_packing.metadata]
[file.internal_packing.time]
[file.internal_packing.bounds]
[file.internal_packing.data]

Currently FILE004a-d are all run by the same check function and this PR does not change it. But each could be registered as separate check as well (only the time and time bounds check should probably stay combined). Should I do that? Would they still be labeled FILE004a-d, or eg. FILE004/5/6?

[glevava]

Hi @sol1105

Thanks for the scientific explanation; since I am not an expert, my question was rather naive.
Regarding the separation of the checks, if you can do it, yes, I think that would be better and more consistent with the strategy we have adopted in cc-plugin-wcrp.

You can keep the FILE004a, FILE004b, etc. labels. And if time and time_bounds need to remain in the same check, then you can simply keep:
[file.internal_packing.time]
but in that case the same severity would be applied to both within a single check. As you prefer.

[sol1105]

@glevava One issue why this checks were running as a single check in the first place was probably to not require opening the file with pyfive more than once. The latest 2 commits segregates the 3 packing checks (metadata, time+time_bnds, data) in the cmip7.py (and cordex-cmip6.py) as well as in the toml configuration. It will close the file after calling check_internal_packing (or rather its finalize helper function) 3 times. This number can be modified in case one would remove/add packing checks at a later date or for other/custom plugins.

[glevava]

Great, thanks!

I will check with @Ayoubnac1 regarding the merge.

As for opening the file with pyfive, we may be able to do something smart: if any of these checks is enabled, the file could be opened once in memory, and the resulting object could then be reused by each of the checks.

I will also discuss this with @Ayoubnac1 .

[sol1105]

As for opening the file with pyfive, we may be able to do something smart: if any of these checks is enabled, the file could be opened once in memory, and the resulting object could then be reused by each of the checks.

I set it up like this, but the tricky part is to find out when one can close the file, this is why each of the checks runs a _finalize helper function that basically counts to three, and when reaching three will close the file (if the file was openend). _finalize will be executed for each packing check whether it is enabled in the toml config or not (see

cc-plugin-wcrp/checks/format_checks/check_internal_packing.py

Lines 82 to 122 in 6e7572c

	def finalize_internal_packing_session(ds, total_packing_checks: int = 3) -> None:
	"""Called after each internal packing section check method (metadata / time / data).
	Closes the shared FILE004 pyfive file handle after all section check methods
	have reported in.
	Parameters
	----------
	ds : dataset
	The dataset being checked.
	total_packing_checks : int, optional
	Total number of internal packing section check methods in the plugin.
	Default is 3 (metadata + time + data). Custom plugins that only implement
	a subset should pass the actual number so the file is closed promptly.
	"""
	file_path = _get_file_path(ds)
	key = _session_key(ds, file_path)
	entry = _INTERNAL_PACKING_FINALIZE_COUNTERS.get(key)
	if entry is None:
	# First finalize call: record the threshold and count.
	entry = {"count": 1, "total": total_packing_checks}
	else:
	if entry["total"] != total_packing_checks:
	raise ValueError(
	f"finalize_internal_packing_session called with total_packing_checks={total_packing_checks} "
	f"but was previously initialized with total_packing_checks={entry['total']}. "
	"Ensure all section check methods pass the same total_packing_checks value."
	)
	entry = {"count": entry["count"] + 1, "total": entry["total"]}
	if entry["count"] >= entry["total"]:
	session = _INTERNAL_PACKING_FILE_SESSIONS.pop(key, None)
	if session:
	try:
	session["file"].close()
	except Exception:
	pass
	_INTERNAL_PACKING_FINALIZE_COUNTERS.pop(key, None)
	else:
	_INTERNAL_PACKING_FINALIZE_COUNTERS[key] = entry

and eg.

cc-plugin-wcrp/plugins/cmip7/cmip7.py

Lines 437 to 454 in 6e7572c

	def check_File_Internal_Packing_Metadata(self, ds):
	try:
	if not self.config or not self.config.file or not self.config.file.internal_packing:
	return []
	r = self.config.file.internal_packing
	if not r.metadata:
	return []
	return check_internal_packing(
	ds,
	severity=self.get_severity(r.metadata.severity),
	run_metadata=True,
	run_time=False,
	run_data=False,
	)
	finally:
	finalize_internal_packing_session(ds, total_packing_checks=3)

).