Fix numpy and nose import errors at installation time #11
Conversation
|
I was able to create a wheel file that doesn't require Thanks @snuderl for this patch! |
|
Btw I just realized that this package seems to be built for python2 only, this will become an issue |
|
I thibk we just need to build one for python3 too. Or enable unoversal
builds if possible
…On Wed, 20 Feb 2019, 10:04 Marc A. Anoma ***@***.*** wrote:
Btw I just realized that this package seems to be built for python2 only,
this will become an issue
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#11 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA_5MpdfnSAyC7IHTnd-dccs91cFI-XFks5vPQ-lgaJpZM4bEs44>
.
|
|
It is already ready to build Python-3 - see |
setup.py
Outdated
| @@ -137,6 +137,7 @@ def link_dylib_lib(self, objects, output_libname, output_dir=None, | |||
| PKG_DATA.append(os.path.join('src', 'orig', 'solpos', '*.*')) | |||
| PKG_DATA.append(os.path.join('src', 'orig', 'spectrl2', '*.*')) | |||
| elif not LIB_FILES_EXIST: | |||
| from solar_utils.tests import test_cdlls | |||
There was a problem hiding this comment.
Better to wrap this in a try:except block. This patch solves the issue of installs using wheels when the user doesn't have Numpy, but I believe this will still fail if Numpy or Nose is not installed for installs that build from source.
Another option to consider is adding install_requires=['nose>=1.3.7', 'numpy>=1.8.1']and possibly tests_requiresto the setup file so that in installs Numpy and Nose by default. I think that would work, perhaps in addition to thetry:excpt` block. But it depends if you want to make those requirements, maybe you don't want SolarUtils to depend on Numpy or Nose, since they're not used anywhere else in the package.
There was a problem hiding this comment.
Thanks @mikofski ! Quick questions:
- In which cases would that import fail?
- since "SolarUtils has no requirements for usage" I would be in favor of adding it to
test_requiresif we had to choose - how difficult or easy would it be to remove the dependency on numpy and nose altogether?
There was a problem hiding this comment.
Also, is there any reason why we didn't enable universal builds for this package?
There was a problem hiding this comment.
Also, is there any reason why we didn't enable universal builds for this package?
No, I don't think so because this package uses extensions that are platform and Python specific.
In which cases would that import fail?
If the user downloads a source distribution or clones the repo, and tries to install or build, then setup.py will call the tests
how difficult or easy would it be to remove the dependency on numpy and nose altogether?
just rewrite the tests to not use nose or numpy.
since "SolarUtils has no requirements for usage" I would be in favor of adding it to
test_requiresif we had to choose
yah, I agree, test_requires is better, and then I would remove the test calls in setup.py
Actually this seems like the right idea, then you can also just remove the failing import from setup.py and problem solved! Then ignore my other comment on that line
| @@ -17,7 +17,6 @@ | |||
| __version__ as VERSION, __name__ as NAME, __author__ as AUTHOR, | |||
| __email__ as EMAIL, __url__ as URL | |||
| ) | |||
| from solar_utils.tests import test_cdlls | |||
There was a problem hiding this comment.
maybe replace this with a try-catch block and dummy functions?
try:
from solar_utils.tests import test_cdlls
except ImportError as _:
exc = _
class test_cdlls:
def test_solposAM():
raise exc
def test_spectrl2():
raise exc
|
FYI: there's also a sunpower Anaconda channel - you can use conda skeleton and conda build to upload files for conda to anaconda or try conda-forge instead. |
|
FYI: I just uploaded a Python-3.7 wheel for Windows (amd64) to PyPI. Use pip to install. |
Fix #10