From 18366684f4bf3455beb310032a97eabd871ff514 Mon Sep 17 00:00:00 2001
From: Benjamin Bossan <benjamin.bossan@gmail.com>
Date: Thu, 16 Mar 2023 14:35:41 +0100
Subject: [PATCH] ENH: Default section for add_* methods

Description

The add_* methods for Card now have default sections (the one used in
the skops template) and no longer set descriptions by default. None is
no longer a valid argument for section.

This is useful because it allows us to completely remove all the
template checks in the diverse add_* methods. Before this change, every
add_* method had to check the template and if it was a custom template,
would raise an error when the section is not indicated.

All methods have been homogenized in that they no longer add a
description by default and put a placeholder there if empty. This means
that after this PR is merged, the same code will sometimes produce a
different looking model card.

Comment

For the Card class, this is a nice simplification of the code. The main
change affects the unit tests. All tests that used to raise when using a
custom template without a section argument no longer raise but use the
default template. Furthermore, since some default descriptions have been
removed, other tests had to be touched too. Apart from that, some minor
cleanups in the tests.
---
 docs/changes.rst              |   3 +
 skops/card/_model_card.py     |  96 +++++----------
 skops/card/_templates.py      |   4 +-
 skops/card/tests/test_card.py | 214 +++++++++++++++++-----------------
 4 files changed, 139 insertions(+), 178 deletions(-)

diff --git a/docs/changes.rst b/docs/changes.rst
index 2a6438a3..bf0d29ea 100644
--- a/docs/changes.rst
+++ b/docs/changes.rst
@@ -22,6 +22,9 @@ v0.6
   `Benjamin Bossan`_.
 - Fix: skops persistence now also works with many functions from the
   :mod:`operator` module. :pr:`287` by `Benjamin Bossan`_.
+- ``add_*`` methods on :class:`.Card` now have default section names (but
+  ``None`` is no longer valid) and no longer add descriptions by default.
+  :pr:`321` by `Benjamin Bossan`_.
 
 v0.5
 ----
diff --git a/skops/card/_model_card.py b/skops/card/_model_card.py
index 42931031..5ab5b25f 100644
--- a/skops/card/_model_card.py
+++ b/skops/card/_model_card.py
@@ -791,7 +791,7 @@ def _add_single(self, key: str, val: str | Section) -> Section:
 
     def add_model_plot(
         self,
-        section: str | None = None,
+        section: str = "Model description/Training Procedure/Model Plot",
         description: str | None = None,
     ) -> Self:
         """Add a model plot
@@ -805,11 +805,10 @@ def add_model_plot(
 
         Parameters
         ----------
-        section : str or None, default=None
-            The section that the model plot should be added to. If you're using
-            the default skops template, you can leave this parameter as
-            ``None``, otherwise you have to indicate the section. If the section
-            does not exist, it will be created for you.
+        section : str (default="Model description/Training Procedure/Model Plot")
+            The section that the model plot should be added to. By default, the
+            section is set to fit the skops model card template. If you're using
+            a different template, you may have to choose a different section name.
 
         description : str or None, default=None
             An optional description to be added before the model plot. If you're
@@ -821,18 +820,8 @@ def add_model_plot(
         -------
         self : object
             Card object.
-        """
-        if section is None:
-            if self.template == Templates.skops.value:
-                section = "Model description/Training Procedure/Model Plot"
-            else:
-                msg = NEED_SECTION_ERR_MSG.format(action="add a model plot")
-                raise ValueError(msg)
-
-        if description is None:
-            if self.template == Templates.skops.value:
-                description = "The model plot is below."
 
+        """
         self._add_model_plot(
             self.get_model(), section_name=section, description=description
         )
@@ -864,17 +853,19 @@ def _add_model_plot(
         self._add_single(section_name, section)
 
     def add_hyperparams(
-        self, section: str | None = None, description: str | None = None
+        self,
+        section: str = "Model description/Training Procedure/Hyperparameters",
+        description: str | None = None,
     ) -> Self:
         """Add the model's hyperparameters as a table
 
         Parameters
         ----------
