Avoiding circular imports for the 100th time

Question

Summary

I keep on having an ImportError in a complex project. I've distilled it to the bare minimum that still gives the error.

Example

A wizard has containers with green and brown potions. These can be added together, resulting in new potions that are also either green or brown.

We have a Potion ABC, which gets its __add__, __neg__ and __mul__ from the PotionArithmatic mixin. Potion has 2 subclasses: GreenPotion and BrownPotion.

In one file, it looks like this:

onefile.py:

from abc import ABC, abstractmethod

def add_potion_instances(potion1, potion2): # some 'outsourced' arithmatic
    return BrownPotion(potion1.volume + potion2.volume)

class PotionArithmatic:
    def __add__(self, other):
        # Adding potions always returns a brown potion.
        if isinstance(other, base.Potion):
            return add_potion_instances(self, other)
        return BrownPotion(self.volume + other)

    def __mul__(self, other):
        # Multiplying a potion with a number scales it.
        if isinstance(other, Potion):
            raise TypeError("Cannot multiply Potions")
        return self.__class__(self.volume * other)

    def __neg__(self):
        # Negating a potion changes its color but not its volume.
        if isinstance(self, GreenPotion):
            return BrownPotion(self.volume)
        else:  # isinstance(self, BrownPotion):
            return GreenPotion(self.volume)

    # (... and many more)


class Potion(ABC, PotionArithmatic):
    def __init__(self, volume: float):
        self.volume = volume

    __repr__ = lambda self: f"{self.__class__.__name__} with volume of {self.volume} l."

    @property
    @abstractmethod
    def color(self) -> str:
        ...


class GreenPotion(Potion):
    color = "green"


class BrownPotion(Potion):
    color = "brown"


if __name__ == "__main__":

    b1 = GreenPotion(5)
    b2 = BrownPotion(111)

    b3 = b1 + b2
    assert b3.volume == 116
    assert type(b3) is BrownPotion

    b4 = b1 * 3
    assert b4.volume == 15
    assert type(b4) is GreenPotion

    b5 = b2 * 3
    assert b5.volume == 333
    assert type(b5) is BrownPotion

    b6 = -b1
    assert b6.volume == 5
    assert type(b6) is BrownPotion

This works.

Split into files into importable module

Each part is put in its own file inside the folder potions, like so:

usage.py
potions
| arithmatic.py
| base.py
| green.py
| brown.py
| __init__.py

potions/arithmatic.py:

from . import base, brown, green

def add_potion_instances(potion1, potion2):
    return brown.BrownPotion(potion1.volume + potion2.volume)

class PotionArithmatic:
    def __add__(self, other):
        # Adding potions always returns a brown potion.
        if isinstance(other, base.Potion):
            return add_potion_instances(self, other)
        return brown.BrownPotion(self.volume + other)

    def __mul__(self, other):
        # Multiplying a potion with a number scales it.
        if isinstance(other, base.Potion):
            raise TypeError("Cannot multiply Potions")
        return self.__class__(self.volume * other)

    def __neg__(self):
        # Negating a potion changes its color but not its volume.
        if isinstance(self, green.GreenPotion):
            return brown.BrownPotion(self.volume)
        else:  # isinstance(self, BrownPotion):
            return green.GreenPotion(self.volume)

potions/base.py:

from abc import ABC, abstractmethod
from .arithmatic import PotionArithmatic

class Potion(ABC, PotionArithmatic):
    def __init__(self, volume: float):
        self.volume = volume

    __repr__ = lambda self: f"{self.__class__.__name__} with volume of {self.volume} l."

    @property
    @abstractmethod
    def color(self) -> str:
        ...

potions/green.py:

from .base import Potion

class GreenPotion(Potion):
    color = "green"

potions/brown.py:

from .base import Potion

class BrownPotion(Potion):
    color = "brown"

potions/__init__.py:

from .base import Potion
from .brown import GreenPotion
from .brown import BrownPotion

usage.py:

from potions import GreenPotion, BrownPotion

b1 = GreenPotion(5)
b2 = BrownPotion(111)

b3 = b1 + b2
assert b3.volume == 116
assert type(b3) is BrownPotion

b4 = b1 * 3
assert b4.volume == 15
assert type(b4) is GreenPotion

b5 = b2 * 3
assert b5.volume == 333
assert type(b5) is BrownPotion

b6 = -b1
assert b6.volume == 5
assert type(b6) is BrownPotion

Running usage.py gives the following ImportError:

ImportError                               Traceback (most recent call last)
usage.py in <module>
----> 1 from potions import GreenPotion, BrownPotion
      2 
      3 b1 = GreenPotion(5)
      4 b2 = BrownPotion(111)
      5 

potions\__init__.py in <module>
----> 1 from .green import GreenPotion
      2 from .brown import BrownPotion

potions\brown.py in <module>
----> 1 from .base import Potion
      2 
      3 class GreenPotion(Potion):
      4     color = "green"

potions\base.py in <module>
      1 from abc import ABC, abstractmethod
      2 
----> 3 from .arithmatic import PotionArithmatic
      4  

