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

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

Open
wholmgren opened this issue Sep 15, 2020 · 9 comments
Open

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

wholmgren opened this issue Sep 15, 2020 · 9 comments

Comments

@wholmgren
Copy link
Member

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 #1059 (comment)

@wholmgren
Copy link
Member Author

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

@kandersolar
Copy link
Member

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!
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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants