Usability tweaks (#75)

* Fail early if JSONL file is present without --overwrite

* Add warnings if IDs across property files and entries do not match

* Improve error message when failing to parse custom property into the specified type

* Add validation for types and names in config that will otherwise fail later

* Drop aliases from the property dataframe to allow for easier validation

* Improve warning for mismatching property fields between property file and config

* don't warn if property id matches immutable_id

* fix pre-commit

---------

Co-authored-by: Kristjan Eimre <kristjaneimre@gmail.com>

Author: ml-evs

src/optimade_maker/config.py

@@ -3,8 +3,10 @@
 
 """
 
+from optimade.models.optimade_json import DataType
 from pydantic import ConfigDict, field_validator, model_validator
 
+IDENTIFIER_REGEX = r"^[a-z_][a-z_0-9]*$"
 __version__ = "0.1.0"
 
 from pathlib import Path
@@ -24,9 +26,10 @@ class PropertyDefinition(BaseModel):
     """
 
     name: str = Field(
-        description="""The field name of the property, as provided in the included
-the auxiliary property files.
-Will be served with a provider-specific prefix in the actual API, so must not start with an underscore."""
+        description="""The field name of the property to use in the API. Will be searched for in the included
+the auxiliary property files, unless `aliases` is also specified.
+Will be served with a provider-specific prefix in the actual API, so must not start with an underscore or contain upper case characters.""",
+        pattern=IDENTIFIER_REGEX,
     )
 
     title: Optional[str] = Field(
@@ -38,8 +41,7 @@ class PropertyDefinition(BaseModel):
     unit: Optional[str] = Field(
         None, description="The unit of the property, e.g. 'eV' or 'Å'."
     )
-    type: Optional[str] = Field(
-        None,
+    type: Optional[DataType] = Field(
         description="The OPTIMADE type of the property, e.g., `float` or `string`.",
     )
     maps_to: Optional[str] = Field(

src/optimade_maker/convert.py

@@ -89,6 +89,8 @@ def convert_archive(
 
     if not jsonl_path:
         jsonl_path = archive_path / "optimade.jsonl"
+    if jsonl_path.exists() and not overwrite:
+        raise RuntimeError(f"Not overwriting existing file at {jsonl_path}")
 
     # if the config specifies just a JSON-L, then extract any archives
     # and return the JSONL path
@@ -397,6 +399,10 @@ def _parse_and_assign_properties(
     if not property_matches_by_file:
         return
 
+    optimade_immutable_ids = {
+        entry["attributes"].get("immutable_id") for entry in optimade_entries.values()
+    }
+
     for archive_file in property_matches_by_file:
         for _path in tqdm.tqdm(
             property_matches_by_file[archive_file],
@@ -409,6 +415,14 @@ def _parse_and_assign_properties(
                     for id in properties:
                         parsed_properties[id].update(properties[id])
                         all_property_fields |= set(properties[id].keys())
+                        if (
+                            id not in optimade_entries
+                            and id not in optimade_immutable_ids
+                        ):
+                            warnings.warn(
+                                f"Could not find entry {id!r} in OPTIMADE entries.",
+                            )
+                            continue
                     break
                 except Exception as exc:
                     errors.append(exc)
@@ -430,9 +444,13 @@ def _parse_and_assign_properties(
     expected_property_fields = set(property_def_dict.keys())
 
     if expected_property_fields != all_property_fields:
-        warnings.warn(
-            f"Found {all_property_fields=} in data but {expected_property_fields} in config"
-        )
+        warning_message = "Mismatch between parsed property fields (A) and those defined in config (B)."
+        if all_property_fields - expected_property_fields:
+            warning_message += f"\n(A - B) = {all_property_fields - expected_property_fields} (will be omitted from API; if intended this can be ignored)."
+        if expected_property_fields - all_property_fields:
+            warning_message += f"\n(B - A) = {expected_property_fields - all_property_fields} (configured, but missing; check for typos or missing aliases)"
+
+        warnings.warn(warning_message)
 
     # Look for precisely matching IDs, or 'filename' matches
     for id in optimade_entries:
@@ -442,6 +460,14 @@ def _parse_and_assign_properties(
             # try to find a matching ID based on the filename
             property_entry_id = id.split("/")[-1].split(".")[0]
 
+        if (property_entry_id not in parsed_properties) and (
+            id not in parsed_properties
+        ):
+            warnings.warn(
+                f"Could not find entry {id!r} (or fully-qualified {property_entry_id!r}) in parsed properties",
+            )
+            continue
+
         # Loop over all defined properties and assign them to the entry, setting to None if missing
         # Also cast types if provided
         for property in all_property_fields:
@@ -451,10 +477,15 @@ def _parse_and_assign_properties(
                 property, None
             ) or parsed_properties.get(id, {}).get(property, None)
             if property not in property_def_dict:
-                warnings.warn(f"Missing property definition for {property=}")
+                # These are already warned about above: fields that are not configured but are present in the property file
                 continue
             if value is not None and property_def_dict[property].type in TYPE_MAP:
-                value = TYPE_MAP[property_def_dict[property].type](value)
+                try:
+                    value = TYPE_MAP[property_def_dict[property].type](value)
+                except Exception as exc:
+                    raise RuntimeError(
+                        f"Could not cast {value=} for {property=} to type {property_def_dict[property].type!r} for entry {id!r}"
+                    ) from exc
 
             optimade_entries[id]["attributes"][f"_{provider_prefix}_{property}"] = value
 

src/optimade_maker/parsers.py

@@ -13,7 +13,7 @@
     ) from exc
 
 from optimade.adapters import Structure
-from optimade.models import EntryResource
+from optimade.models import DataType, EntryResource
 
 from optimade_maker.config import PropertyDefinition
 
@@ -57,6 +57,7 @@ def load_csv_file(
         for alias in prop.aliases or []:
             if alias in df:
                 df[prop.name] = df[alias]
+                df.drop(columns=[alias], inplace=True)
                 break
 
     return df.to_dict(orient="index")
@@ -68,11 +69,11 @@ def load_csv_file(
     ".csv": [load_csv_file],
 }
 
-TYPE_MAP: dict[str | None, type] = {
-    "float": float,
-    "string": str,
-    "integer": int,
-    "boolean": bool,
+TYPE_MAP: dict[DataType, type] = {
+    DataType.FLOAT: float,
+    DataType.STRING: str,
+    DataType.INTEGER: int,
+    DataType.BOOLEAN: bool,
 }