Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid circular import in registry #288

Closed
sevdog opened this issue Oct 24, 2018 · 5 comments · Fixed by #365
Closed

Avoid circular import in registry #288

sevdog opened this issue Oct 24, 2018 · 5 comments · Fixed by #365
Labels
Projects

Comments

@sevdog
Copy link
Contributor

sevdog commented Oct 24, 2018

In current version (3.0.0) there is a circular import involving registry.py which makes interpreter to fully load the whole library everytime a calendar is used.

The circular may be summarized like this: registry.py -> <continent-module> -> <country-cal>.py -> registry.py. This however works thanks to how interpreters read the code (ie: iso_register decorator is defined before importing every country in registry.py, so it works).

To test this try:

# see https://docs.python.org/3.5/library/trace.html
from trace import Trace
tracer = Trace() 
# make run a simple import for a single country
tracer.run('from workalendar.europe.italy import Italy')
res = tracer.results() 
# print results
res.write_results(coverdir='.', summary=True)

You will see that to load a single calendar the whole library was loaded. This may be fine in some cases, but its a waste of execution time and memory.

A kind of lazy registry may perform better. However some kind of mapping and a bit of logic are required for this to be accomplished.

@brunobord
Copy link
Member

Interesting. I suspected that something was wrong when I was working on this global registry but I couldn't get my head around it.
What do you think would be the best way to split these gracefully?

@brunobord brunobord added the bug label Oct 25, 2018
@brunobord brunobord added this to To Do in Workalendar Oct 25, 2018
@sevdog
Copy link
Contributor Author

sevdog commented Oct 28, 2018

This is not an easy guess.

Maybe mappings could be defined inside a JSON or in a config dict, using a string, then importlib could be used to load the right element (in a way like django does with its import_string method. This approach may be less pythonic (cause registration would be done by via "config" file instead then by decorator) and could bring some annoiyng problem when a class is moved from one file the an other, but should drastically reduce the number of module loads.

ATM this was my first idea, I am not saying its the best option.

@brunobord
Copy link
Member

back on the saddle on this. I need some fresh ideas, let's see what can emerge, now.

@brunobord brunobord moved this from To Do to Working in Workalendar May 24, 2019
@brunobord
Copy link
Member

I think I've got something in this branch: https://github.com/peopledoc/workalendar/compare/288-new-registry

Invoking @Natim because he was the one who implemented the ISO register in the first place. Do you remember why you've used OrderedDict? Does it really matter to remember the order of the items were inserted?

@Natim
Copy link
Contributor

Natim commented May 24, 2019

You can probably get rid of it

Workalendar automation moved this from Working to Done Jun 7, 2019
brunobord added a commit that referenced this issue Jul 11, 2019
…ct's

refs #375 and somehow refers to #288 ; when it has been raised here: #288 (comment)
brunobord added a commit that referenced this issue Jul 11, 2019
…ct's

refs #375 and somehow refers to #288 ; when it has been raised here: #288 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Workalendar
  
Done/Closed/Published
Development

Successfully merging a pull request may close this issue.

3 participants