Add .from_proto() and .from_json() class methods (#278)

Author: codesue

model_card_toolkit/base_model_card_field.py

@@ -20,19 +20,23 @@
 import abc
 import dataclasses
 import json as json_lib
-from typing import Any, Dict
+from textwrap import dedent
+from typing import Any, Dict, Type, TypeVar
+from warnings import warn
 
 from google.protobuf import descriptor, message
 
 from model_card_toolkit.utils import json_utils
 
+T = TypeVar('T', bound='BaseModelCardField')
+
 
 class BaseModelCardField(abc.ABC):
   """Model card field base class.
 
   This is an abstract class. All the model card fields should inherit this class
   and override the _proto_type property to the corresponding proto type. This
-  abstract class provides methods `copy_from_proto`, `merge_from_proto` and
+  abstract class provides methods `from_proto`, `merge_from_proto` and
   `to_proto` to convert the class from and to proto. The child class does not
   need to override this unless it needs some special process.
   """
@@ -55,7 +59,7 @@ def to_proto(self) -> message.Message:
     for field_name, field_value in self.__dict__.items():
       if not hasattr(proto, field_name):
         raise ValueError(
-            "%s has no such field named '%s'." % (type(proto), field_name)
+            '%s has no such field named "%s".' % (type(proto), field_name)
         )
       if not field_value:
         continue
@@ -80,19 +84,19 @@ def to_proto(self) -> message.Message:
 
     return proto
 
-  def _from_proto(self, proto: message.Message) -> "BaseModelCardField":
+  def _from_proto(self: T, proto: message.Message) -> T:
     """Convert proto to this class object."""
     if not isinstance(proto, self._proto_type):
       raise TypeError(
-          "%s is expected. However %s is provided." %
+          '%s is expected. However %s is provided.' %
           (self._proto_type, type(proto))
       )
 
     for field_descriptor in proto.DESCRIPTOR.fields:
       field_name = field_descriptor.name
       if not hasattr(self, field_name):
         raise ValueError(
-            "%s has no such field named '%s.'" % (self, field_name)
+            '%s has no such field named "%s".' % (self, field_name)
         )
 
       # Process Message type.
@@ -120,28 +124,44 @@ def _from_proto(self, proto: message.Message) -> "BaseModelCardField":
 
     return self
 
-  def merge_from_proto(self, proto: message.Message) -> "BaseModelCardField":
+  def merge_from_proto(self: T, proto: message.Message) -> T:
     """Merges the contents of the model card proto into current object."""
     current = self.to_proto()
     current.MergeFrom(proto)
     self.clear()
     return self._from_proto(current)
 
-  def copy_from_proto(self, proto: message.Message) -> "BaseModelCardField":
+  def copy_from_proto(self: T, proto: message.Message) -> T:
     """Copies the contents of the model card proto into current object."""
+    notice = dedent(
+        '''
+        This function is deprecated and will be removed in a future version.
+
+        If you would like to create a new model card from a proto, please use
+        `ModelCard.from_proto(proto)` instead.
+
+        If you would like to copy the contents of a proto into an existing model
+        card, please use `model_card.clear()` and `model_card.merge_from_proto(proto)`
+        instead.
+        '''
+    )
+    warn(notice, DeprecationWarning, stacklevel=2)
     self.clear()
     return self._from_proto(proto)
 
-  def _from_json(
-      self, json_dict: Dict[str, Any], field: "BaseModelCardField"
-  ) -> "BaseModelCardField":
+  @classmethod
+  def from_proto(cls: Type[T], proto: message.Message) -> T:
+    """Constructs an object of this class from a model card proto."""
+    return cls()._from_proto(proto)
+
+  def _from_json(self: T, json_dict: Dict[str, Any], field: T) -> T:
     """Parses a JSON dictionary into the current object."""
     for subfield_key, subfield_json_value in json_dict.items():
       if subfield_key.startswith(json_utils.SCHEMA_VERSION_STRING):
         continue
       elif not hasattr(field, subfield_key):
         raise ValueError(
-            "BaseModelCardField %s has no such field named '%s.'" %
+            'BaseModelCardField %s has no such field named "%s".' %
             (field, subfield_key)
         )
       elif isinstance(subfield_json_value, dict):

model_card_toolkit/core.py

@@ -193,7 +193,7 @@ def _read_proto_file(self, path: str) -> Optional[ModelCard]:
     model_card_proto = model_card_pb2.ModelCard()
     with open(path, 'rb') as f:
       model_card_proto.ParseFromString(f.read())
-    return ModelCard().copy_from_proto(model_card_proto)
+    return ModelCard.from_proto(model_card_proto)
 
   def _annotate_eval_results(self, model_card: ModelCard) -> ModelCard:
     """Annotates a model card with info from TFMA evaluation results.

model_card_toolkit/model_card.py

@@ -489,14 +489,14 @@ def to_json(self) -> str:
                     ] = json_utils.get_latest_schema_version()
     return json_lib.dumps(model_card_dict, indent=2)
 
-  def from_json(self, json_dict: Dict[str, Any]) -> None:
+  def merge_from_json(self, json: Union[Dict[str, Any], str]) -> 'ModelCard':
     """Reads ModelCard from JSON.
 
-    This function will overwrite all existing ModelCard fields.
+    This function will only overwrite ModelCard fields specified in the JSON.
 
     Args:
-      json_dict: A JSON dict from which to populate fields in the model card
-        schema.
+      json: A JSON object from which to populate fields in the model card. This
+        can be provided as either a dictionary or a string.
 
     Raises:
       JSONDecodeError: If `json_dict` is not a valid JSON string.
@@ -505,19 +505,19 @@ def from_json(self, json_dict: Dict[str, Any]) -> None:
       ValueError: If `json_dict` contains a value not in the class or schema
         definition.
     """
+    if isinstance(json, str):
+      json = json_lib.loads(json)
+    json_utils.validate_json_schema(json)
+    self._from_json(json, self)
+    return self
 
-    json_utils.validate_json_schema(json_dict)
-    self.clear()
-    self._from_json(json_dict, self)
-
-  def merge_from_json(self, json: Union[Dict[str, Any], str]) -> None:
-    """Reads ModelCard from JSON.
-
-    This function will only overwrite ModelCard fields specified in the JSON.
+  @classmethod
+  def from_json(cls, json_dict: Dict[str, Any]) -> 'ModelCard':
+    """Constructs a ModelCard from JSON.
 
     Args:
-      json: A JSON object from whichto populate fields in the model card. This
-        can be provided as either a dictionary or a string.
+      json_dict: A JSON dict from which to populate fields in the model card
+        schema.
 
     Raises:
       JSONDecodeError: If `json_dict` is not a valid JSON string.
@@ -526,7 +526,8 @@ def merge_from_json(self, json: Union[Dict[str, Any], str]) -> None:
       ValueError: If `json_dict` contains a value not in the class or schema
         definition.
     """
-    if isinstance(json, str):
-      json = json_lib.loads(json)
-    json_utils.validate_json_schema(json)
-    self._from_json(json, self)
+
+    json_utils.validate_json_schema(json_dict)
+    model_card = cls()
+    model_card._from_json(json_dict, model_card)
+    return model_card

model_card_toolkit/model_card_test.py

@@ -37,10 +37,9 @@
 
 
 class ModelCardTest(absltest.TestCase):
-  def test_copy_from_proto_and_to_proto_with_all_fields(self):
+  def test_from_proto_and_to_proto_with_all_fields(self):
     want_proto = text_format.Parse(_FULL_PROTO, model_card_pb2.ModelCard())
-    model_card_py = model_card.ModelCard()
-    model_card_py.copy_from_proto(want_proto)
+    model_card_py = model_card.ModelCard.from_proto(want_proto)
     got_proto = model_card_py.to_proto()
 
     self.assertEqual(want_proto, got_proto)
@@ -53,23 +52,27 @@ def test_merge_from_proto_and_to_proto_with_all_fields(self):
 
     self.assertEqual(want_proto, got_proto)
 
-  def test_copy_from_proto_success(self):
+  def test_copy_from_proto_shows_deprecation_warning(self):
+    with self.assertWarns(DeprecationWarning):
+      owner = model_card.Owner(name="my_name1")
+      owner_proto = model_card_pb2.Owner(
+          name="my_name2", contact="my_contact2"
+      )
+      owner.copy_from_proto(owner_proto)
+
+  def test_from_proto_success(self):
     # Test fields convert.
-    owner = model_card.Owner(name="my_name1")
     owner_proto = model_card_pb2.Owner(name="my_name2", contact="my_contact2")
-    owner.copy_from_proto(owner_proto)
+    owner = model_card.Owner.from_proto(owner_proto)
     self.assertEqual(
         owner, model_card.Owner(name="my_name2", contact="my_contact2")
     )
 
     # Test message convert.
-    model_details = model_card.ModelDetails(
-        owners=[model_card.Owner(name="my_name1")]
-    )
     model_details_proto = model_card_pb2.ModelDetails(
         owners=[model_card_pb2.Owner(name="my_name2", contact="my_contact2")]
     )
-    model_details.copy_from_proto(model_details_proto)
+    model_details = model_card.ModelDetails.from_proto(model_details_proto)
     self.assertEqual(
         model_details,
         model_card.ModelDetails(
@@ -104,16 +107,15 @@ def test_merge_from_proto_success(self):
         )
     )
 
-  def test_copy_from_proto_with_invalid_proto(self):
-    owner = model_card.Owner()
+  def test_from_proto_with_invalid_proto(self):
     wrong_proto = model_card_pb2.Version()
     with self.assertRaisesRegex(
         TypeError,
         "<class 'model_card_toolkit.proto.model_card_pb2.Owner'> is expected. "
         "However <class 'model_card_toolkit.proto.model_card_pb2.Version'> is "
         "provided."
     ):
-      owner.copy_from_proto(wrong_proto)
+      model_card.Owner.from_proto(wrong_proto)
 
   def test_merge_from_proto_with_invalid_proto(self):
     owner = model_card.Owner()
@@ -152,34 +154,16 @@ def test_to_proto_with_invalid_field(self):
     owner = model_card.Owner()
     owner.wrong_field = "wrong"
     with self.assertRaisesRegex(
-        ValueError, "has no such field named 'wrong_field'."
+        ValueError, "has no such field named \"wrong_field\"."
     ):
       owner.to_proto()
 
   def test_from_json_and_to_json_with_all_fields(self):
     want_json = json.loads(_FULL_JSON)
-    model_card_py = model_card.ModelCard()
-    model_card_py.from_json(want_json)
+    model_card_py = model_card.ModelCard.from_json(want_json)
     got_json = json.loads(model_card_py.to_json())
     self.assertEqual(want_json, got_json)
 
-  def test_from_json_overwrites_previous_fields(self):
-    overwritten_limitation = model_card.Limitation(
-        description="This model can only be used on text up to 140 characters."
-    )
-    overwritten_user = model_card.User(description="language researchers")
-    model_card_py = model_card.ModelCard(
-        considerations=model_card.Considerations(
-            limitations=[overwritten_limitation], users=[overwritten_user]
-        )
-    )
-    model_card_json = json.loads(_FULL_JSON)
-    model_card_py.from_json(model_card_json)
-    self.assertNotIn(
-        overwritten_limitation, model_card_py.considerations.limitations
-    )
-    self.assertNotIn(overwritten_user, model_card_py.considerations.users)
-
   def test_merge_from_json_does_not_overwrite_all_fields(self):
     # We want the "Limitations" field to be overwritten, but not "Users".
 
@@ -222,7 +206,7 @@ def test_merge_from_json_dict_and_str(self):
   def test_from_invalid_json(self):
     invalid_json_dict = {"model_name": "the_greatest_model"}
     with self.assertRaises(jsonschema.ValidationError):
-      model_card.ModelCard().from_json(invalid_json_dict)
+      model_card.ModelCard.from_json(invalid_json_dict)
 
   def test_from_invalid_json_vesion(self):
     model_card_dict = {
@@ -238,7 +222,7 @@ def test_from_invalid_json_vesion(self):
             "model card."
         )
     ):
-      model_card.ModelCard().from_json(model_card_dict)
+      model_card.ModelCard.from_json(model_card_dict)
 
   def test_from_proto_to_json(self):
     model_card_proto = text_format.Parse(
@@ -251,10 +235,10 @@ def test_from_proto_to_json(self):
         _FULL_JSON,
         model_card_py.merge_from_proto(model_card_proto).to_json()
     )
-    # Use copy_from_proto
+    # Use from_proto
     self.assertJsonEqual(
         _FULL_JSON,
-        model_card_py.copy_from_proto(model_card_proto).to_json()
+        model_card.ModelCard.from_proto(model_card_proto).to_json()
     )
 
   def test_from_json_to_proto(self):
@@ -263,8 +247,7 @@ def test_from_json_to_proto(self):
     )
 
     model_card_json = json.loads(_FULL_JSON)
-    model_card_py = model_card.ModelCard()
-    model_card_py.from_json(model_card_json)
+    model_card_py = model_card.ModelCard.from_json(model_card_json)
     model_card_json2proto = model_card_py.to_proto()
 
     self.assertEqual(model_card_proto, model_card_json2proto)