-
Notifications
You must be signed in to change notification settings - Fork 2
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
enable windows wheels #24
Conversation
It looks like Appveyor needs to be configured to monitor this repo. I cancelled Travis to save CI resources. |
Thanks for looking at this, @grlee77. I looked in AppVeyor to see if I could add this to the org, but seem to lack the permissions. Guessing this is the same for you. @hgomersall, would you be able to look? FWIW AppVeyor has made some major improvements to their UI. |
Okay, Appveyor is now trying to build this. Thanks @hgomersall. I will try to take a look at this PR again later today. |
Great, thanks! |
For some reason the submodule initialization on Appveyor is failing with:
any ideas? |
Is it being created during the build, and then the deployment is failing? I seem to recall something like this on the previous way of doing things - the deploy script needed to delete the directory before building the wheel (or something). |
This was based off of a combination of the script at scikit-image-wheels with the scripts in the appveyor subfolder added from the main pyFFTW repository
…d and run_with_env.cmd
Okay. I see what happened now. The FFTW .dlls were placed in a pyFFTW subdirectory, but submodule init cannot be run after that is done. |
as of commit 4ca0f6e, builds are completing successfully, but there was a remaining issue of trying to make sure we are not doing a pre-release build that I am still working on in d81a121. I am unable to cancel pending Appveyor builds, but that is probably an issue related to collaborator permissions on Appveyor. One outstanding question I could use some help with: There is some MSVC configuration going in the main
I am not sure if this conflicts with some related settings that are going on within the |
This is great stuff, well done! |
AFAIK VS 2010 was only used with Python 3.4. If we are not supporting Python3.4, that line can probably be dropped. |
@jakirkham sorry, can you clarify what you need? Do you want to be made a collaborator too? |
Sure, that sounds good. Though my comment was more about the issue @grlee77 raised about the |
…build wheels based on a release tag
One issue in the d81a121 builds is that the CONTAINER variable was still getting set improperly to "pre-release" instead of "wheels". I think we would have to be building from a different branch than master on pyFFTW-wheels if we want the logic to work properly as it does for scikit-image-wheels. This variable effects where the wheels get uploaded to so it needs to be set to "wheels" so that the wheels don't end up in the pre-release location. I am going to simplify by removing this pre-release logic as I don't think we need it here. I think just building from release tags is sufficient for now. |
feel free to cancel builds 1.0.11 and 1.0.12 on Appveyor. I only need to see the result for the final commit. |
Okay. This is ready for review. The final If there is still a need for 32-bit windows wheels, they can be added, but I have only built the 64-bit ones for now. |
Okay, this is all green now with 0.11.1. @hgomersall: can you add me to the ci team under the pyFFTW organization on GitHub? I would like to be able to review and merge future PRs from others in this repo. |
.appveyor.yml
Outdated
# Check Python version just in case | ||
- python --version | ||
# Run unit tests with pytest | ||
- "python setup.py test" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we cleanup the downloaded FFTW DLLs before this step to ensure it is picking the ones in the wheel up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking more closely, the wheel is not actually being used during testing (or even getting installed!). This is because of the following build using:
python setup.py -v build_ext --inplace
that gets used for the tests. This approach was copied from the appveyor script in the main repo.
I can comment out that line installing the wheel as it is not doing anything. The tests do not actually get distributed with the source .tar.gz or the wheels, so I am actually not even sure how to even run the tests without using an inplace build. The comment mentions "pytest", but this was a copy/paste error. Tests effectively get run via:
python -m unittest discover
and this only works for me if I do it from the top level pyfftw folder with an in-place build.
I did verify on a Windows 7 machine that downloading the Python 3.5 wheel from the Appveyor artifacts and installing it does import and run basic ffts okay and the expected dynamic libraries are in the .whl.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually you can run the tests as long as the package is installed somewhere the Python interpreter can find it. So would try pip install
ing the wheel, cleaning the source directory, and running the tests with python -m unittest discover
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That said, it's not obvious to me if we are running the tests for the other platforms. So maybe this is good enough as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have more variants in this repo than we test in the main pyFFTW one, so it would probably be worth having the tests all running here as well at some point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried testing as you suggest locally (i.e. installing from the wheel, cleaning the source directory with git clean -xfd
and then running tests via python -m unittest discover
. It finds the tests, but I get a ton of ImportErrors
.
C:\Users\Greg\src\pyFFTW>python -m unittest discover
EEEEEEEEEEEEEEEEEE
======================================================================
ERROR: pyfftw (unittest.loader._FailedTest)
----------------------------------------------------------------------
ImportError: Failed to import test module: pyfftw
Traceback (most recent call last):
File "F:\Miniconda3\lib\unittest\loader.py", line 462, in _find_test_path
package = self._get_module_from_name(name)
File "F:\Miniconda3\lib\unittest\loader.py", line 369, in _get_module_from_name
__import__(name)
File "C:\Users\Greg\src\pyFFTW\pyfftw\__init__.py", line 18, in <module>
from .pyfftw import (
ImportError: No module named 'pyfftw.pyfftw'
======================================================================
ERROR: test.test_pyfftw_base (unittest.loader._FailedTest)
----------------------------------------------------------------------
ImportError: Failed to import test module: test.test_pyfftw_base
Traceback (most recent call last):
File "F:\Miniconda3\lib\unittest\loader.py", line 428, in _find_test_path
module = self._get_module_from_name(name)
File "F:\Miniconda3\lib\unittest\loader.py", line 369, in _get_module_from_name
__import__(name)
File "C:\Users\Greg\src\pyFFTW\test\test_pyfftw_base.py", line 35, in <module>
from pyfftw import FFTW, _supported_types, _all_types_human_readable
File "C:\Users\Greg\src\pyFFTW\pyfftw\__init__.py", line 18, in <module>
from .pyfftw import (
ImportError: No module named 'pyfftw.pyfftw'
If I first cd test
to move out of the root source directory and into the test
subfolder. I get similar errors, but this time about invalid relative imports.
======================================================================
ERROR: test_pyfftw_builders (unittest.loader._FailedTest)
----------------------------------------------------------------------
ImportError: Failed to import test module: test_pyfftw_builders
Traceback (most recent call last):
File "F:\Miniconda3\lib\unittest\loader.py", line 428, in _find_test_path
module = self._get_module_from_name(name)
File "F:\Miniconda3\lib\unittest\loader.py", line 369, in _get_module_from_name
__import__(name)
File "C:\Users\Greg\src\pyFFTW\test\test_pyfftw_builders.py", line 38, in <module>
from .test_pyfftw_base import run_test_suites, require
ImportError: attempted relative import with no known parent package
Looks pretty good to me. I don't totally understand everything going on here, but IDK if it's necessary for me to understand it if you do. Did ask a couple questions above though. |
For now tests are run from an inplace build performed immediately after the wheel build. It is probably preferable to install the wheel and use that for running the tests, but this may require some changes to upstream pyFFTW before it can be implemented here.
Can someone please merge this if there are no strong objections? Not having wheels on PyPI is resulting in a lot of issues being raised at the main repo. |
ping. can we merge this soon and work out improvements to running the tests in the future? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy
Apologies for the delay on this, I'm rather busy at the moment! |
no problem. thanks @hgomersall |
I do not see the Windows wheels on Rackspace, but they were built and are available via the Appveyor artifacts tab. Unfortunately the linux/OS X wheels got built from the latest commit and not the 0.11.1 tag, so I have a new PR at #27 to try and fix that prior to uploading everything to PyPI. |
This is built on top of #22. The new commits start from 94367ca