pvlib / pvlib-python

A set of documented functions for simulating the performance of photovoltaic energy systems.
https://pvlib-python.readthedocs.io
BSD 3-Clause "New" or "Revised" License
1.19k stars 997 forks source link

reevaluate numba minimum, usage patterns in spa.py #1060

Open wholmgren opened 4 years ago

wholmgren commented 4 years ago

conda search -c main numba | grep py36 shows that numba 0.36.1 is the oldest package compatible with python 3.6 and numpy 1.12. The numba git history says 0.36.1 was released on Dec 7, 2017. (0.17.0 was released Feb 3, 2015.) I'll add that to the asv conf.

I'd be fine with also adding a minimum numba requirement to setup.py, but I think that should be done in combination with changes to the import logic in spa.py. In particular, I don't like the numpy fallback - that led to hard to track down errors when developing this. It would also be worth reviewing modern numba best practices. If we're adding a minimum numba requirement to setup.py then we should probably be testing against it too. More than I want to tackle in this PR.

_Originally posted by @wholmgren in https://github.com/pvlib/pvlib-python/pull/1059#discussion_r488775560_

wholmgren commented 4 years ago

@kanderso-nrel looked into adding numba support to temperature.fuentes in #1037. Whatever we do in spa.py should simplify that process.

kandersolar commented 3 years ago

I think my hesitation in #1037 around numba was in part because the SPA code switches numba on/off based on what the user has requested (how='numba'), not just whether numba is installed or not. I think it's valuable to be able to turn numba off somehow, but I don't think adding a how parameter to every numba-accelerated function is a good idea. And although I like being able to disable numba, I would guess that not many people want to enable/disable numba for specific function invocations, and the extra complexity is hard to reason about, at least for me. See also #401.

If we're willing to give up the call-by-call numba control, I propose to enable or disable numba (by using a dummy @jit decorator) when pvlib is imported according to the behavior in this table. The default is to use numba if it's available, but that can be overridden by an environment variable if needed:

PVLIB_USE_NUMBA numba installed? result
no value no dummy
no value yes numba jit
0 no dummy
0 yes dummy
1 no error
1 yes numba jit

Curious what people think about this. Here's an example decorator definition for reference:

Click to expand! ```python import os use_numba = os.getenv('PVLIB_USE_NUMBA', None) def dummy_jit(func=None, *args, **kwargs): # accommodate using as either `@jit` or `@jit()` def wrapper(func): return func if func is not None: return func return wrapper if use_numba == '0': jit = dummy_jit else: try: from numba import jit except ImportError: if use_numba is None: jit = dummy_jit else: raise ```
wholmgren commented 3 years ago

I'm fine with giving up call-by-call numba control. We could also give up PVLIB_USE_NUMBA and instead point users to NUMBA_DISABLE_JIT. Situations in which a user wants numba for one library but not another are probably pretty rare, so I'm skeptical that it's worth our time to code around that.

kandersolar commented 3 years ago

Slowly making progress here. I have a numba integration that I'm happy with for temperature.fuentes and now I'm looking at the SPA code. What should we do with the numpy/numba switch in solarposition.get_solarposition? Would it make sense to deprecate method='nrel_numpy' and method='nrel_numba' in favor of a new method='nrel' (or something) and remove solar_position._spa_python_import?

wholmgren commented 3 years ago

That makes sense to me. For simplicity, the deprecation could just be for the kwargs and we could make an abrupt change in the actual behavior. I'd be ok with that if it occurred in a 0.9 release. If you think deprecating the behavior can be done cleanly then I'm good with that too.

kandersolar commented 3 years ago

I'm having difficulty getting the tests working in #1098. I think the fundamental issue is that spa.py falls back to an entirely different code path if numba isn't available, and the test suite wants to exercise both branches (numba and numpy) but having both branches active at the same time in a pytest session is a big mess. If anyone can point me to another package with this pattern (different functions depending on whether numba is used), that would be helpful -- I did some searching but it feels like a needle in a haystack.

One option that would make #1098 easier would be to add numba as a non-optional requirement so we can drop the non-numba functions. I think requiring numba is probably a lot more practical today than it was some years ago; the LLVM dependency now has wheels on pypi, but I'm not sure about microcomputer compatibility. Is requiring numba a possibility?

wholmgren commented 3 years ago

I'm -1 on requiring numba at this time. In part a desire to be cautious about dependencies and in part a desire to avoid forcing the jit cost on every workflow. For now, do you have any concern with using pytest skip markers for both numba and not numba paths in spa?

kandersolar commented 3 years ago

Note that jitting can always be prevented with NUMBA_DISABLE_JIT or numba.config.DISABLE_JIT, but dependency caution is fair.

For pytest skips, it's hard to say without trying it out, but I would guess that it wouldn't solve the problem unless we skipped everything that uses numba. I shouldn't have said "entirely different code paths" in my earlier comment -- they are different at first, but they end up using the same set of SPA utility functions, which need to be jitted for the numba path and not jitted for the numpy path. I'm not sure how to accomplish that in the tests without a house of cards built on importlib.reload().

wholmgren commented 3 years ago

I was thinking we'd rely on different ci configurations for total coverage. The coverage within a single configuration would be incomplete. So I don't think we'd need any special import or environment machinery.