WIP: fix circular imports

[SMoraisAnsys]

Currently, there are several files __init__.py triggering multiple imports. While this seems to be wise for most cases it happens that calling from ansys.edb.terminal import terminals raises an ImportError due to circular imports.

This PR is a WIP to see how this could be avoided. Current refactoring leverages:

• the removal of global imports;
• making Terminal and TerminalInstance inherit from ObjBase instead of ConnObj.
  To my knowledge, there seem to be no issue to change the inheritance since Terminal and TerminalInstance seem to only use methods inherited from ObjBase (i.e. self.msg, `self.is_null).

Current status : seems to lead to something "working" as I'm able to pass the test marked as current (try it with pytest -m current), the other tests are disabled atm as the refactoring is still in progress.

Should close #312

@hiro727 Do not hesitate to propose another approach to tackle this problem or if I'm wrong on something.

[hiro727]

the main idea behind including all classes into __init__.py was to have a flat file structure that avoids path complexities for end users.

from ansys.edb.definition import BondwireDef

instead of

from ansys.edb.definition.bondwire_def import BondwireDef

This loosely resembles the legacy ironpython API structure we had to ease the transition into this API instead of making dramatic changes from first version

[hiro727]

how do you think about keeping these untouched and instead remove all imports of classes inside individual files?

Below is a simple example, (the only class definition that must be initialized on first load is ObjBase since it's a base class.)

# component_def.py

# definitions (directly import enums, external packages, or a base class with no dependencies)

from ansys.api.edb.v1.component_def_pb2_grpc import ComponentDefServiceStub
from ansys.edb.edb_defs import DefinitionObjType
from ansys.edb.session import StubAccessor, StubType
from ansys.edb.core import ObjBase # base class must be loaded

# semi-lazy dependencies

import ansys.edb.core
import ansys.edb.definition
import ansys.edb.layout

# class

class ComponentDef(ObjBase):
  @property
    def footprint(self):
        return ansys.edb.layout.Cell(self.__stub.GetFootprintCell(self.msg))

with something like

# layout.py
import ansys.edb.definition

class Layout:
  @property
  def component_defs(self):
    return [ansys.edb.definition.ComponentDef(cd) for cd in self.__stub.GetComponentDefs(...)]

no classes are imported from any other classes so it will not cause circular dependency this way. thoughts?

[SMoraisAnsys]

I understand why you followed the current approach and I agree with the idea of simplifying everything as much as possible for end users. However, the current circular dependencies are probably a side effect of this approach. I feel like keeping it will make the package harder to maintain.

Keeping imports of classes inside individual files (i.e. import classes and functions in the file they are needed) makes it easier to see which classes or functions are being used in that file and should help to avoid circular dependencies.

how do you think about keeping these untouched and instead remove all imports of classes inside individual files?

I don’t think that this is a good practice. This would lead to:

• a loss of context / code complexity as we can’t easily understand which classes and functions are being used in a file and we would need to constantly refer to the central files;
• a decrease in modularity and could make any refactoring becoming a headache;

Another side effect would be that your users would have to follow the same principles (which probable won't be the case).

Any other idea on how to solve the problem ? Some non-trivial refactoring may be required :/

[drewm102]

@SMoraisAnsys I believe your approach of importing classes/functions in individual files as they are needed is the best approach. As Hiroki mentioned, this API is effectively a cpython port of the existing EDB API in AEDT which is a IronPython based API. Unfortunately, this means that it came with the baggage of some of the API reflecting the IronPython models for hierarchy and importing packages which, at least in our use case, didn't translate well to cpython (thus all the circular imports).

One way we have worked around these circular imports is to do a delayed import of modules that cause circular dependencies. Most of the circular imports are due to classes having methods that return objects of a type whose import results in a circular import. For example, the method SimulationSetup.cast() requires the HfssSimulationSetup class be imported but the HfssSimulationSetup class derives from the SimulationSetup class, resulting in a potential circular import. By importing the HfssSimulationSetup class in the SimulationSetup.cast() method rather than the top of the simulation_setup module we were able to avoid the circular import. This is certainly a hacky solution that isn't ideal, but it would get the job done for this release. I think we will need to look into giving the API a more pythonic structure in future releases so that we can avoid these sorts of circular dependencies altogether.

So perhaps for this release, the best solution is to import classes/functions as needed in each file like you suggested and circumvent any resulting circular imports using delayed imports. What are your thoughts? Thanks!

@hiro727

[SMoraisAnsys]

@drewm102 In general, I would recommend to always place import statements at the top of the file but there are specific cases where it makes sense to move some imports as you suggest. Here seems to be one of such cases.

I think that your proposition of importing classes/functions as needed in each file and overcoming any resulting circular imports using delayed imports might be a short-term solution 👍 However, make sure to deal with this later on and make sure to document your reasons for moving imports to help future maintainability. I insist on this point because, although it solves the problem of circular dependencies, it also comes with a :

• reduction of code readability which can make the code more difficult to read;
• drop in performance as the import statement might be executed multiple times if the function is called repeatedly.

[hiro727]

these actually need to be a subclass of ConnObj as users need to be able to access those methods defined in ConnObj through Terminal/TerminalInstance

[SMoraisAnsys]

In that case, another change should be proposed.

[SMoraisAnsys]

I'm closing this issue following my last comment #313 (comment)