-        section : str or None, default=None
-            The section that the hyperparamters should be added to. If you're
-            using the default skops template, you can leave this parameter as
-            ``None``, otherwise you have to indicate the section. If the section
-            does not exist, it will be created for you.
+        section : str (default="Model description/Training Procedure/Hyperparameters")
+            The section that the hyperparameters should be added to. By default,
+            the section is set to fit the skops model card template. If you're
+            using a different template, you may have to choose a different section
+            name.
 
         description : str or None, default=None
             An optional description to be added before the hyperparamters. If
@@ -888,17 +879,6 @@ def add_hyperparams(
             Card object.
 
         """
-        if section is None:
-            if self.template == Templates.skops.value:
-                section = "Model description/Training Procedure/Hyperparameters"
-            else:
-                msg = NEED_SECTION_ERR_MSG.format(action="add model hyperparameters")
-                raise ValueError(msg)
-
-        if description is None:
-            if self.template == Templates.skops.value:
-                description = "The model is trained with below hyperparameters."
-
         self._add_hyperparams(
             self.get_model(), section_name=section, description=description
         )
@@ -924,7 +904,7 @@ def _add_hyperparams(
 
     def add_get_started_code(
         self,
-        section: str | None = None,
+        section: str = "How to Get Started with the Model",
         description: str | None = None,
         file_name: str | None = None,
         model_format: Literal["pickle", "skops"] | None = None,
@@ -936,11 +916,11 @@ def add_get_started_code(
 
         Parameters
         ----------
-        section : str or None, default=None
-            The section that the code should be added to. If you're using the
-            default skops template, you can leave this parameter as ``None``,
-            otherwise you have to indicate the section. If the section does not
-            exist, it will be created for you.
+        section : str (default="How to Get Started with the Model")
+            The section that the code for loading the model should be added to.
+            By default, the section is set to fit the skops model card template.
+            If you're using a different template, you may have to choose a
+            different section name.
 
         description : str or None, default=None
             An optional description to be added before the code. If you're using
@@ -984,17 +964,6 @@ def add_get_started_code(
         if (not file_name) or (not model_format):
             return self
 
-        if section is None:
-            if self.template == Templates.skops.value:
-                section = "How to Get Started with the Model"
-            else:
-                msg = NEED_SECTION_ERR_MSG.format(action="add get started code")
-                raise ValueError(msg)
-
-        if description is None:
-            if self.template == Templates.skops.value:
-                description = "Use the code below to get started with the model."
-
         self._add_get_started_code(
             section,
             file_name=file_name,
@@ -1157,7 +1126,7 @@ def add_table(
 
     def add_metrics(
         self,
-        section: str | None = None,
+        section: str = "Model description/Evaluation Results",
         description: str | None = None,
         **kwargs: str | int | float,
     ) -> Self:
@@ -1167,11 +1136,10 @@ def add_metrics(
 
         Parameters
         ----------
-        section : str or None, default=None
-            The section that the metrics should be added to. If you're using the
-            default skops template, you can leave this parameter as ``None``,
-            otherwise you have to indicate the section. If the section does not
-            exist, it will be created for you.
+        section : str (default="Model description/Evaluation Results")
+            The section that metrics should be added to. By default, the section
+            is set to fit the skops model card template. If you're using a
+            different template, you may have to choose a different section name.
 
         description : str or None, default=None
             An optional description to be added before the metrics. If you're
@@ -1186,20 +1154,8 @@ def add_metrics(
         -------
         self : object
             Card object.
-        """
-        if section is None:
-            if self.template == Templates.skops.value:
-                section = "Model description/Evaluation Results"
-            else:
-                msg = NEED_SECTION_ERR_MSG.format(action="add metrics")
-                raise ValueError(msg)
 
-        if description is None:
-            if self.template == Templates.skops.value:
-                description = (
-                    "You can find the details about evaluation process and "
-                    "the evaluation results."
-                )
+        """
         self._metrics.update(kwargs)
         self._add_metrics(section, description=description, metrics=self._metrics)
         return self
diff --git a/skops/card/_templates.py b/skops/card/_templates.py
index d30a39a7..e343c427 100644
--- a/skops/card/_templates.py
+++ b/skops/card/_templates.py
@@ -35,9 +35,9 @@ class Templates(Enum):
 SKOPS_TEMPLATE = {
     "Model description": CONTENT_PLACEHOLDER,
     "Model description/Intended uses & limitations": CONTENT_PLACEHOLDER,
-    "Model description/Training Procedure": "",
+    "Model description/Training Procedure": CONTENT_PLACEHOLDER,
     "Model description/Training Procedure/Hyperparameters": CONTENT_PLACEHOLDER,
-    "Model description/Training Procedure/Model Plot": "The model plot is below.",
+    "Model description/Training Procedure/Model Plot": CONTENT_PLACEHOLDER,
     "Model description/Evaluation Results": CONTENT_PLACEHOLDER,
     "How to Get Started with the Model": CONTENT_PLACEHOLDER,
     "Model Card Authors": (
diff --git a/skops/card/tests/test_card.py b/skops/card/tests/test_card.py
index 82043bba..7a3dfb27 100644
--- a/skops/card/tests/test_card.py
+++ b/skops/card/tests/test_card.py
@@ -18,6 +18,7 @@
 from skops import hub_utils
 from skops.card import Card, metadata_from_config
 from skops.card._model_card import (
+    CONTENT_PLACEHOLDER,
     SKOPS_TEMPLATE,
     PlotSection,
     Section,
@@ -153,9 +154,9 @@ class TestAddModelPlot:
     def test_default(self, model_card):
         result = model_card.select(
             "Model description/Training Procedure/Model Plot"
-        ).content
+        ).format()
         # don't compare whole text, as it's quite long and non-deterministic
-        assert result.startswith("The model plot is below.\n\n<style>#sk-")
+        assert result.startswith("<style>#sk-")
         assert "<style>" in result
         assert result.endswith(
             "<pre>LinearRegression()</pre></div></div></div></div></div>"
@@ -164,7 +165,7 @@ def test_default(self, model_card):
     def test_no_overflow(self, model_card):
         result = model_card.select(
             "Model description/Training Procedure/Model Plot"
-        ).content
+        ).format()
         # test if the model doesn't overflow the huggingface models page
         assert result.count("sk-top-container") == 1
         assert 'style="overflow: auto;' in result
@@ -174,8 +175,8 @@ def test_model_diagram_false(self):
         model_card = Card(model, model_diagram=False)
         result = model_card.select(
             "Model description/Training Procedure/Model Plot"
-        ).content
-        assert result == "The model plot is below."
+        ).format()
+        assert result == CONTENT_PLACEHOLDER
 
     def test_model_diagram_str(self):
         # if passing a str, use that as the section name
@@ -187,11 +188,11 @@ def test_model_diagram_str(self):
         result = model_card.select(
             "Model description/Training Procedure/Model Plot"
         ).format()
-        assert result == "The model plot is below."
+        assert result == CONTENT_PLACEHOLDER
 
         # now check that the actual model diagram is in the other section
         result = model_card.select(other_section_name).format()
-        assert result.startswith("The model plot is below.\n\n<style>#sk-")
+        assert result.startswith("<style>#sk-")
         assert "<style>" in result
         assert result.endswith(
             "<pre>LinearRegression()</pre></div></div></div></div></div>"
@@ -200,29 +201,34 @@ def test_model_diagram_str(self):
     def test_other_section(self, model_card):
         model_card.add_model_plot(section="Other section")
         result = model_card.select("Other section").content
-        assert result.startswith("The model plot is below.\n\n<style>#sk-")
+        assert result.startswith("<style>#sk-")
         assert "<style>" in result
         assert result.endswith(
             "<pre>LinearRegression()</pre></div></div></div></div></div>"
         )
 
-    def test_other_description(self, model_card):
+    def test_with_description(self, model_card):
         model_card.add_model_plot(description="Awesome diagram below")
         result = model_card.select(
             "Model description/Training Procedure/Model Plot"
-        ).content
+        ).format()
         assert result.startswith("Awesome diagram below\n\n<style>#sk-")
 
     @pytest.mark.parametrize("template", CUSTOM_TEMPLATES)
-    def test_custom_template_no_section_raises(self, template):
+    def test_custom_template_no_section_uses_default(self, template):
         model = fit_model()
         model_card = Card(model, template=template)
-        msg = (
-            "You are trying to add a model plot but you're using a custom template, "
-            "please pass the 'section' argument to determine where to put the content"
+        model_card.add_model_plot()
+        result = model_card.select(
+            "Model description/Training Procedure/Model Plot"
+        ).format()
+
+        # don't compare whole text, as it's quite long and non-deterministic
+        assert result.startswith("<style>#sk-")
+        assert "<style>" in result
+        assert result.endswith(
+            "<pre>LinearRegression()</pre></div></div></div></div></div>"
         )
-        with pytest.raises(ValueError, match=msg):
-            model_card.add_model_plot()
 
     @pytest.mark.parametrize("template", CUSTOM_TEMPLATES)
     def test_custom_template_init_str_works(self, template):
@@ -244,26 +250,31 @@ def test_default_template_and_model_diagram_true(self, model_card):
         model_card = Card(model, model_diagram=True)
         result = model_card.select(
             "Model description/Training Procedure/Model Plot"
-        ).content
+        ).format()
         # don't compare whole text, as it's quite long and non-deterministic
-        assert result.startswith("The model plot is below.\n\n<style>#sk-")
+        assert result.startswith("<style>#sk-")
         assert "<style>" in result
         assert result.endswith(
             "<pre>LinearRegression()</pre></div></div></div></div></div>"
         )
 
     @pytest.mark.parametrize("template", CUSTOM_TEMPLATES)
-    def test_custom_template_and_model_diagram_true(self, model_card, template):
-        # in contrast to the previous test, when setting model_diagram=True but
-        # using a custom template, we expect an error during initialization of
-        # the model cord
+    def test_custom_template_and_model_diagram_true_uses_default(
+        self, model_card, template
+    ):
+        # when using a custom template and not indicating a section, use the
+        # default section
         model = fit_model()
-        msg = (
-            "You are trying to add a model plot but you're using a custom template, "
-            "please pass the 'section' argument to determine where to put the content"
+        Card(model, template=template, model_diagram=True)
+        result = model_card.select(
+            "Model description/Training Procedure/Model Plot"
+        ).format()
+        # don't compare whole text, as it's quite long and non-deterministic
+        assert result.startswith("<style>#sk-")
+        assert "<style>" in result
+        assert result.endswith(
+            "<pre>LinearRegression()</pre></div></div></div></div></div>"
         )
-        with pytest.raises(ValueError, match=msg):
-            Card(model, template=template, model_diagram=True)
 
     def test_add_twice(self, model_card):
         # it's possible to add the section twice, even if it doesn't make a lot
@@ -294,8 +305,6 @@ class TestAddHyperparams:
     @pytest.fixture
     def expected(self):
         lines = [
-            "The model is trained with below hyperparameters.",
-            "",
             "<details>",
             "<summary> Click to expand </summary>",
             "",
@@ -314,7 +323,7 @@ def expected(self):
         major, minor, *_ = sklearn.__version__.split(".")
         major, minor = int(major), int(minor)
         if (major >= 1) and (minor >= 2):
-            del lines[10]
+            del lines[8]
 
         table = "\n".join(lines)
         # remove multiple whitespaces and dashes, as they're not important and may
@@ -344,7 +353,7 @@ def test_other_section(self, model_card, expected):
         result = _strip_multiple_chars(result, "-")
         assert result == expected
 
-    def test_other_description(self, model_card, expected):
+    def test_with_description(self, model_card, expected):
         model_card.add_hyperparams(description="Awesome hyperparams")
         result = model_card.select(
             "Model description/Training Procedure/Hyperparameters"
@@ -352,16 +361,19 @@ def test_other_description(self, model_card, expected):
         assert result.startswith("Awesome hyperparams")
 
     @pytest.mark.parametrize("template", CUSTOM_TEMPLATES)
-    def test_custom_template_no_section_raises(self, template):
+    def test_custom_template_no_section_uses_default(self, template, expected):
         model = fit_model()
         model_card = Card(model, template=template)
-        msg = (
-            "You are trying to add model hyperparameters but you're using a custom "
-            "template, please pass the 'section' argument to determine where to put "
-            "the content"
-        )
-        with pytest.raises(ValueError, match=msg):
-            model_card.add_hyperparams()
+        model_card.add_hyperparams()
+        result = model_card.select(
+            "Model description/Training Procedure/Hyperparameters"
+        ).format()
+
+        # remove multiple whitespaces and dashes, as they're not important and may
+        # differ depending on OS
+        result = _strip_multiple_chars(result, " ")
+        result = _strip_multiple_chars(result, "-")
+        assert result == expected
 
     def test_add_twice(self, model_card):
         # it's possible to add the section twice, even if it doesn't make a lot
@@ -397,16 +409,14 @@ class TestAddMetrics:
     def test_default(self, model_card):
         # by default, don't add a table, as there are no metrics
         result = model_card.select("Model description/Evaluation Results").format()
-        expected = "[More Information Needed]"
+        expected = CONTENT_PLACEHOLDER
         assert result == expected
 
     def test_empty_metrics_table(self, model_card):
         model_card.add_metrics()
         result = model_card.select("Model description/Evaluation Results").format()
         expected = (
-            "You can find the details about evaluation process and the evaluation "
-            "results.\n\n"
-            "| Metric   | Value   |\n"
+            "| Metric   | Value   |\n"  # fmt: skip
             "|----------|---------|"
         )
         assert result == expected
@@ -419,8 +429,6 @@ def test_multiple_metrics(self, model_card):
         )
         result = model_card.select("Model description/Evaluation Results").format()
         expected = (
-            "You can find the details about evaluation process and the evaluation "
-            "results.\n\n"
             "| Metric      |   Value |\n"
             "|-------------|---------|\n"
             "| acc         |     0.1 |\n"
@@ -432,30 +440,35 @@ def test_multiple_metrics(self, model_card):
     def test_other_section(self, model_card):
         model_card.add_metrics(accuracy=0.9, section="Other section")
         result = model_card.select("Other section").format()
+        # fmt: off
         expected = (
-            "You can find the details about evaluation process and the evaluation "
-            "results.\n\n"
             "| Metric   |   Value |\n"
             "|----------|---------|\n"
             "| accuracy |     0.9 |"
         )
+        # fmt: on
         assert result == expected
 
-    def test_other_description(self, model_card):
+    def test_with_description(self, model_card):
         model_card.add_metrics(accuracy=0.9, description="Awesome metrics")
         result = model_card.select("Model description/Evaluation Results").format()
-        assert result.startswith("Awesome metrics")
+        assert result.startswith("Awesome metrics\n\n| Metric ")
 
     @pytest.mark.parametrize("template", CUSTOM_TEMPLATES)
     def test_custom_template_no_section_raises(self, template):
         model = fit_model()
         model_card = Card(model, template=template)
-        msg = (
-            "You are trying to add metrics but you're using a custom template, "
-            "please pass the 'section' argument to determine where to put the content"
+        model_card.add_metrics(accuracy=0.9)
+
+        result = model_card.select("Model description/Evaluation Results").format()
+        # fmt: off
+        expected = (
+            "| Metric   |   Value |\n"
+            "|----------|---------|\n"
+            "| accuracy |     0.9 |"
         )
-        with pytest.raises(ValueError, match=msg):
-            model_card.add_metrics(accuracy=0.9)
+        # fmt: on
+        assert result == expected
 
     def test_add_twice(self, model_card):
         # it's possible to add the section twice, even if it doesn't make a lot
@@ -623,11 +636,9 @@ def model_card_skops(self, metadata_skops):
         card = Card(model, metadata=metadata_skops)
         return card
 
-    def test_default_pickle(self, model_card):
-        # by default, don't add a table, as there are no metrics
-        result = model_card.select("How to Get Started with the Model").format()
-        expected = (
-            "Use the code below to get started with the model.\n\n"
+    @pytest.fixture
+    def text_pickle(self):
+        return (
             "```python\n"
             "import json\n"
             "import pandas as pd\n"
@@ -638,13 +649,10 @@ def test_default_pickle(self, model_card):
             'model.predict(pd.DataFrame.from_dict(config["sklearn"]["example_input"]))\n'
             "```"
         )
-        assert result == expected
 
-    def test_default_skops(self, model_card_skops):
-        # by default, don't add a table, as there are no metrics
-        result = model_card_skops.select("How to Get Started with the Model").format()
-        expected = (
-            "Use the code below to get started with the model.\n\n"
+    @pytest.fixture
+    def text_skops(self):
+        return (
             "```python\n"
             "import json\n"
             "import pandas as pd\n"
@@ -655,7 +663,16 @@ def test_default_skops(self, model_card_skops):
             'model.predict(pd.DataFrame.from_dict(config["sklearn"]["example_input"]))\n'
             "```"
         )
-        assert result == expected
+
+    def test_default_pickle(self, model_card, text_pickle):
+        # by default, don't add a table, as there are no metrics
+        result = model_card.select("How to Get Started with the Model").format()
+        assert result == text_pickle
+
+    def test_default_skops(self, model_card_skops, text_skops):
+        # by default, don't add a table, as there are no metrics
+        result = model_card_skops.select("How to Get Started with the Model").format()
+        assert result == text_skops
 
     def test_no_metadata_file_name(self):
         model = fit_model()
@@ -674,50 +691,28 @@ def to_dict(self):
         card = Card(model, metadata=Metadata())
         card.add_get_started_code()  # does not raise
 
-    def test_other_section(self, model_card):
+    def test_other_section(self, model_card, text_pickle):
         model_card.add_get_started_code(section="Other section")
         result = model_card.select("Other section").format()
-        expected = "Use the code below to get started with the model."
-        assert result.startswith(expected)
+        assert result == text_pickle
 
-    def test_other_description(self, model_card):
+    def test_use_description(self, model_card):
         model_card.add_get_started_code(description="Awesome code")
         result = model_card.select("How to Get Started with the Model").format()
         assert result.startswith("Awesome code")
 
-    def test_other_filename(self, model_card):
+    def test_other_filename(self, model_card, text_pickle):
         model_card.add_get_started_code(file_name="foobar.pkl")
+        text = text_pickle.replace("my-model.pickle", "foobar.pkl")
         result = model_card.select("How to Get Started with the Model").format()
-        expected = (
-            "Use the code below to get started with the model.\n\n"
-            "```python\n"
-            "import json\n"
-            "import pandas as pd\n"
-            "import joblib\n"
-            'model = joblib.load("foobar.pkl")\n'
-            'with open("config.json") as f:\n'
-            "    config = json.load(f)\n"
-            'model.predict(pd.DataFrame.from_dict(config["sklearn"]["example_input"]))\n'
-            "```"
-        )
-        assert result == expected
+        assert result == text
 
-    def test_other_model_format(self, model_card):
+    def test_explicitly_set_other_model_format(self, model_card, text_skops):
         model_card.add_get_started_code(model_format="skops")
         result = model_card.select("How to Get Started with the Model").format()
-        expected = (
-            "Use the code below to get started with the model.\n\n"
-            "```python\n"
-            "import json\n"
-            "import pandas as pd\n"
-            "import skops.io as sio\n"
-            'model = sio.load("my-model.pickle")\n'
-            'with open("config.json") as f:\n'
-            "    config = json.load(f)\n"
-            'model.predict(pd.DataFrame.from_dict(config["sklearn"]["example_input"]))\n'
-            "```"
-        )
-        assert result == expected
+        # file name is still "my-model.pickle", only the loading code changes
+        text = text_skops.replace(".skops", ".pickle")
+        assert result == text
 
     def test_invalid_model_format_passed(self, model_card):
         # json is not a valid model format
@@ -743,15 +738,22 @@ def to_dict(self):
             Card(model, metadata=Metadata())
 
     @pytest.mark.parametrize("template", CUSTOM_TEMPLATES)
-    def test_custom_template_no_section_raises(self, template):
+    def test_custom_template_no_section_uses_default(self, template, text_pickle):
         model = fit_model()
-        model_card = Card(model, template=template)
-        msg = (
-            "You are trying to add get started code but you're using a custom template,"
-            " please pass the 'section' argument to determine where to put the content"
-        )
-        with pytest.raises(ValueError, match=msg):
-            model_card.add_get_started_code(file_name="foo.bar", model_format="skops")
+
+        class Metadata:
+            def to_dict(self):
+                return {
+                    "model_file": "my-model.pickle",
+                    "sklearn": {
+                        "model_format": "pickle",
+                    },
+                }
+
+        model_card = Card(model, metadata=Metadata(), template=template)
+        model_card.add_get_started_code()
+        result = model_card.select("How to Get Started with the Model").format()
+        assert result == text_pickle
 
     def test_add_twice(self, model_card):
         # it's possible to add the section twice, even if it doesn't make a lot