Skip to content
This repository has been archived by the owner on Apr 23, 2021. It is now read-only.

Problem: Cyclic import if loading extensions prior to simphony itself #291

Merged
merged 6 commits into from
May 2, 2016

Conversation

mehdisadeghi
Copy link
Contributor

Solution: re-organize packages and fix #290.

@mehdisadeghi
Copy link
Contributor Author

I realized if we import the extension package prior to the simphony itself a cyclic import will cause a failure in loading engine metadata information. I moved dependencies to ext package.

@kitchoi
Copy link
Contributor

kitchoi commented Apr 28, 2016

Another suggestion I would make would be to put ext under the engine directory, and instead of calling ext, call it extension. This would be more clear especially when you want to import create_wrapper, you would be calling engine.extension.create_wrapper, whose purpose can be hinted by the namespace.

Looks fine to me (assuming the tests are fixed, which looks to me simple, but I could be wrong)

@kitchoi
Copy link
Contributor

kitchoi commented Apr 28, 2016

Another alternative would be to move imports to the module-level functions. This is generally justified by having to avoid import cycles.

@codecov-io
Copy link

codecov-io commented Apr 28, 2016

Current coverage is 96.51%

Merging #291 into master will increase coverage by +<.01%

@@             master       #291   diff @@
==========================================
  Files            47         48     +1   
  Lines          3806       3814     +8   
  Methods           0          0          
  Messages          0          0          
  Branches        571        571          
==========================================
+ Hits           3673       3681     +8   
  Misses           44         44          
  Partials         89         89          

Powered by Codecov. Last updated by acc8d52...9ba72a6

@mehdisadeghi
Copy link
Contributor Author

put ext under the engine directory

This will not solve the issue. Loading extensions happen upon importing engine which executes engine.__init__. When an extension imports anything from engine package, the load_metadata method fails, because the classes inside the extension module are not yet defined.

Another alternative would be to move imports to the module-level functions

Yes this is possible. However, it implies relying on extension developers to do this (because this only happens when the first import is triggered from within an extension), which I prefer to avoid.

instead of calling ext, call it extension

I will rename the package.

@kitchoi
Copy link
Contributor

kitchoi commented May 2, 2016

I think the one thing that is blocking you is the create_wrapper, which is in both extension.__init__ and engine.__init__. I see you want to expose the function to the public API, is it possible to have it in one place and not in both?

@mehdisadeghi
Copy link
Contributor Author

one thing that is blocking you is the create_wrapper

I removed that. It is not necessary (and not blocking anything either).

@kitchoi
Copy link
Contributor

kitchoi commented May 2, 2016

Looks good! 👍

@mehdisadeghi mehdisadeghi merged commit d1c878f into master May 2, 2016
@mehdisadeghi mehdisadeghi deleted the fix-cyclic-imports branch May 2, 2016 11:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problem: Cyclic import if loading extensions prior to simphony itself.
3 participants