search

ProjectDrawdown / solutions

The mission of Project Drawdown is to help the world reach “Drawdown”— the point in the future when levels of greenhouse gases in the atmosphere stop climbing and start to steadily decline, thereby stopping catastrophic climate change — as quickly, safely, and equitably as possible.
https://www.drawdown.org/
Other
211 stars 91 forks source link

Add creation_date to AdvancedControls python object, to correspond with its addition to the .json files. #469

Closed johnpaulalex closed 2 years ago

johnpaulalex commented 2 years ago

Can't verify. I ran 'python -m pytest solution/commercialglass' and it said _ ERROR collecting solution/commercialglass/tests/test_commercialglass.py __ solution/commercialglass/tests/test_commercialglass.py:6: in from tools import expected_result_tester tools/expected_result_tester.py:16: in from model import scenario model/scenario.py:15: in from solution import factory solution/factory.py:25: in def load_scenario(solution, scenario=None) -> scenario.Scenario : E AttributeError: partially initialized module 'model.scenario' has no attribute 'Scenario' (most likely due to a circular import)

...and that happens on a clean branch too, so there's some other issue that needs fixing before we can confirm this PR is ok.

denised commented 2 years ago

Wow, that is obscure. It happens to me too, but only when I try to run pytest on solution or as you did, a specific solution. Running just 'pytest' (on the whole directory) works fine. I can see the problem is the type hint I added to load_scenario recently, so I will remove that asap, since it is not necessary.

Someday maybe I'll figure out why this happened at a deeper level.

-- Denise Draper @.***

On Fri, Sep 24, 2021, at 5:22 PM, johnpaulalex wrote:

Can't verify. I ran 'python -m pytest solution/commercialglass' and it said _ ERROR collecting solution/commercialglass/tests/test_commercialglass.py __ solution/commercialglass/tests/test_commercialglass.py:6: in from tools import expected_result_tester tools/expected_result_tester.py:16: in from model import scenario model/scenario.py:15: in from solution import factory solution/factory.py:25: in def load_scenario(solution, scenario=None) -> scenario.Scenario : E AttributeError: partially initialized module 'model.scenario' has no attribute 'Scenario' (most likely due to a circular import)

...and that happens on a clean branch too, so there's some other issue that needs fixing before we can confirm this PR is ok.

You can view, comment on, or merge this pull request online at:

https://github.com/ProjectDrawdown/solutions/pull/469

Commit Summary

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/ProjectDrawdown/solutions/pull/469, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKAMTUC6XZOKXOKCH6LV4TUDUI2VANCNFSM5EW6WJQQ.

denised commented 2 years ago

Pull the most recent commit and you should be good to go.

-- Denise Draper @.***

On Fri, Sep 24, 2021, at 5:32 PM, Denise Draper wrote:

Wow, that is obscure. It happens to me too, but only when I try to run pytest on solution or as you did, a specific solution. Running just 'pytest' (on the whole directory) works fine. I can see the problem is the type hint I added to load_scenario recently, so I will remove that asap, since it is not necessary.

Someday maybe I'll figure out why this happened at a deeper level.

-- Denise Draper @.***

On Fri, Sep 24, 2021, at 5:22 PM, johnpaulalex wrote:

Can't verify. I ran 'python -m pytest solution/commercialglass' and it said _ ERROR collecting solution/commercialglass/tests/test_commercialglass.py __ solution/commercialglass/tests/test_commercialglass.py:6: in from tools import expected_result_tester tools/expected_result_tester.py:16: in from model import scenario model/scenario.py:15: in from solution import factory solution/factory.py:25: in def load_scenario(solution, scenario=None) -> scenario.Scenario : E AttributeError: partially initialized module 'model.scenario' has no attribute 'Scenario' (most likely due to a circular import)

...and that happens on a clean branch too, so there's some other issue that needs fixing before we can confirm this PR is ok.

You can view, comment on, or merge this pull request online at:

https://github.com/ProjectDrawdown/solutions/pull/469

Commit Summary

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/ProjectDrawdown/solutions/pull/469, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKAMTUC6XZOKXOKCH6LV4TUDUI2VANCNFSM5EW6WJQQ.

DentonGentry commented 2 years ago

My suspicion would be the metaclass cache https://github.com/ProjectDrawdown/solutions/blob/develop/model/metaclass_cache.py, which is a sizable performance win but can result in caching something across pytest cases. In this case, I guess it would be caching something which is fully initialized where the individual test doesn't fully initialize it? If you put your type hint back, but remove the metaclass cache, and the error goes away that would be pretty telling.

The metaclass cache had been pretty well debugged previously, but still seems plausible.

denised commented 2 years ago

@DentonGentry Interesting thought --- I'll give it a shot.

johnpaulalex commented 2 years ago

The issue is the circular dependency, no? scenario and factory import each other. Seems like factory is a pretty small class and the two modules don't have a lot of dependencies between them; there are probably a few ways to untangle it.

I would guess that they load successfully if you import scenario first, but not if factory is imported first. So maybe running all the tests coincidentally imports scenario first? Seems like factory needs scenario on startup (to dereference the type hint) whereas scenario doesn't need factory (it calls it in a method, which won't try to actually dereference until it's called).

johnpaulalex commented 2 years ago

Ok I repulled and I think this PR is ready to go - PTAL.