Repository: caikit/caikit
Pull request #72 (2023-04-26 21:28:37)

test: Docstrings module tests
This PR is part of Issue #15

- refactors the 3 methods in `docstrings.py` that use 3rd party `docstring_parser.parse` [function](https://github.com/rr-/docstring_parser/blob/master/docstring_parser/parser.py#L23) to instead of checking for None, handle the `ParseError` exception. 
- adjusts the tests to mock the exception raised. I don't think we ever get this exception because we call `docstring_parser.parse` with Auto style, we always get a Docstring object back, albeit a Docstring object with empty params.
- add more tests to increase docstring test coverage:
![Screen Shot 2023-04-26 at 3 18 01 PM](https://user-images.githubusercontent.com/1454122/234706956-b0b1b66f-336c-41c7-aebe-932ab0212b34.png)


**Updated:**
- When handling `ParseError` exception, this PR adds a raise for `CaikitRuntimeException` and lets the callers of those 3 methods in `parsers.py` decide what to do with that exception. In this case, these callers log a warning that the docstring was not parseable and continue their type deduction task without docstring.
- Also figured I'd add tests to cover the rest of `module_signature.py` file to bring the coverage of `signature_parsing` module to 100%
![Screen Shot 2023-04-27 at 4 19 53 PM](https://user-images.githubusercontent.com/1454122/235004229-649a7254-750c-4f94-9aa3-20ac48aaa74c.png)



Patch:
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
index 8d1a6f6b..35cb6eb6 100644
--- a/CONTRIBUTING.md
+++ b/CONTRIBUTING.md
@@ -2,10 +2,12 @@
 
 👍🎉 First off, thank you for taking the time to contribute! 🎉👍
 
-The following is a set of guidelines for contributing. These are just guidelines, not rules. Use your best judgment, and feel free to propose changes to this document in a pull request. Please [read community contribution guide](TODO) for general practices for the Caikit community.
+The following is a set of guidelines for contributing. These are just guidelines, not rules. Use your best judgment, and feel free to propose changes to this document in a pull request.
 
 ## What Should I Know Before I Get Started?
 
+If you're new to GitHub and working with open source repositories, this section will be helpful. Otherwise, you can skip to learning how to [set up your dev environment](#set-up-your-dev-environment).
+
 ### Code of Conduct
 
 This project adheres to the [Contributor Covenant](./CODE-OF-CONDUCT.md). By participating, you are expected to uphold this code.
@@ -20,24 +22,7 @@ Help on open source projects is always welcome and there is always something tha
 
 ## How Can I Contribute?
 
-When contributing, it's useful to start by looking at [issues](https://github.com/caikit/caikit/issues). After picking up an issue, writing code, or updating a document, make a pull request and your work will be reviewed and merged. If you're adding a new feature or find a bug, it's best to [write an issue](https://github.com/caikit/caikit/issues/new?assignees=&labels=&template=feature_request.md&title=) first to discuss it with maintainers.
-
-To contribute to this repo, you'll use the Fork and Pull model common in many open source repositories. For details on this process, check out [The GitHub Workflow
-Guide](https://github.com/kubernetes/community/blob/master/contributors/guide/github-workflow.md)
-from Kubernetes.
-
-When your contribution is ready, you can create a pull request. Pull requests are often referred to as "PR". In general, we follow the standard [github pull request](https://help.github.com/en/articles/about-pull-requests) process. Follow the template to provide details about your pull request to the maintainers.
-
-Before sending pull requests, make sure your changes pass formatting, linting and unit tests.
-
-#### Code Review
-
-Once you've [created a pull request](#how-can-i-contribute), maintainers will review your code and may make suggestions to fix before merging. It will be easier for your pull request to receive reviews if you consider the criteria the reviewers follow while working. Remember to:
-
-- Run tests locally and ensure they pass
-- Follow the project coding conventions
-- Write detailed commit messages
-- Break large changes into a logical series of smaller patches, which are easy to understand individually and combine to solve a broader issue
+When contributing, it's useful to start by looking at [issues](https://github.com/caikit/caikit/issues). After picking up an issue, writing code, or updating a document, make a pull request and your work will be reviewed and merged. If you're adding a new feature, it's best to [write an issue](https://github.com/caikit/caikit/issues/new?assignees=&labels=&template=feature_request.md&title=) first to discuss it with maintainers first.
 
 ### Reporting Bugs
 
@@ -63,29 +48,20 @@ Improvements to existing functionality are tracked as [GitHub issues using the U
 
 ### Set up your dev environment
 
-The following tools are required:
-
-- [git](https://git-scm.com)
-- [python](https://www.python.org) (v3.8+)
-- [pip](https://pypi.org/project/pip/) (v23.0+)
-
-You can setup your dev environment using [tox](https://tox.wiki/en/latest/), an environment orchestrator which allows for setting up environments for and invoking builds, unit tests, formatting, linting, etc. Install tox with:
+Your dev environment is set up by [tox](https://tox.wiki/en/latest/), an environment orchestrator which allows for setting up environments for and invoking builds, unit tests, formatting, linting, etc. Install tox with:
 
 ```sh
 pip install -r setup_requirements.txt
 ```
 
 If you want to manage your own virtual environment instead of using `tox`, you can install `caikit` and all dependencies with:
-
 ```sh
 pip install .
 ```
 
-### Unit tests
-
-Unit tests are enforced by the CI system. When making changes, run the tests before pushing the changes to avoid CI issues.
+### Run unit tests
 
-Running unit tests against all supported Python versions is as simple as:
+Running tests against all supported Python versions is as simple as:
 
 ```sh
 tox
@@ -97,25 +73,18 @@ Running tests against a single Python version can be done with:
 tox -e 3.9
 ```
 
-### Coding style
 
-Caikit follows the python [pep8](https://peps.python.org/pep-0008/) coding style. The coding style is enforced by the CI system, and your PR will fail until the style has been applied correctly.
+### Code formatting
 
-We use [pre-commit](https://pre-commit.com/) to enforce coding style using [black](https://github.com/psf/black), [prettier](https://github.com/prettier/prettier) and [isort](https://pycqa.github.io/isort/).
+This project uses [pre-commit](https://pre-commit.com/) to enforce coding style using [black](https://github.com/psf/black).
 
 You can invoke formatting with:
-
 ```sh
 tox -e fmt
 ```
 
-In addition, we use [pylint](https://www.pylint.org) to perform static code analysis of the code.
-
-You can invoke the linting with the following command
+Coding style is enforced by the CI tests, so if not installed locally, your PR will fail until formatting has been applied.
 
-```sh
-tox -e lint
-```
 
 ## Your First Code Contribution
 
@@ -124,6 +93,23 @@ Unsure where to begin contributing? You can start by looking through these issue
 - Issues with the [`good first issue` label](https://github.com/caikit/caikit/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22) - these should only require a few lines of code and are good targets if you're just starting contributing.
 - Issues with the [`help wanted` label](https://github.com/caikit/caikit/issues?q=is%3Aopen+is%3Aissue+label%3A%22help+wanted%22) - these range from simple to more complex, but are generally things we want but can't get to in a short time frame.
 
+### How to contribute
+
+To contribute to this repo, you'll use the Fork and Pull model common in many open source repositories. For details on this process, watch [how to contribute](https://egghead.io/courses/how-to-contribute-to-an-open-source-project-on-github).
+
+When ready, you can create a pull request. Pull requests are often referred to as "PR". In general, we follow the standard [github pull request](https://help.github.com/en/articles/about-pull-requests) process. Follow the template to provide details about your pull request to the maintainers.
+
+Before sending pull requests, make sure your changes pass tests.
+
+#### Code Review
+
+Once you've [created a pull request](#how-to-contribute), maintainers will review your code and likely make suggestions to fix before merging. It will be easier for your pull request to receive reviews if you consider the criteria the reviewers follow while working. Remember to:
+
+- Run tests locally and ensure they pass
+- Follow the project coding conventions
+- Write detailed commit messages
+- Break large changes into a logical series of smaller patches, which are easy to understand individually and combine to solve a broader issue
+
 <!-- ## Releasing (Maintainers only)
 
 The responsibility for releasing new versions of the libraries falls to the maintainers. Releases will follow standard [semantic versioning](https://semver.org/) and be hosted on [pypi](https://pypi.org/project/jtd-to-proto/). -->
diff --git a/caikit/core/data_model/base.py b/caikit/core/data_model/base.py
index 76a72aa0..0fe0698b 100644
--- a/caikit/core/data_model/base.py
+++ b/caikit/core/data_model/base.py
@@ -256,7 +256,7 @@ class _DataBaseMetaClass(type):
     def _make_property_getter(mcs, field):
         """This helper creates an @property attribute getter for the given field
 
-        NOTE: This needs to live as a standalone funciton in order for the given
+        NOTE: This needs to live as a standalone function in order for the given
             field name to be properly bound to the closure for the attrs
         """
         private_name = f"_{field}"
diff --git a/caikit/runtime/service_generation/create_service.py b/caikit/runtime/service_generation/create_service.py
index 56b30378..ad157350 100644
--- a/caikit/runtime/service_generation/create_service.py
+++ b/caikit/runtime/service_generation/create_service.py
@@ -123,11 +123,12 @@ def _remove_non_primitive_modules(
     # If the module is not "primitive" we won't include it
     for ck_module in modules:
         signature = CaikitCoreModuleMethodSignature(ck_module, "run")
-        if not is_primitive_method(signature, primitive_data_model_types):
-            log.debug("Skipping non-primitive module %s", ck_module)
-            continue
+        if signature.parameters and signature.return_type:
+            if not is_primitive_method(signature, primitive_data_model_types):
+                log.debug("Skipping non-primitive module %s", ck_module)
+                continue
 
-        primitive_modules.append(ck_module)
+            primitive_modules.append(ck_module)
     return primitive_modules
 
 
diff --git a/caikit/runtime/service_generation/signature_parsing/docstrings.py b/caikit/runtime/service_generation/signature_parsing/docstrings.py
index a233b171..90ba4a72 100644
--- a/caikit/runtime/service_generation/signature_parsing/docstrings.py
+++ b/caikit/runtime/service_generation/signature_parsing/docstrings.py
@@ -21,12 +21,14 @@ import sys
 
 # Third Party
 import docstring_parser
+import grpc
 
 # First Party
 import alog
 
 # Local
 from caikit.core.data_model.base import DataBase
+from caikit.runtime.types.caikit_runtime_exception import CaikitRuntimeException
 import caikit.core
 
 log = alog.use_channel("DOCSTRINGS")
@@ -42,10 +44,13 @@ def get_return_type(fn: Callable) -> Optional[Type]:
     Returns:
         The return type of `fn`, if it can be parsed from the docstring. Otherwise, None
     """
-    docstring = docstring_parser.parse(fn.__doc__)
-    if not docstring:
-        log.warning("Failed to parse the docstring")
-        return None
+    try:
+        docstring = docstring_parser.parse(fn.__doc__)
+    except Exception as exc:
+        raise CaikitRuntimeException(
+            grpc.StatusCode.INVALID_ARGUMENT,
+            f"ParseError when parsing docstring for function: {fn.__name__}",
+        ) from exc
 
     type_names, desc_names = _get_candidate_type_names_from_docstring(docstring.returns)
 
@@ -68,10 +73,13 @@ def is_optional(fn: Callable, arg_name: str) -> bool:
         arg_name: The name of the parameter that we should try to get the type of
             e.g. "raw_document"
     """
-    docstring = docstring_parser.parse(fn.__doc__)
-    if not docstring:
-        log.warning("Failed to parse the docstring for %s", fn.__name__)
-        return False
+    try:
+        docstring = docstring_parser.parse(fn.__doc__)
+    except Exception as exc:
+        raise CaikitRuntimeException(
+            grpc.StatusCode.INVALID_ARGUMENT,
+            f"ParseError when parsing docstring for function: {fn.__name__}",
+        ) from exc
 
     ds_param = [param for param in docstring.params if param.arg_name == arg_name]
     if ds_param:
@@ -88,6 +96,7 @@ def is_optional(fn: Callable, arg_name: str) -> bool:
                     return True
 
         return False
+    return False
 
 
 def get_arg_type(fn: Callable, arg_name: str) -> Optional[Type]:
@@ -102,10 +111,13 @@ def get_arg_type(fn: Callable, arg_name: str) -> Optional[Type]:
         The return type of `fn`, if it can be parsed from the docstring. Otherwise, None
     """
 
-    docstring = docstring_parser.parse(fn.__doc__)
-    if not docstring:
-        log.warning("Failed to parse the docstring for function %s", fn.__name__)
-        return None
+    try:
+        docstring = docstring_parser.parse(fn.__doc__)
+    except Exception as exc:
+        raise CaikitRuntimeException(
+            grpc.StatusCode.INVALID_ARGUMENT,
+            f"ParseError when parsing docstring for function: {fn.__name__}",
+        ) from exc
 
     ds_param = [param for param in docstring.params if param.arg_name == arg_name]
     if ds_param:
@@ -196,7 +208,7 @@ def _get_docstring_type(
             log.debug2(f"Found valid candidate type on sys.modules: {candidate_type}")
             continue
 
-        # If the type was not fully qualified (like a `RawDocument`), look in a couple well known
+        # If the type was not fully qualified (like a `ProducerId`), look in a couple well known
         # places - the caikit core data model itself
         candidate_type = _extract_type_from_pymodule(
             caikit.interfaces.common.data_model, type_name
diff --git a/caikit/runtime/service_generation/signature_parsing/parsers.py b/caikit/runtime/service_generation/signature_parsing/parsers.py
index acc1ce56..e9e92509 100644
--- a/caikit/runtime/service_generation/signature_parsing/parsers.py
+++ b/caikit/runtime/service_generation/signature_parsing/parsers.py
@@ -25,6 +25,7 @@ import alog
 from . import docstrings
 from caikit.core.data_model.base import DataBase
 from caikit.core.module import ModuleBase
+from caikit.runtime.types.caikit_runtime_exception import CaikitRuntimeException
 
 log = alog.use_channel("SIG-PARSING")
 
@@ -59,7 +60,12 @@ def get_output_type_name(
             return fn_signature.return_annotation
 
     # Check the docstring
-    type_from_docstring = docstrings.get_return_type(fn)
+    type_from_docstring = None
+    try:
+        type_from_docstring = docstrings.get_return_type(fn)
+    except CaikitRuntimeException:
+        log.warning("Failed to parse the docstring for %s", fn)
+
     if type_from_docstring:
         return type_from_docstring
 
@@ -133,7 +139,11 @@ def _get_argument_type(
         return dm_type_from_known_arg_types
 
     # Check docstring for optional arg
-    optional_arg = docstrings.is_optional(module_method, arg.name)
+    optional_arg = False
+    try:
+        optional_arg = docstrings.is_optional(module_method, arg.name)
+    except CaikitRuntimeException:
+        log.warning("Failed to parse the docstring for %s", module_method)
 
     # Look for a type annotation
     if arg.annotation != inspect.Parameter.empty:
@@ -150,8 +160,11 @@ def _get_argument_type(
         return default_type
 
     # Parse docstring
-
-    type_from_docstring = docstrings.get_arg_type(module_method, arg.name)
+    type_from_docstring = None
+    try:
+        type_from_docstring = docstrings.get_arg_type(module_method, arg.name)
+    except CaikitRuntimeException:
+        log.warning("Failed to parse the docstring for %s", module_method)
     if type_from_docstring:
         if optional_arg:
             return Optional[type_from_docstring]
diff --git a/pyproject.toml b/pyproject.toml
index d4772751..382ebf30 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -27,7 +27,7 @@ dependencies = [
     "py-grpc-prometheus>=0.7.0,<0.8",
     "PyYAML>=6.0,<7.0",
     "requests>=2.26.0,<3.0",
-    "semver>=2.13.0,<4.0",
+    "semver>=2.13.0,<3.0",
     "six>=1.16.0,<2.0.0",
     "tqdm>=4.59.0,<5.0.0",
     "wheel>=0.37.0",


Test patch:
diff --git a/tests/runtime/service_generation/signature_parsing/test_docstrings.py b/tests/runtime/service_generation/signature_parsing/test_docstrings.py
index d4012c29..4cf81ad1 100644
--- a/tests/runtime/service_generation/signature_parsing/test_docstrings.py
+++ b/tests/runtime/service_generation/signature_parsing/test_docstrings.py
@@ -16,19 +16,30 @@
 Coverage is probably not the best
 """
 # Standard
-from typing import List
+from typing import List, Optional, Union
+from unittest.mock import patch
+
+# Third Party
+from docstring_parser import ParseError
+import pytest
 
 # Local
 from caikit.runtime.service_generation.signature_parsing.docstrings import (
     _extract_nested_type,
     _get_docstring_type,
+    get_arg_type,
+    get_return_type,
+    is_optional,
 )
+from caikit.runtime.types.caikit_runtime_exception import CaikitRuntimeException
 from sample_lib.data_model import SampleInputType
+import caikit
 import sample_lib
 
 
 def test_get_docstring_type():
     # TODO: fun edge case where producer word in description is found in types
+    # works on a sample_lib type
     assert (
         _get_docstring_type(
             candidate_type_names=["sample_lib.data_model.SampleOutputType"],
@@ -36,6 +47,36 @@ def test_get_docstring_type():
         == sample_lib.data_model.SampleOutputType
     )
 
+    # works on nested type
+    assert (
+        _get_docstring_type(
+            candidate_type_names=["List(int)"],
+        )
+        == List[int]
+    )
+
+    # works on a well known type
+    assert (
+        _get_docstring_type(
+            candidate_type_names=["ProducerId"],
+        )
+        == caikit.core.data_model.ProducerId
+    )
+
+    # returns a Union on valid types
+    assert (
+        _get_docstring_type(
+            candidate_type_names=[
+                "SampleInputType",
+                "sample_lib.data_model.SampleOutputType",
+            ],
+        )
+        == Union[
+            sample_lib.data_model.SampleOutputType,
+            sample_lib.data_model.SampleInputType,
+        ]
+    )
+
 
 def test_extract_nested_type():
     """
@@ -45,3 +86,85 @@ def test_extract_nested_type():
     assert _extract_nested_type("list(str)") == List[str]
     assert _extract_nested_type("List(SampleInputType)") == List[SampleInputType]
     assert _extract_nested_type("int") == None
+
+
+def test_is_optional_works_on_corner_cases_docstrings():
+    """
+    Test that is_optional works on different corner cases of docstrings for an optional field
+    """
+    # test docstring with multiple params of the same name still works, but
+    # evaluated whether "optional" on the first description
+    def _fn(self, some_input: Optional[int]):
+        """
+        Args:
+            some_input: yadda yadda blah int
+                optional int for input
+            some_input: I repeat myself
+
+        Returns:
+            None
+        """
+        pass
+
+    assert is_optional(_fn, "some_input") == True
+
+    # test if docstring_parser.parse throws an exception, we throw an exception
+    with patch("docstring_parser.parse", side_effect=ParseError("mocked error")):
+        with pytest.raises(CaikitRuntimeException) as e:
+            is_optional(_fn, "some_input")
+        assert "ParseError when parsing docstring for function: _fn" in e.value.message
+
+    def _fn(input):
+        pass
+
+    # test is_optional works on empty docstring
+    assert is_optional(_fn, "input") == False
+
+
+def test_get_arg_type_works_on_corner_cases_docstrings():
+    """
+    Test that get_arg_type works on different corner cases of docstrings
+    """
+    # test docstring with multiple params of the same name still works, but
+    # evaluated the type on the first description
+    def _fn(self, some_input: str):
+        """
+
+        Args:
+            some_input: yadda yadda blah int
+                int for input
+            some_input: I repeat myself but type str here
+                str for input
+
+        Returns:
+            None
+        """
+        pass
+
+    assert get_arg_type(_fn, "some_input") == int
+
+    # test if docstring_parser.parse throws an exception, we throw an exception
+    with patch("docstring_parser.parse", side_effect=ParseError("mocked error")):
+        with pytest.raises(CaikitRuntimeException) as e:
+            get_arg_type(_fn, "some_input")
+        assert "ParseError when parsing docstring for function: _fn" in e.value.message
+
+    # test get_arg_type works on empty docstring
+    def _fn_no_docstring(input):
+        pass
+
+    assert get_arg_type(_fn_no_docstring, "input") == None
+
+
+def test_get_return_type_corner_case_with_exception():
+    def _fn():
+        pass
+
+    # test get_return_type works on empty docstring
+    assert get_return_type(_fn) == None
+
+    # test if docstring_parser.parse throws an exception, we throw an exception
+    with patch("docstring_parser.parse", side_effect=ParseError("mocked error")):
+        with pytest.raises(CaikitRuntimeException) as e:
+            get_return_type(_fn)
+        assert "ParseError when parsing docstring for function: _fn" in e.value.message
diff --git a/tests/runtime/service_generation/signature_parsing/test_parsers.py b/tests/runtime/service_generation/signature_parsing/test_parsers.py
index c902cdbf..2ed518b9 100644
--- a/tests/runtime/service_generation/signature_parsing/test_parsers.py
+++ b/tests/runtime/service_generation/signature_parsing/test_parsers.py
@@ -17,9 +17,11 @@ Coverage is probably not the best
 """
 # Standard
 from typing import List, Optional
+from unittest.mock import patch
 import inspect
 
 # Third Party
+from docstring_parser import ParseError
 import pytest
 
 # Local
@@ -124,6 +126,17 @@ def test_get_output_type_name():
         == None
     )
 
+    # Test that if a ParseError was raised with docstring.parsers, we have no idea the type and return None
+    with patch("docstring_parser.parse", side_effect=ParseError("mocked error")):
+        assert (
+            get_output_type_name(
+                module_class=sample_lib.blocks.sample_task.InnerBlock,
+                fn_signature=empty_sign,
+                fn=sample_lib.blocks.sample_task.InnerBlock.run,
+            )
+            == None
+        )
+
 
 def test_get_argument_types_with_real_block():
     """Quick check that we get the right type for our sample block"""
@@ -134,6 +147,23 @@ def test_get_argument_types_with_real_block():
         == sample_lib.data_model.SampleInputType
     )
 
+    # Test that if a ParseError was raised with docstring.parsers, we could still parse from type annotation
+    with patch("docstring_parser.parse", side_effect=ParseError("mocked error")):
+        assert (
+            get_argument_types(sample_lib.blocks.sample_task.SampleBlock.run)[
+                "sample_input"
+            ]
+            == sample_lib.data_model.SampleInputType
+        )
+
+    # Test that if a ParseError was raised with docstring.parsers, but no type annotation provided, we cannot deduct type and return None
+    def run_fn(some_input: str, some_input_2):
+        pass
+
+    with patch("docstring_parser.parse", side_effect=ParseError("mocked error")):
+        assert get_argument_types(run_fn)["some_input"] == str
+        assert get_argument_types(run_fn)["some_input_2"] == None
+
 
 def test_optional_type_annotation():
     """Check that we keep the `Optional` wrapping on input types"""
diff --git a/tests/runtime/service_generation/test_create_service.py b/tests/runtime/service_generation/test_create_service.py
index 82195c44..52513e6c 100644
--- a/tests/runtime/service_generation/test_create_service.py
+++ b/tests/runtime/service_generation/test_create_service.py
@@ -29,6 +29,15 @@ untrainable_module_class = sample_lib.blocks.sample_task.SamplePrimitiveBlock
 ### create_inference_rpcs tests #################################################
 
 
+def test_create_inference_rpcs_for_module_with_no_run_function():
+    class Foo:
+        def __init__(self):
+            pass
+
+    rpcs = create_inference_rpcs([Foo])
+    assert len(rpcs) == 0
+
+
 def test_create_inference_rpcs():
     rpcs = create_inference_rpcs([widget_class])
     assert len(rpcs) == 1
@@ -78,6 +87,21 @@ def test_create_inference_rpcs_remove_non_primitive_modules():
 ### create_training_rpcs tests #################################################
 
 
+def test_create_inference_rpcs_for_module_with_no_train_function():
+    class Foo:
+        def __init__(self):
+            pass
+
+        def run(self):
+            pass
+
+        def train_in_progress(self):
+            pass
+
+    rpcs = create_inference_rpcs([Foo])
+    assert len(rpcs) == 0
+
+
 def test_create_training_rpcs():
     rpcs = create_training_rpcs([widget_class])
     assert len(rpcs) == 1