python circular imports once again (aka what's wrong with this design)
Asked Answered
A

5

48

Let's consider python (3.x) scripts:

main.py:

from test.team import team
from test.user import user

if __name__ == '__main__':
    u = user()
    t = team()
    u.setTeam(t)
    t.setLeader(u)

test/user.py:

from test.team import team

class user:
    def setTeam(self, t):
        if issubclass(t, team.__class__):
            self.team = t

test/team.py:

from test.user import user

class team:
    def setLeader(self, u):
        if issubclass(u, user.__class__):
            self.leader = u

Now, of course, i've got circular import and splendid ImportError.

So, not being pythonista, I have three questions. First of all:

i. How can I make this thing work ?

And, knowing that someone will inevitably say "Circular imports always indicate a design problem", the second question comes:

ii. Why is this design bad?

And the finally, third one:

iii. What would be better alternative?

To be precise, type checking as above is only an example, there is also a index layer based on class, which permits ie. find all users being members of one team (user class has many subclasses, so index is doubled, for users in general and for each specific subclass) or all teams having given user as a member

Edit:

I hope that more detailed example will clarify what i try to achieve. Files omitted for readibility (but having one 300kb source file scares me somehow, so please assume that every class is in different file)

# ENTITY

class Entity:
    _id    = None
    _defs  = {}
    _data  = None

    def __init__(self, **kwargs):
        self._id   = uuid.uuid4() # for example. or randint(). or x+1.
        self._data = {}.update(kwargs)

    def __settattr__(self, name, value):
        if name in self._defs:
            if issubclass(value.__class__, self._defs[name]):
                self._data[name] = value

                # more stuff goes here, specially indexing dependencies, so we can 
                # do Index(some_class, name_of_property, some.object) to find all   
                # objects of some_class or its children where
                # given property == some.object

            else:
                raise Exception('Some misleading message')
        else:
            self.__dict__[name] = value    

    def __gettattr__(self, name):
        return self._data[name]

# USERS 

class User(Entity):
    _defs  = {'team':Team}

class DPLUser(User):
    _defs  = {'team':DPLTeam}

class PythonUser(DPLUser)
    pass

class PerlUser(DPLUser)
    pass

class FunctionalUser(User):
    _defs  = {'team':FunctionalTeam}

class HaskellUser(FunctionalUser)
    pass

class ErlangUser(FunctionalUser)
    pass

# TEAMS

class Team(Entity):
    _defs  = {'leader':User}

class DPLTeam(Team):
    _defs  = {'leader':DPLUser}

class FunctionalTeam(Team):
    _defs  = {'leader':FunctionalUser}

and now some usage:

t1 = FunctionalTeam()
t2 = DLPTeam()
t3 = Team()

u1 = HaskellUser()
u2 = PythonUser()

t1.leader = u1 # ok
t2.leader = u2 # ok
t1.leader = u2 # not ok, exception
t3.leader = u2 # ok

# now , index

print(Index(FunctionalTeam, 'leader', u2)) # -> [t2]
print(Index(Team, 'leader', u2)) # -> [t2,t3]

So, it works great (implementation details ommitted, but there is nothing complicated) besides of this unholy circular import thing.

Anonymous answered 17/10, 2010 at 23:30 Comment(3)
It's considered good practice to capitalize your classes-- Team/User.Righteousness
Also: check out python's properties for the preferred alternative to declaring setters and getters.Asha
@intuited: i am loving decorators, but apparently it does not work great with setattr / getattr (example above is rather simplified)Anonymous
S
83

Circular imports are not inherently a bad thing. It's natural for the team code to rely on user whilst the user does something with team.

The worse practice here is from module import member. The team module is trying to get the user class at import-time, and the user module is trying to get the team class. But the team class doesn't exist yet because you're still at the first line of team.py when user.py is run.

Instead, import only modules. This results in clearer namespacing, makes later monkey-patching possible, and solves the import problem. Because you're only importing the module at import-time, you don't care than the class inside it isn't defined yet. By the time you get around to using the class, it will be.

So, test/users.py:

import test.teams

class User:
    def setTeam(self, t):
        if isinstance(t, test.teams.Team):
            self.team = t

test/teams.py:

import test.users

class Team:
    def setLeader(self, u):
        if isinstance(u, test.users.User):
            self.leader = u

from test import teams and then teams.Team is also OK, if you want to write test less. That's still importing a module, not a module member.

Also, if Team and User are relatively simple, put them in the same module. You don't need to follow the Java one-class-per-file idiom. The isinstance testing and set methods also scream unpythonic-Java-wart to me; depending on what you're doing you may very well be better off using a plain, non-type-checked @property.

Supportable answered 18/10, 2010 at 1:10 Comment(3)
Circular imports are inherently a bad thing. If you ever want to take a chunk of your application and, for e.g, make a library out of it, then the important thing about the dependencies of that chunk is that they must all go from the app to the library. A library which has dependencies on your app is no use to anyone. Not even you - you won't be able to run tests on the library without bundling it up with the app, which defeats the purpose of trying to decouple them in the first place. Since circular dependencies go BOTH ways, it is impossible to ever split up the code into decoupled chunks.Jervis
Let me repunctuate that for you: Circular imports are inherently a bad thing, if you ever want to take a chunk of your application and, for e.g, make a library out of it. Within a decoupled chunk, it may be perfectly reasonable to use circular references.Maillot
"Inherently" has a bad smell. It's like saying "obviously" or "surely." If the world has circular dependencies, and you use classes to model the world, then your classes may have circular dependencies. Yes, at the implementation level, they make things difficult and it is worthwhile discussing solutions.Wadleigh
R
3

i. To make it work, you can use a deferred import. One way would be to leave user.py alone and change team.py to:

class team:
    def setLeader(self, u):
        from test.user import user
        if issubclass(u, user.__class__):
            self.leader = u

iii. For an alternative, why not put the team and user classes in the same file?

Righteousness answered 17/10, 2010 at 23:35 Comment(2)
ad. iii - I have 60+ such classes and putting them into one file is not really the optionAnonymous
ad i. - isn't that hitting performance? anyone knows if python internally optimize that kind of imports?Anonymous
M
2

Bad practice/smelly are the following things:

  • Probaly unnecessary type checking (see also here). Just use the objects you get as it were a user/team and raise an exception (or in most cases, one is raised without needing additional code) when it breaks. Leave this away, and you circular imports go away (at least for now). As long as the objects you get behave like a user / a team, they could be anything. (Duck Typing)
  • lower case classes (this is more or less a matter of taste, but the general accepted standard (PEP 8) does it differently
  • setter where not necessary: you just could say: my_team.leader=user_b and user_b.team=my_team
  • problems with data consistency: what if (my_team.leader.team!=my_team)?
Maricela answered 18/10, 2010 at 0:37 Comment(6)
In reality getters and setters were much simplified.Normally they take more responsability, i.e. checking data consistency (i am not posting whole code here). Lower case classes - ok, I am just reading PEP8 ;). Last but not least - type checking. The problem is that objects which are set here, are propagated and their use is often deferred. So, if the object has wrong type i should backtrace all propagation which can be quite impossible. Instead of that I am validating object before (yes, violating EAFP), and validation is based on object's class rather than properties. Any sane solution here?Anonymous
You could just add a member to your instances saying what class it is: Then your test would look like : if not u.is_a == "user": which would then just be a sanity check using duck-typing.Abigael
@Michael: it looks that i have to deal with it in that way, although this doubles class inheritance structureAnonymous
#511472 has a slightly neater solution u.__class__.__name__ == "user"Abigael
@MichaelAnderson: I fail to see why if u.__class__.__name__ == "user" is any neater than if isinstance(u, user). the former looks quite "hackish", and fails for subclasses of user. If you want the class name as a string, fine, but for comparision I'd stich with isinstanceDelisle
@knitti: Downvoted. Your criticisms aren't specific to the question being asked. I don't feel that this is an appropriate forum for general criticism of posted code samples.Warm
W
0

Here is something I haven't seen yet. Is it a bad idea/design using sys.modules directly? After reading @bobince solution I thought I had understood the whole importing business but then I encountered a problem similar to a question which links to this one.

Here is another take on the solution:

# main.py
from test import team
from test import user

if __name__ == '__main__':
    u = user.User()
    t = team.Team()
    u.setTeam(t)
    t.setLeader(u)

# test/team.py
from test import user

class Team:
    def setLeader(self, u):
        if isinstance(u, user.User):
            self.leader = u

# test/user.py
import sys
team = sys.modules['test.team']

class User:
    def setTeam(self, t):
        if isinstance(t, team.Team):
            self.team = t

and the file test/__init__.py file being empty. The reason this works is because test.team is being imported first. The moment python is importing/reading a file it appends the module to sys.modules. When we import test/user.py the module test.team will already be defined since we are importing it in main.py.

I'm starting to like this idea for modules that grow quite large but there are functions and classes that depend on each other. Lets suppose that there is a file called util.py and this file contains many classes that depend on each other. Perhaps we could split the code among different files that depend on one another. How do we get around the circular import?

Well, in the util.py file we simply import all the objects from the other "private" files, I say private since those files are not meant to be accessed directly, instead we access them through the original file:

# mymodule/util.py
from mymodule.private_util1 import Class1
from mymodule.private_util2 import Class2
from mymodule.private_util3 import Class3

Then on each of the other files:

# mymodule/private_util1.py
import sys
util = sys.modules['mymodule.util']
class Class1(object):
    # code using other classes: util.Class2, util.Class3, etc

# mymodule/private_util2.py
import sys
util = sys.modules['mymodule.util']
class Class2(object):
    # code using other classes: util.Class1, util.Class3, etc

The sys.modules call will work as long as the mymodule.util is attempted to be imported first.

Lastly, I will only point out that this is being done to help users with readability (shorter files) and thus I would not say that circular imports are "inherently" bad. Everything could have been done in the same file but we are using this so that we can separate the code and not confused ourselves while scrolling through the huge file.

Wellthoughtof answered 13/3, 2014 at 19:37 Comment(0)
P
0

You could just fix the dependency graph; for example, the user may not have to know about the fact that it is a part of a team. Most circular dependencies admit of such a refactoring.

# team -> user instead of team <-> user
class Team:
    def __init__(self):
        self.users = set()
        self.leader = None

    def add_user(self, user):
        self.users.add(user)

    def get_leader(self):
        return self.leader

    def set_leader(self, user):
        assert user in self.users, 'leaders must be on the team!'
        self.leader = user

Circular dependencies significantly complicate refactoring, inhibit code reuse, and reduce isolation in testing.

Although in Python it is possible to circumvent an ImportError by importing at runtime, importing to the module level, or using other tricks mentioned here, these strategies do paper over a design flaw. It is worth avoiding circular imports if at all possible.

Phototherapy answered 25/3, 2019 at 5:24 Comment(0)

© 2022 - 2024 — McMap. All rights reserved.