Skip to content

Avoid importing nx directly in submodules#13

Closed
stefanv wants to merge 1 commit intodschult:doc_classicfrom
stefanv:doc_classic
Closed

Avoid importing nx directly in submodules#13
stefanv wants to merge 1 commit intodschult:doc_classicfrom
stefanv:doc_classic

Conversation

@stefanv
Copy link
Copy Markdown

@stefanv stefanv commented Dec 6, 2020

This is how I would restructure imports to make use of relative imports. Feel free to use or not use as you see fit.

@dschult
Copy link
Copy Markdown
Owner

dschult commented Dec 6, 2020

Interesting... Thanks for this.
I learned two things from it so far:

  • one reason for using relative imports is to avoid loading "nx" into the namespace of the submodule. Though I'm still not quite sure why that's a bad thing.
  • if we go this route, we had better come up with better names for things like exception and utils so that a newbee reading the code will know that they are networkx's exceptions and utils without having to go up to the top of the module and learn our file structure.

One of the difficulties I've had learning about relative imports is that the code is no longer "stand alone". It changes depending on where in your package it is located. It means I have to learn a lot more about NetworkX than just one module before I feel comfortable contributing.

There must be an advantage to having code depend on the module's location in the package. I understand that relative imports makes it easier to change the directory structure of the package. But 1) how often is that done? and 2) does it really save that much? It's an easy grep and replace to change the name generators to builders. There must be more advantage than this.

@jarrodmillman
Copy link
Copy Markdown

I also don't understand the advantages of relative imports. Here is my current thinking (which could possibly be confused or wrong):

Relative imports are succinct

I don't have a problem with the longer versions. Our directory structure is not that deep.

I think this is one of our longest imports

from networkx.algorithms.isomorphism.temporalisomorphvf2 import ...

It is pretty long, but we don't use it much.

This seems like a small benefit for projects with deeper hierarchies.

Relative imports makes changing things harder

If, for example, we move algorithms/community/centrality.py to algorithms/centrality.py,
then we will need to changes lines from

from ..community import centrality
from .community import centrality
from . import centrality

to

from .. import centrality
from . import centrality
from .. import centrality

But we would need to be careful not to change

from . import centrality

in functions inside of algorithms/bipartite/.

And we would also need to change all the imports inside of algorithms/centrality.py

With absolute imports, we would just need to do one global search for

from networkx.algorithms.community import centrality

and replace them with

from networkx.algorithms import centrality

This seems like a pain that would likely lead to errors.

Relative imports seem less easy to read

It requires paying more attention to the directory structure and remembering where you are in it. With absolute imports I always use the same import no matter which file I am editing.

This seems like a negative.

@stefanv
Copy link
Copy Markdown
Author

stefanv commented Dec 6, 2020

I think the absolute imports are fine, as long as you specify the whole path to modules. These are different syntaxes for the same thing.

What should be avoided is doing: import networkx as nx and then using nx.whatever.

@jarrodmillman
Copy link
Copy Markdown

I am also unclear on the advantages / disadvantages of

 import networkx as nx 

vs

 import networkx

inside the library. Although, I prefer (though I don't know exactly why)

 import networkx

More generally, my current (slight) preference would be to always do something like

from networkx.generators.geometric import random_geometric_graph

vs

import networkx as nx

nx.random_geometric_graph

or

import networkx 

networkx.random_geometric_graph

or

from networkx.generators import random_geometric_graph

The only exceptions being things like

from networkx.algorithms import bipartite
from networkx.algorithms import community

bipartite.centrality
community.centrality

if we need to (for example) use both centralities in the same module.

But I don't have a strong sense for why other than it seems more explicit (and less likely to cause confusion) and more succinct.

I am not as concerned about having to read the imports at the top of the file to see where things are coming from. So I don't think we need to do

import networkx

networkx.algorithms.bipartite.centrality
networkx.algorithms.community.centrality

But I am not strongly opposed to this.

@jarrodmillman
Copy link
Copy Markdown

jarrodmillman commented Dec 6, 2020

Regardless of what we decide to do about the imports, we still need to figure out how to stop our circular imports.

Currently

import networkx

gives us

networkx
networkx.networkx
networkx.nx
...
networkx.nx.networkx.nx
...

And

import networkx as nx

gives us

nx
nx.networkx
nx.nx
...
nx.nx.networkx.nx
...

@dschult
Copy link
Copy Markdown
Owner

dschult commented Dec 6, 2020

Now this will probably mess up the discussion completely, but:

why are circular imports a problem? (they fill up namespace dict, but only with 1 extra item.. right? They don't get used. Any introspection code should be able to stop an infinite loop, right?)

why should we avoid using nx.whatever inside the package? (it makes it clear where it comes from)

Are these style concerns (which I can definitely understand) or is there a deeper rationale? I can probably get used to relative imports even just for style. But I'm more interested in understanding the advantages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants