Slow calculation and memory leaking with 0.5.1

[jkfm]

Hi all,

I am using pvlib to calculate many pv arrays and do this by looping over the individual system specs. Since updating on 0.5.1, I noticed much longer calculation times and a steady increase in memory usage.

From a quick glance at the changes from 0.5.0 I couldn't determine an obvious cause for this behavior. Did I miss something?

My parameters for the calculation are
ModelChain(system, location, dc_model='singlediode', ac_model='snlinverter', spectral_model='no_loss', temp_model='sapm', aoi_model='ashrae', solar_position_method='nrel_numba', transposition_model='haydavies', losses_model='pvwatts', clearsky_model='simplified_solis', orientation_strategy=None)

[wholmgren]

It could be related to fixing ModelChain to actually use solar_position_method, especially since you're specifying nrel_numba. See #377 and #379. I suggest setting it to nrel_numpy and retesting the performance of your code. Also make sure that you're consistently using nrel_numba for every single solar position calculation in your whole script. This is because the spa module needs to be reimported when the method changes, and this takes some time, especially when the module needs to be JIT compiled again. Note that methods such as Location.get_clearsky will use the default solar position method (nrel_numpy) unless explicitly told otherwise.

Is pvlib the only thing that's changed in your environment?

[jkfm]

Thanks for your suggestions! I will look into them.

Updating pvlib was not the only change that occurred, but after reverting to 0.5.0 the behavior was normal again.

[markcampanelli]

Looks like you are using the single diode model, so it also could be my changes to support ideal devices.

[wholmgren]

@jkfm did you get a chance to look into what might have caused the pvlib performance problem?

[jkfm]