potions\arithmatic.py in <module>
----> 1 from . import base, brown, green
      2 
      3 class PotionArithmatic:
      4     def __add__(self, other):

potions\green.py in <module>
----> 1 from .base import Potion
      2 
      3 class GreenPotion(Potion):
      4     color = "green"

ImportError: cannot import name 'Potion' from partially initialized module 'potions.base' (most likely due to a circular import) (potions\base.py)

Further analysis

• Because Potion is a subclass of the mixin PotionArithmatic, the import of PotionArithmatic in base.py cannot be changed.
• Because GreenPotion and BrownPotion are subclasses of Potion, the import of Potion in green.py and brown.py cannot be changed.
• That leaves the imports in arithmatic.py. This is where the change must be made.

Possible solutions

I've looked for hours and hours into this type of problem.

• The usual solution is to not import the classes Potion, GreenPotion, and BrownPotion into the file arithmatic.py, but rather import the files in their entirety, and access the classes with base.Potion, green.GreenPotion, brown.BrownPotion. This I have already done in the code above, and does not solve my problem.

• A possible solution is to move the imports into the functions that need them, like so:

arithmatic.py:

def add_potion_instances(potion1, potion2):
    from . import base, brown, green # <-- added imports here
    return brown.BrownPotion(potion1.volume + potion2.volume)

class PotionArithmatic:
    def __add__(self, other):
        from . import base, brown, green # <-- added imports here
        # Adding potions always returns a brown potion.
        if isinstance(other, base.Potion):
            return add_potion_instances(self, other)
        return brown.BrownPotion(self.volume + other)

    def __mul__(self, other):
        from . import base, brown, green # <-- added imports here
        # Multiplying a potion with a number scales it.
        if isinstance(other, base.Potion):
            raise TypeError("Cannot multiply Potions")
        return self.__class__(self.volume * other)

    def __neg__(self):
        from . import base, brown, green # <-- added imports here
        # Negating a potion changes its color but not its volume.
        if isinstance(self, green.GreenPotion):
            return brown.BrownPotion(self.volume)
        else:  # isinstance(self, BrownPotion):
            return green.GreenPotion(self.volume)

Though this works, you can imagine this results in many additional lines if the file contains many more methods for the mixin class, esp. if these in turn call functions on the module's top level.

• Any other solution...? That actually works and is not completely cumbersome as the duplicated imports in the code block above?

Many thanks!

Conceited

There's no way you can make this into a minimal reproducible example? This question is absolutely huge and I doubt anybody wants to take the 10+ minutes just to read through it, let alone try to help out. Just saying. – Caulicle

Agree with above guy that I don't really have the time to look through this all. If you don't want to mess with things too much, a lazy importer might help github.com/tensorflow/agents/blob/v0.11.0/tf_agents/utils/…. – Robomb

@RandomDavis I agree it's huge, but I honestly cannot reduce it further without the problem disappearing or the solution becoming trivial. There have to be >1 subclases, in separate files. The mixin file needs at least a few methods to illustrate its use and the issue with putting import statements inside every function it has. And then I think it's useful to show what I've tried and why it's not working. But let me know if you have any suggestions where I can shorten it and I'll gladly do it. – Conceited

The easiest solution honestly is to just have these in a single file, When you split modules apart you generally want a directed dependency graph, but the way you've designed this everything is inextricably enmeshed -- you can't have a BrownPotion that doesn't implement PotionArithmetic, and you can't implement PotionArithmetic without knowing about BrownPotion. – Weisbart

Thanks for that comment @Samwise. I'm aware of the dependency mesh/mess, and it is exactly why I put the question here. I'd put everything in 1 file if that was feasible, but in my actual use case, there are currently 6 mixins, and each is 500-700 lines long as it is. – Conceited

If your codebase is this tightly coupled with 3,000LOC just for the mixins, the long term answer might be rewriting. I know you don't want to hear that.... or lazy importing/dependency injecting as @AndrewHolmgren suggests. Incidentally, whilst massive files aren't exactly great, 3,000 lines of text ought to be manageable in any decent modern editor. And you can definitely edit it in vim! – Indecisive

The limitation is not the text editor, but rather my own sanity :) I'm always up for refactoring though - if you have any suggestions how to reduce coupling... I'm all ears @2e0byo. Thanks :) – Conceited

Answer 1

TLDR: Rule of thumb

you should not use on the mixin/inheritance architecture if the mixin returns an instance of the class (or of one of its descendants). In that case, the methods should be appended to the class object itself.

Details: Solutions

I thought of 2 (very similar) ways to get it to work. None is ideal, but they both seem to resolve the problem, by no longer relying on inheritance for the mixin.

In both, the potions/base.py file is changed to the following:

potions/base.py:

from abc import ABC, abstractmethod

class Potion(ABC): # <-- mixin is gone
    # (nothing changed here)

from . import arithmatic  # <-- moved to the end
arithmatic.append_methods()  # <-- explicitly 'do the thing'

What we do with potions/arithmatic.py depends on the solution.

