Repair model attribute typing and docstrings

[davidorme]

With the change to the static model mechanism (#373), model attributes are now largely being defined in the _setup method, which means that the actual model class documentation is missing the model attributes: they are only picked up from __init__ or from class attributes.

We need to fix this - our docs are broken until we do. From @dalonsoa's comments on Teams, we have two clean choices.

1. We start using __init__ again in the models. It's banned at the moment - only the BaseModel.__init__ gets run and it can't be redefined. This is because __init__ shouldn't do anything but setup the static/non_static model mode and all actual stuff that needs to happen is isolated in _setup, so it can conditionally executed based on the model mode. But we can just agree that actual models __init__ methods should be "logic-free" and are used simply to maintain the documentation and typing of model attributes. They have to call super().__init__().

2. We document and type all of the model attributes as if they were class attributes (below the class ModelName line and outside of any method). We then don't have to use __init__.

I prefer option (1). Although it is a bit more verbose, it follows the canonical Python practice of declaring instance attributes in __init__. These are attributes that differ between instances, which is true for all of these attributes. In option 2, we are pretending they are class attributes, which should be things that are fixed across all class instances. The only reason we can do this is because we don't actually want to assign values to the attributes at a class level, only at an instance level.

The code below shows the two options in a basic setup. The issue we are trying to fix is that attribute eee defined in _setup is not documented in sphinx (and I don't think can be), and nearly all our model attributes are now in this state.

from abc import ABC


class Base(ABC):
    """Base class docstring."""

    base_class_attr: int
    "A class attribute"

    def __init__(self) -> None:
        """Base init docstring."""
        self.aaa: int
        "An integer"
        self.bbb: str
        "A string"


class Actual(Base):
    """Actual class docstring."""

    def __init__(self) -> None:
        """Actual init docstring."""
        super().__init__()

        self.ccc: int
        "Another integer"

        self.ddd: str
        "Another string"

    def _setup(self) -> None:
        """Actual setup docstring."""

        self.eee: int
        "This attribute is not documented"


class Base2(ABC):
    """Base2 class docstring."""

    base_class_attr: int
    "A class attribute"

    def __init__(self) -> None:
        """Base2 init docstring."""
        self.aaa: int
        "An integer"
        self.bbb: str
        "A string"


class Actual2(Base):
    """Actual2 class docstring."""

    ccc: int
    "Another integer"

    ddd: str
    "Another string"

    def _setup(self) -> None:
        """Actual2 setup docstring."""

        self.eee: int
        "This attribute is not documented"

This can be built in an API doc using:

.. automodule:: virtual_ecosystem.test
    :autosummary:
    :members:
    :inherited-members: # needed to get inherited members in autosummary
    :special-members: __init__
    :private-members:

[jacobcook1995]

The first one seems like the least likely to get confused to me, as _setup will have other things going on in it.

We could write something in the docstring like "we are only using init to define attributes, all attributes should be defined here so that they get documented" (or similar) to make it clear our purpose in using __init__ like this

[davidorme]

The first one seems like the least likely to get confused to me, as _setup will have other things going on in it.

The behaviour of _setup will be identical in both cases: do the stuff to populate the attributes that have been defined. The only real difference is a semantic one about where those definitions go. I guess (1) has the advantage that if sphinx does at any point figure out how to document class and instance attributes separately - which it should but apparently can't - then we won't have to change it all.

See sphinx-doc/sphinx#3141

[jacobcook1995]

Oh, I somehow managed to confuse myself despite reading the example code and thought that the definitions would go within _setup 🫠

I don't see much difference between the options honestly so if you think the first option is better future proofed against improvements to sphinx then I think that's probably the way to go

[davidorme]

I should have pointed out that there are two pairs of Base model + instance of Base Model, so we're choosing between Actual and Actual2.

The rest is mostly just me checking that that the attribute documentation can be made to inherit from the base class, which it can.

[davidorme]

Oh for god's sake. The two Base models are of course identical. 🫠

[davidorme]

Of relevance here - with the move to ruff, we lost a few rules, and specifically of interest here is the one that checks for attributes defined outside of __init__. That's not implemented yet:

astral-sh/ruff#970

It might be worth running pylint manually here to help catch definitions.

[sallymatson]

Of relevance here - with the move to ruff, we lost a few rules, and specifically of interest here is the one that checks for attributes defined outside of __init__. That's not implemented yet:

astral-sh/ruff#970

It might be worth running pylint manually here to help catch definitions.

Weirdly pylint isn't picking up attribute-defined-outside-init when the attributes are defined in _setup, but if I change it to a public function (literally just remove the _ so it is setup()) then it does. Not sure why this is but at least I can use pylint to confirm using that hacky method !!

[sallymatson]

Just to check, does this new change mean that this exception is no longer necessary:

virtual_ecosystem/virtual_ecosystem/core/base_model.py

Lines 597 to 598 in 517b0a3

	if cls.__init__ != BaseModel.__init__:
	raise NotImplementedError("Model subclasses cannot override __init__.")

[davidorme]

Just to check, does this new change mean that this exception is no longer necessary:

virtual_ecosystem/virtual_ecosystem/core/base_model.py

Lines 597 to 598 in 517b0a3

if cls.init != BaseModel.init:
raise NotImplementedError("Model subclasses cannot override init.")

Thats correct - in fact, we have to remove it or it won't work.