Test failures if Sphinx is not installed

[hiker]

If sphinx is not installed, I get several test failures:

FAILED docstring_parser_test.py::test_create_docstring_data - assert "<class 'int'>" == 'int'
FAILED psyGen_test.py::test_transformation_get_valid_options - assert "<class 'bool'>" == 'bool'

10 test failures all in all (some are slightly different, but all seem to involve some type). At least two cases have been confirmed to be caused by missing sphinx, e.g. in psyGen.py:

try:
    from sphinx.util.typing import stringify_annotation
except ImportError:
    # No Sphinx available so use our own, simpler version.
    from psyclone.utils import stringify_annotation

So, without Sphinx, stringify_annotation is different, and the output of stringify_annotation is just different when sphinx is not available.

IMHO, the best approach might be in all tests to disable import of sphinx during testing, not sure if this is possible. So, we might adjust tests to be:

    assert options['valid2'].typename in ["int", "<class 'int'>"]

Of course, we might also make sphinx a requirement? It seems to be reasonable big (all sphinx* package on my system is >30MB))
Or we could try to make our stringify_annotation to be more similar to Sphinx's :)

[sergisiso]

I was tempted to say this is not an issue since pip install -e .[test] must install Sphinx. But then, it is odd that what we are testing is different than what happens in production which doesn't have Sphinx. I would still prefer to remove Sphinx as a dependency and improve our stringify

[LonelyCat124]

I'd love to just use a standalone stringify_annotation instead of relying Sphinx, but as soon as you move out of very simple types (e.g. 'int') its very complex. I had also thought that moving to newer pythons (3.10+) also gives output closer to sphinx's but I'm not certain.

Its also a bit awkward because stringify_annotation is only used (in production) for the docstring generation, so it relying on Sphinx also makes sense (since we use sphinx for our docs).

Edit: We could maybe change our implementation to return str(annotation.__name__) but I'm not sure what that does more generally, but at least with 3.12 this gives int.

[LonelyCat124]

Could also adapt something like this:

def obj2dict(obj0, ignore_start='__', max_recursion=8):
    curr_type = str(type(obj0))
    if curr_type.startswith("<class '"):
        curr_type = curr_type.split("'")[-2]
    if curr_type in ['NoneType', 'float', 'int', 'str', 'bytes', 'bool']:
        return obj0
    if curr_type.startswith('np.int') or curr_type.startswith('numpy.int'):
        return int(obj0)
    if curr_type.startswith('np.float') or curr_type.startswith('numpy.float'):
        return float(obj0)
    if curr_type == 'np.ndarray' or curr_type == 'numpy.ndarray':
        curr_type, obj0 = 'list', obj0.tolist()
    if curr_type == 'list':
        return [obj2dict(k, ignore_start, max_recursion-1) for k in obj0]
    if curr_type == 'dict':
        return {obj2dict(k, ignore_start, max_recursion-1): obj2dict(v, ignore_start, max_recursion-1) for k,v in obj0}
    if max_recursion <= 0:
        return None
    ret = {}
    attr = dir(obj0)
    for a in attr:
        if a.startswith('_'):
            continue
        ret[a] = obj2dict(getattr(obj0,a), ignore_start, max_recursion-1)
    return ret

from https://stackoverflow.com/questions/5008828/convert-a-python-type-object-to-a-string

[sergisiso]

So there are 2 distinct situations:

• Generating docs, it requires pip install -e .[doc] so it has Sphinx anyway, and its stringify_annotation produces nicer types.
• Generating error messages, in production there may not be Shpinx. Maybe the stringify_annotation is not that important because it says it is a TypeError anyway. Could we make the get_options validation produce the str(annotation.name) and keep the stringify_annotation only for the "generating docs" case? Then whatever is in the tests should take the path without Sphinx.

[LonelyCat124]

So I'd missed the 2nd case, and I'd like to maybe improve those messages, but we'd need to check the output with 3.9 as I'm not sure I tested with 3.9 and annotation.__name__. I know 3.8 and before didn't give the same as 3.10+ but I don't remember where 3.9 was.

I could do something recursive and neat, and also try to improve the type checking for Generics at 2888 (which is more difficult), but I don't think that could be prioritised right now.

[sergisiso]

Ok, but that a python type string is different depending of the python version is not a problem specific to get_options, it is everywhere: grep -r "\.__name__" src/

And tbh, I don't think its worth fixing, it is whatever that interpreter version wants to print as type for the TypeError

[hiker]

I think it should be fix. If a non-dev wants to check/train/do-something-with PSyclone, one of the first things would be to run the tests to see that the installation works.

What about simply changing the tests to:

assert "int" in options['valid2'].typename 

(or maybe a regex \bintb`? That would be a trivial change, would be independent of python versions, and both stringifys.

[LonelyCat124]

We could also pull stringify_annotations out of sphinx and either add it into PSyclone or a new (small) dependency. The only issue is we'd need to make sure we update it periodically with new versions of Python/sphinx (or automate that somehow).