Keep the mixin class, but append the methods manually

This solution I like the best. In arithmatic.py, we can keep the original PotionArithmatic class. We just add a list of relevant dunder methods it, and the append_methods() function to do the appending.

potions/arithmatic.py:

from . import base, brown, green

def add_potion_instances(potion1, potion2):
    # (nothing changed here)

def PotionArithmatic:
    ATTRIBUTES = ["__add__", "__mul__", "__neg__"] # <-- this is new
    # (nothing else changed here)

def append_methods(): # <-- this is new as well
    for attr in PotionArithmatic.ATTRIBUTES:
        setattr(base.Potion, attr, getattr(PotionArithmatic, attr))

Completely get rid of the mixin

Alternatively, we can get rid of the PotionArithmatic class alltogether, and just append the methods directly to the Potion class object:

potions/arithmatic.py:

from . import base, brown, green

def _add_potion_instances(potion1, potion2):
    return brown.BrownPotion(potion1.volume + potion2.volume)

def _ext_add(self, other):
    # Adding potions always returns a brown potion.
    if isinstance(other, base.Potion):
        return _add_potion_instances(self, other)
    return brown.BrownPotion(self.volume + other)

def _ext_mul(self, other):
    # Multiplying a potion with a number scales it.
    if isinstance(other, base.Potion):
        raise TypeError("Cannot multiply Potions")
    return self.__class__(self.volume * other)

def _ext_neg(self):
    # Negating a potion changes its color but not its volume.
    if isinstance(self, green.GreenPotion):
        return brown.BrownPotion(self.volume)
    else:  # isinstance(self, BrownPotion):
        return green.GreenPotion(self.volume)

def append_methods():
    base.Potion.__add__ = _ext_add
    base.Potion.__mul__ = _ext_mul
    base.Potion.__neg__ = _ext_neg

Consequences

Both solutions work, but be aware that

(a) they introduce more coupling and necessitate moving imports to the end of base.py, and

(b) the IDE will no longer know about these methods when writing code, as they are added at run-time.

Conceited

My idea was to not import base, green or brown into PotionArithmatic, breaking the cycle. If there is nothing else going on in the class, then it does become quite redundant - so I would say that my idea is very similar to yours. Realising that you can just overwrite the __add__ function is more elegant than what I suggested, so that is a definite improvement. – Gomes

I suggest you put the base.Potion.__add__ = ext_add commands into a function and call that function after the from . import arithmetic - then you make the construction more explicit. – Gomes

@vorrade - good idea, I just implemented it – Conceited

Answer 2

You somehow have to break the circle of the class dependencies. I haven't tried it out, but I think the following strategy might work. The idea is to construct the class PotionArithmatic first with no dependencies. Then you can inject the methods after the class has been fully constructed. But it is perhaps just as cumbersome as your solution:

class PotionArithmatic:
    external_add = None
    external_mul = None
    external_neg = None
    def __add__(self, other):
        return PotionArithmatic.external_add(self,other)

    def __mul__(self, other):
        return PotionArithmatic.external_mul(self,other)

    def __neg__(self):
        return PotionArithmatic.external_neg(self)

In an external file you would need:

def external_add(a,b):
    pass # put your code here

def external_mul(a,b):
    pass # put your code here

def external_neg(a):
    pass # put your code here

PotionArithmatic.external_add = external_add
PotionArithmatic.external_mul = external_mul
PotionArithmatic.external_neg = external_neg

Gomes

The three lines external_xyz = None are not required, but perhaps help to document the procedure... – Gomes

Thanks @vorrade for your answer, but I'm not really following. How do the imports get into this file? Do I understand correctly, that I put the from . import base, green, brown lines inside the external_mul(a, b) function definitions? If yes, I might just as well put all the code directly inside the __add__(self, other) - or am I missing something? – Conceited

Answer 3

(Take 2) Could you do the imports in your Mixin class' __init__, save them to properties, and then reference them from your methods? I think that would be cleaner than importing things in every method/function.

./test.py

import potion

p1 = potion.Sub1()
p1.foo()

./potion/__init__.py

from .sub1 import Sub1
from .sub2 import Sub2

./potion/mixin.py

def bar(p):
    return isinstance(p, p.sub1.Sub1) or isinstance(p, p.sub2.Sub2)

class Mixin:
    def __init__(self):
        from . import sub1
        from . import sub2
        self.sub1 = sub1
        self.sub2 = sub2    
    
    def foo(self):
        return bar(self)

    def baz(self):
        return self.sub1.Sub1(), self.sub2.Sub2()

./potion/sub1.py

from .base import Base

class Sub1(Base):
    pass

./potion/sub2.py

from .base import Base

class Sub2(Base):
    pass

./potion/base.py

from .mixin import Mixin

class Base(Mixin):
    pass

Mismatch

Thanks for your answer, @ChrisSears. I had also noticed that putting the subclasses in a single file fixes the problem - though I'm unsure exactly why. Either way, it's again not a real possibility for me, as in my use case there are several subclasses and they are quite long; it's really cumbersome to have them in a single file. – Conceited