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

Fix windows tests #1104

Merged
merged 5 commits into from
Aug 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ repos:
- test_requirements.txt

- repo: https://github.com/psf/black
rev: 24.4.2
rev: 24.8.0
hooks:
- id: black
language_version: python3
Expand All @@ -31,12 +31,12 @@ repos:


- repo: https://github.com/astral-sh/ruff-pre-commit
rev: v0.5.0
rev: v0.5.7
hooks:
- id: ruff

- repo: https://github.com/tox-dev/pyproject-fmt
rev: 2.1.3
rev: 2.2.1
hooks:
- id: pyproject-fmt

Expand Down
4 changes: 2 additions & 2 deletions compliance_checker/cf/cf_1_6.py
Original file line number Diff line number Diff line change
Expand Up @@ -3340,7 +3340,7 @@ def check_packed_data(self, ds):

# IMPLEMENTATION CONFORMANCE 8.1 REQUIRED 1/3
# scale_factor and add_offset same type
if type(add_offset) != type(scale_factor):
if not isinstance(add_offset, type(scale_factor)):
valid = False
reasoning.append(
"Attributes add_offset and scale_factor have different data type.",
Expand All @@ -3350,7 +3350,7 @@ def check_packed_data(self, ds):
# if not the same type
# FIXME: Check add_offset too.

elif type(scale_factor) != var.dtype.type:
elif not isinstance(scale_factor, var.dtype.type):
# Check both attributes are type float or double
if not isinstance(scale_factor, (float, np.floating)):
valid = False
Expand Down
25 changes: 18 additions & 7 deletions compliance_checker/cf/cf_1_9.py
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,10 @@ def check_domain_variables(self, ds: Dataset):
continue
appendix_a_not_recommended_attrs = []
for attr_name in domain_var.ncattrs():
if attr_name in self.appendix_a and "D" not in self.appendix_a[attr_name]["attr_loc"]:
if (
attr_name in self.appendix_a
and "D" not in self.appendix_a[attr_name]["attr_loc"]
):
appendix_a_not_recommended_attrs.append(attr_name)

if appendix_a_not_recommended_attrs:
Expand All @@ -163,21 +166,29 @@ def check_domain_variables(self, ds: Dataset):
# no errors occurred
domain_valid.score += 1


# IMPLEMENTATION CONFORMANCE 5.8 REQUIRED 4/4
if hasattr(domain_var, "cell_measures"):
cell_measures_var_names = regex.findall(r"\b(?:area|volume):\s+(\w+)", domain_var.cell_measures)
cell_measures_var_names = regex.findall(
r"\b(?:area|volume):\s+(\w+)",
domain_var.cell_measures,
)
# check exist
for var_name in cell_measures_var_names:
try:
cell_measures_variable = ds.variables[var_name]
except ValueError:
# TODO: what to do here?
continue
domain_coord_var_names = {var_like.name for var_like in domain_coord_vars}
domain_valid.assert_true(set(cell_measures_variable.dimensions).issubset(domain_coord_var_names),
"Variables named in the cell_measures attributes must have a dimensions attribute with "
"values that are a subset of the referring domain variable's dimension attribute")
domain_coord_var_names = {
var_like.name for var_like in domain_coord_vars
}
domain_valid.assert_true(
set(cell_measures_variable.dimensions).issubset(
domain_coord_var_names,
),
"Variables named in the cell_measures attributes must have a dimensions attribute with "
"values that are a subset of the referring domain variable's dimension attribute",
)

results.append(domain_valid.to_result())

Expand Down
15 changes: 10 additions & 5 deletions compliance_checker/protocols/zarr.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import platform
import zipfile
from pathlib import Path
from urllib.parse import urlparse
Expand All @@ -6,7 +7,11 @@

from compliance_checker.protocols import netcdf

#

def _fix_windows_slashes(zarr_url):
if platform.system() == "Windows":
zarr_url = zarr_url.replace("///", "//")

Check warning on line 13 in compliance_checker/protocols/zarr.py

View check run for this annotation

Codecov / codecov/patch

compliance_checker/protocols/zarr.py#L13

Added line #L13 was not covered by tests
Copy link
Member Author

Choose a reason for hiding this comment

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

This one had me chasing red herrings all over the code base. I'm not sure if nczarr has a bug, or Python as_uri, or something else. All I know is that we can't have 3 / on Windows, only 2!

return zarr_url


def is_zarr(url):
Expand Down Expand Up @@ -55,10 +60,9 @@
pr = urlparse(str(url))

if "mode=nczarr" in pr.fragment:
if pr.netloc:
return str(url) # already valid nczarr url
elif pr.scheme == "file":
return str(url) # already valid nczarr url
if pr.netloc or pr.scheme == "file":
url = _fix_windows_slashes(url)
return url

zarr_url = Path(
url2pathname(pr.path),
Expand All @@ -78,4 +82,5 @@
url_base = url if mode == "s3" else zarr_url.as_uri()

zarr_url = f"{url_base}#mode=nczarr,{mode}"
zarr_url = _fix_windows_slashes(zarr_url)
return zarr_url
5 changes: 0 additions & 5 deletions compliance_checker/suite.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
import inspect
import itertools
import os
import platform
import re
import subprocess
import sys
Expand Down Expand Up @@ -893,10 +892,6 @@ def load_local_dataset(self, ds_str):
ds_str = self.generate_dataset(ds_str)

if zarr.is_zarr(ds_str):
if platform.system() != "Linux":
print(
f"WARNING: {platform.system()} OS detected. NCZarr is not officially supported for your OS as of when this API was written. Your mileage may vary.",
)
return Dataset(zarr.as_zarr(ds_str))

if netcdf.is_netcdf(ds_str):
Expand Down
12 changes: 7 additions & 5 deletions compliance_checker/tests/test_cf.py
Original file line number Diff line number Diff line change
Expand Up @@ -3249,7 +3249,7 @@ def test_domain(self):
dataset.createDimension("lat", 20)
dataset.createDimension("depth", 20)
domain_var.setncattr("dimensions", "lon lat depth")
cube = dataset.createVariable("cube", "f8", ("lon", "lat", "depth"))
dataset.createVariable("cube", "f8", ("lon", "lat", "depth"))
# OK, coordinates in cell_measures are subset of coordinates of
# referring domain variable's coordinates attribute
results = self.cf.check_domain_variables(dataset)
Expand All @@ -3258,10 +3258,12 @@ def test_domain(self):
domain_var.cell_measures = "volume: cube_bad"
dataset.createVariable("cube_bad", "f8", ("lon", "lat", "depth", "time"))
results = self.cf.check_domain_variables(dataset)
self.assertTrue("Variables named in the cell_measures attributes must "
"have a dimensions attribute with values that are a "
"subset of the referring domain variable's dimension "
"attribute" in results[0].msgs)
self.assertTrue(
"Variables named in the cell_measures attributes must "
"have a dimensions attribute with values that are a "
"subset of the referring domain variable's dimension "
"attribute" in results[0].msgs,
)
del dataset
dataset = MockTimeSeries()
# domain should be dimensionless -- currently not an error in
Expand Down
47 changes: 23 additions & 24 deletions compliance_checker/tests/test_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,13 @@

from .conftest import datadir, static_files

on_windows = platform.system() == "Windows"

if on_windows:
ncconfig = ["sh", f"{os.environ['CONDA_PREFIX']}\\Library\\bin\\nc-config"]
else:
ncconfig = ["nc-config"]
Comment on lines +23 to +28
Copy link
Member Author

Choose a reason for hiding this comment

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

@benjwadams while this works, I'm not sure we need this. We can find the version with netCDF4 and let the zarr availability be checked by the failing test. What do you think?



@pytest.mark.usefixtures("checksuite_setup")
class TestCLI:
Expand Down Expand Up @@ -221,32 +228,19 @@ def test_multi_checker_return_value(self, tmp_txt_file):
assert not return_value

def _check_libnetcdf_version():
if platform.system() == "Linux":
# nc-config doesn't work on windows... and neither does NCZarr so this skipif is mutually exclusive to the OS check skipif
return (
float(
subprocess.check_output(
["nc-config", "--version"],
encoding="UTF-8",
)[9:12],
)
< 8.0
return (
float(
subprocess.check_output(
ncconfig + ["--version"],
encoding="UTF-8",
)[9:12],
)
else:
return True
< 8.0
)

# TODO uncomment the third parameter once S3 support is working
@pytest.mark.skipif(
_check_libnetcdf_version(),
reason="NCZarr support was not available until netCDF version 4.8.0. Please upgrade to the latest libnetcdf version to test this functionality",
)
@pytest.mark.skipif(
platform.system() != "Linux",
reason="NCZarr is not officially supported for your OS as of when this API was written",
)
@pytest.mark.skipif(
subprocess.check_output(["nc-config", "--has-nczarr"]) != b"yes\n",
reason="NCZarr support was not built with this netCDF version",
subprocess.check_output(ncconfig + ["--has-nczarr"]) != b"yes\n",
reason="NCZarr is not available.",
)
@pytest.mark.parametrize(
"zarr_url",
Expand All @@ -255,7 +249,12 @@ def _check_libnetcdf_version():
str(datadir / "zip.zarr"),
# "s3://hrrrzarr/sfc/20210408/20210408_10z_anl.zarr#mode=nczarr,s3"
],
ids=["local_file", "zip_file"], # ,'s3_url'
ids=[
"local_file",
"zip_file",
# TODO uncomment once S3 support is working.
# "s3_url",
],
)
def test_nczarr_pass_through(self, zarr_url):
"""
Expand Down
Loading