Not yet :-( But I will report back.

[cwhanse]

@jkfm any reports back from your investigation? We'd like to fix the issue you experienced, if we can narrow down the cause.

[jkfm]

Hey all, sorry for the delay. I am now looking into the problem.

I wrote a script which replicates my actual use case but is much simpler: https://gist.github.com/jkfm/6c5668da8f28403bf37268cda4afeb05

The code for following memory usage is from here.

With 30 iterations (my RAM can't handle more ;-) ), the code has the following stats:
0.5.0:
duration 4s,
memory usage:

0.5.1:
duration: 1min 47s
memory usage:

I realise, that the memory usage is not really representative as such, but I guess the trend is clear.

[jkfm]

As the script with 0.5.1 also throws different warnings I suspected the reason for the lag to be in v_from_i and i_from_v in pvsystem.py

I manually changed only those two functions back to their state from 0.5.0 et voilà:

Duration: 4.8s
Memory usage:

So I guess the error has to do with changes in v_from_i and i_from_v

@wholmgren
I also checked your suggestion regarding solar_position_method. There is no significant difference between nrel_numba and nrel_numpy.

Some more information regarding the warnings:
With the old code, I get these warnings once during the first iteration:
/pvlib/pvsystem.py:1958: RuntimeWarning: invalid value encountered in true_divide I = -V/(Rs + Rsh) - (nNsVth/Rs)*lambertwterm + Rsh*(IL + I0)/(Rs + Rsh)
/pvlib/pvsystem.py:1869: RuntimeWarning: overflow encountered in exp argW = I0 * Rsh / nNsVth * np.exp(Rsh * (-I + IL + I0) / nNsVth)

With the new code, I get this warning for each iteration:
/pvlib/pvsystem.py:1919: RuntimeWarning: overflow encountered in exp (Gsh[idx_p]*a[idx_p]))

[cwhanse]

Confirmed. I get different numbers for mem_usage using your script, and not as dramatic as your results for v0.5.1, but the trend is the same: since v0.5.1 we have a memory leak.

[wholmgren]

Thanks @jkfm for this very detailed report!

[wholmgren]

good candidate for #530

[cwhanse]

Looked into this a bit more with tracemalloc, the memory leak appears to depend on using numba with spa, because the memory usage is stable with solar_position_method = nrel_numpy. That is with pvlib v0.6.0.

[wholmgren]

Interesting. I thought the earlier report put the blame on iv functions.

…

On Tue, Nov 6, 2018 at 8:43 AM Cliff Hansen ***@***.***> wrote: Looked into this a bit more with tracemalloc, the memory leak appears to depend on using numba with spa, because the memory usage is stable with solar_position_method = nrel_numpy. That is with pvlib v0.6.0. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#401 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AELiRw63XnnDQOzqd3ihJSj7qf3uWdhsks5usa4egaJpZM4RCDHw> .

[cwhanse]

Looking at the v0.5.1 release notes, maybe the issue was obscured in earlier releases and exposed by #482

[wholmgren]

@cwhanse can you reproduce using only solarposition.spa_python(how='numba')?

[cwhanse]

Doesn't leak when using solarposition.spa_python(how='numba') directly.
Doesn't leak when using Location.get_solarposition(how='numba').
Leaks when using ModelChain.location.get_solarposition(how='numba').

[cwhanse]

https://gist.github.com/cwhanse/02bb64c551af2083ffee587674ab966e

[wholmgren]

That's a helpful report + gist.

@jkfm previously said

I also checked your suggestion regarding solar_position_method. There is no significant difference between nrel_numba and nrel_numpy.

@cwhanse what does your modelchain-based script report when changing the method?

[wholmgren]

Looking for other differences between the 3 scripts... I'll speculate that result['output'] += mc.ac could cause python to hold on to the entire ModelChain object. In my experience gc.collect rarely helps.

[cwhanse]

@wholmgren when the model chain script has method=numpy there is no memory leak.

[cwhanse]

Still leaks with result['output'] += mc.ac commented out.

[wholmgren]

I'm almost out of ideas.

Leaks when using ModelChain.location.get_solarposition(how='numba').

Does it matter if it's called directly or as part of the run_model method? In other words, what happens if you simplify the mc_loop to

def mc_loop(times):
        system = get_system()
        location = get_location()
        mc = get_mc(system, location)
    
        mc.solar_position = mc.location.get_solarposition(times)

[cwhanse]

I think I know why this memory leak happens, and I think there's an issue for us but I haven't quite got the issue nailed down. In @jkfm script, mc.solar_position is computed twice each loop, once explicitly using the Location.get_solarposition method, and again inside ModelChain.run_model at the prepare_inputs step:

for i in range(rg):
    system = get_system()
    location = get_location()
    mc = get_mc(system, location)
    mc.solar_position = mc.location.get_solarposition(times)
    weather = get_weather(mc, times)
    mc.run_model(times=times, weather=weather)

The memory leak only happens when both calculations are in the loop: comment either one out, and no leak.

The leak appears to be caused by cycling between nrel_numpy and nrel_numba values for the solar_position_method kwarg.

When mc is created, solar_position_method is set to nrel_numba. However, that attribute is NOT used by the attached Location object's method

mc.solar_position = mc.location.get_solarposition(times)

As a consequence, the first time solar position is calculated, nrel_numpy (the default) is used. The next time is via this line in ModelChain.prepare_inputs:

       self.solar_position = self.location.get_solarposition(
            self.times, method=self.solar_position_method)

which passes the value self.solar_position_method='nrel_numba', assigned when the ModelChain instance is created.

The issue for us, is that ModelChain.location brings a method get_solarposition that does not inspect ModelChain.solar_position_method and hence if called, defaults to nrel_numpy. How to fix this, or whether to fix it, is where I'm unclear.

[wholmgren]

Ah, that is very interesting. pvlib tells python to reload the spa module when the method is changed. This is required due to how the numba jit function works.

An easy thing would be to emit a user warning each time the spa module is reloaded.

Also I have no idea why this leads to a memory leak, but I'm not shocked that it does.

[cwhanse]

Cause confirmed. Adding the method argument

mc.solar_position = mc.location.get_solarposition(times, method=mc.solar_position_method)

stops the leak.

[cwhanse]

Maybe a ModelChain.get_solarposition method would prevent this mishap, by simply wrapping ModelChain.location.get_solarposition.

I'm guessing @jkfm has the solar position computed explicitly because it's not obvious that prepare_inputs will do this for you. Maybe also we could inspect ModelChain and only compute solar position if it's not already there:

if not self.solarposition:
 ....

[wholmgren]

It also takes a few seconds for python to reload the spa module when jitting it. Users should be discouraged from doing this. I don't think this is a ModelChain issue.

I once ran into a similar problem in some code in which I ran solarposition.get_solarposition and then later ModelChain.run_model. In my case I only noticed an unexplained slowness, not a memory leak, but it's possible there was a memory leak problem too. My point is only that there are multiple ways for users to accidentally switch between numba and non-numba calls, and we should try to prevent that from happening at a lower level.

[cwhanse]

Yes, the performance hit for numpy/numba swapping is noticeable. Unless you object I'll prepare a PR with:

• a ModelChain.get_solarposition method, that wraps the Location method. That way we can propagate the solar_position_method attribute. It can be bypassed, but at least we've made it convenient not to do so.

• add a warning when spa is reloaded.

I thought better of an if/then around that statement in prepare_inputs, there's no guarantee that solarposition is consistent with times if solarposition is calculated elsewhere.

[cwhanse]

I once ran into a similar problem in some code in which I ran solarposition.get_solarposition and then later ModelChain.run_model. In my case I only noticed an unexplained slowness, not a memory leak

That's basically what @jkfm was doing.

[wholmgren]

@cwhanse thanks for volunteering to make a PR.

add a warning when spa is reloaded.

+1. Warning should probably go here and here.

a ModelChain.get_solarposition method, that wraps the Location method. That way we can propagate the solar_position_method attribute. It can be bypassed, but at least we've made it convenient not to do so.

I moderately object to this because the reload problem would already be covered by the new warning and ModelChain is already too complicated. I would be in favor of it if we could list a handful of compelling use cases (other than this numba issue). That might be a separate issue.

I thought better of an if/then around that statement in prepare_inputs, there's no guarantee that solarposition is consistent with times if solarposition is calculated elsewhere.

I agree that this could easily lead to trouble and should be avoided.

[cwhanse]

I agree with ModelChain being busy, but I don't see it as complicated. I'm in favor of taking the solar position calculation out of prepare_inputs as its own method, if an issue like this arises again.

If there was an easy way to access the solar_position_method attribute from mc.location instance (where mc is an instance of ModelChain), we could smooth out this wrinkle. But I don't see how to do it without changing ModelChain.location = location to use inheritance.