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.

Sign up for GitHub

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

Jump to bottom

Using Github CI to generate wheels and compiling to Windows too. #345

Merged
merged 2 commits into from
Apr 2, 2022
Merged

Using Github CI to generate wheels and compiling to Windows too. #345

merged 2 commits into from
Apr 2, 2022

Conversation

tfmoraes
Copy link
Contributor

This PR does 2 things:

  1. Make it possible to compile to Windows. I did some change to setup.py and created the bat files to configure ITK and AntPy. I used this tool to help convert from shell to batch, but I needed to some conversions by hand. I tried these 2 tutorials (https://github.com/ANTsX/ANTsPy/blob/master/tutorials/Transform%20Generators.ipynb and https://github.com/ANTsX/ANTsPy/blob/master/tutorials/motionCorrectionExample.ipynb) and everything worked except the registration, it's showing this error:
 file 0000021DE7BF8090 does not exist .
Exception Object caught:

itk::ExceptionObject (000000DE4EFE8360)
Location: "unknown"
File: C:\Users\Oscar\Sources\sss\ANTsPy\itksource\Modules\Registration\Common\include\itkCenteredTransformInitializer.hxx
Line: 31
Description: ITK ERROR: CenteredTransformInitializer(0000021D82F05C90): Fixed Image has not been set


 file 0000021DE7BF7FF0 does not exist .
Exception Object caught:

itk::ExceptionObject (000000DE4EFE8360)
Location: "unknown"
File: C:\Users\Oscar\Sources\sss\ANTsPy\itksource\Modules\Registration\Common\include\itkCenteredTransformInitializer.hxx
Line: 31
Description: ITK ERROR: CenteredTransformInitializer(0000021D82F05300): Fixed Image has not been set
  1. Create Python wheels using Gihub CI workflows. It'll compile and generate the wheels to Linux, Mac and Windows. I tried to generate the wheels to Mac M1 but I was not able to do that, in some cases it is possible to compile to M1 using the Mac nodes from Github CI. I think we'll need to wait Github to offer M1 nodes. This workflow is running for each commit, but it's possible to make it run just for PR merged, new tags, etc. After the workflow run it will generate an artifact with the wheels files compacted. Like in this run https://github.com/tfmoraes/ANTsPy/actions/runs/2061650879. You just need to download the artifact file, unzip it, then submit to Pypi using twine. It's possible to upload the files automaticaly using the github workflows https://cibuildwheel.readthedocs.io/en/stable/deliver-to-pypi/#automatic-method

@stnava
Copy link
Member

stnava commented Mar 30, 2022

thank you for this - really helpful. a few comments:

  1. we use pointer magic to wrap the C++ in python ( similar for R ). I suspect whatever we use that works for OSX and Linux builds does not work for windows. Probably solvable but not likely a quick fix.
  2. pypi has a total project limit of 10GB and a per release file size limit of 400MB ( for our project ) .... consequently, we can't store very many releases at the same time on pypi.
  3. have you run the tests? just bash tests/run_tests.sh

@tfmoraes
Copy link
Contributor Author

This is the output of test:

ANTsPy on  wheels_ci [?] via 🐍 pyenv 3.10.2 (my_env) took 59s
❯ .\tests\run_tests.bat

C:\Users\Oscar\Sources\sss\ANTsPy>SET PYCMD=python

C:\Users\Oscar\Sources\sss\ANTsPy>pushd "tests"

C:\Users\Oscar\Sources\sss\ANTsPy\tests>echo "Running core tests"
"Running core tests"

C:\Users\Oscar\Sources\sss\ANTsPy\tests>python test_core_ants_image.py
.C:\Users\Oscar\Sources\sss\my_env\lib\site-packages\ants\core\ants_image.py:479: RuntimeWarning:

invalid value encountered in true_divide

C:\Users\Oscar\Sources\sss\ANTsPy\tests\test_core_ants_image.py:284: RuntimeWarning:

invalid value encountered in true_divide

.........C:\Users\Oscar\Sources\sss\my_env\lib\site-packages\ants\core\ants_image.py:490: RuntimeWarning:

overflow encountered in power

C:\Users\Oscar\Sources\sss\ANTsPy\tests\test_core_ants_image.py:303: RuntimeWarning:

overflow encountered in power

...........................
----------------------------------------------------------------------
Ran 37 tests in 21.143s

OK

C:\Users\Oscar\Sources\sss\ANTsPy\tests>python test_core_ants_image_io.py
.... file 000001E6F5869730 does not exist .
E...
======================================================================
ERROR: test_images_to_matrix (__main__.TestModule_ants_image_io)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Users\Oscar\Sources\sss\ANTsPy\tests\test_core_ants_image_io.py", line 183, in test_images_to_matrix
    imgmat = ants.images_to_matrix(imglist)
  File "C:\Users\Oscar\Sources\sss\my_env\lib\site-packages\ants\core\ants_image_io.py", line 323, in images_to_matrix
    mask = utils.get_mask(image_list[0])
  File "C:\Users\Oscar\Sources\sss\my_env\lib\site-packages\ants\utils\get_mask.py", line 60, in get_mask
    mask_image = threshold_image(image, low_thresh, high_thresh)
  File "C:\Users\Oscar\Sources\sss\my_env\lib\site-packages\ants\utils\threshold_image.py", line 56, in threshold_image
    libfn(processed_args)
RuntimeError: C:\Users\Oscar\Sources\sss\ANTsPy\itksource\Modules\Core\Common\src\itkProcessObject.cxx:1339:
ITK ERROR: BinaryThresholdImageFilter(000001E6F6398850): Input Primary is required but not set.

----------------------------------------------------------------------
Ran 8 tests in 12.345s

FAILED (errors=1)

C:\Users\Oscar\Sources\sss\ANTsPy\tests>python test_core_ants_transform.py
........................
----------------------------------------------------------------------
Ran 24 tests in 3.527s

OK

C:\Users\Oscar\Sources\sss\ANTsPy\tests>python test_core_ants_transform_io.py
...
----------------------------------------------------------------------
Ran 3 tests in 0.499s

OK

C:\Users\Oscar\Sources\sss\ANTsPy\tests>python test_core_ants_metric.py
..........
----------------------------------------------------------------------
Ran 10 tests in 13.159s

OK

C:\Users\Oscar\Sources\sss\ANTsPy\tests>echo "Running learn tests"
"Running learn tests"

C:\Users\Oscar\Sources\sss\ANTsPy\tests>python test_learn.py
 file 00000118B3B2FB30 does not exist .
E file 00000118B3B2FDE0 does not exist .
You have reached an umimplemented section of code by calling:
iMath 2 00000118B3B30070 Normalize 00000118B3B2FDE0

C:\Users\Oscar\Sources\sss\ANTsPy\tests>echo "Running registation tests"
"Running registation tests"

C:\Users\Oscar\Sources\sss\ANTsPy\tests>python test_registation.py
 file 00000200E84E4EF0 does not exist .
 file 00000200E84E4F90 does not exist .
 zero image1 error E file 00000200E84E5200 does not exist .

C:\Users\Oscar\Sources\sss\ANTsPy\tests>echo "Running segmentation tests"
"Running segmentation tests"

C:\Users\Oscar\Sources\sss\ANTsPy\tests>python test_segmentation.py
 file 000001FBEF56FF60 does not exist .

C:\Users\Oscar\Sources\sss\ANTsPy\tests>echo "Running utils tests"
"Running utils tests"

C:\Users\Oscar\Sources\sss\ANTsPy\tests>python test_utils.py
 file 000001FA3FF134F0 does not exist .

C:\Users\Oscar\Sources\sss\ANTsPy\tests>echo "Running bug tests"
"Running bug tests"

C:\Users\Oscar\Sources\sss\ANTsPy\tests>python test_bugs.py
 file 000001C8735E4C20 does not exist .

C:\Users\Oscar\Sources\sss\ANTsPy\tests>popd

@stnava
Copy link
Member

stnava commented Mar 30, 2022 via email

@stnava
Copy link
Member

stnava commented Mar 31, 2022

I like this PR and would be happy to merge when you are ready.

a few questions:

  • can you enable this only for tags ( as you mentioned )
  • is it possible to push the artifacts to gh releases? pypi can't store many of these .... not sure if gh has limits like pypi
  • what do you think is the likelihood of getting the windows build working?

thx again.

@tfmoraes
Copy link
Contributor Author

@stnava it's already building on Windows, but it's needed to fix this problem that makes registration not work. My C++ level is a little low, but I can try to help.

Yes, it's possible to push to GH release. Some projects, like Neovim, use GH actions to generate nightly build https://github.com/neovim/neovim/blob/master/.github/workflows/release.yml

@stnava
Copy link
Member

stnava commented Apr 2, 2022

ok - I will merge and we can put the windows fix on the (very long) to do list.

@stnava stnava merged commit 309f274 into ANTsX:master Apr 2, 2022
@tfmoraes
Copy link
Contributor Author

tfmoraes commented Apr 4, 2022

@stnava if you give some directions, I can try to fix this problem with WIndows.

@stnava
Copy link
Member

stnava commented Apr 4, 2022

unfortunately, I don't know much about windows development nor do I have a windows machine --- I know there are some ppl out there who would appreciate these efforts and could help but not sure who.

the short story is that the pointer to the image is getting lost somewhere, likely either at the read or in passing the read pointer to downstream functions - it's probably fairly easy to isolate. would involve developing a minimally reproducible example ... or at least finding a simpler example that fails - ants.registration is a bit unwieldy to debug.

@cookpa
Copy link
Member

cookpa commented Oct 3, 2022

The Windows build had known problems, see for example #345. I'm not sure if those got fixed.

If we have working builds for Windows it might be possible to ask PyPi for more space, or we could look into hosting the other artifacts as part of the Github release.

@tfmoraes tfmoraes mentioned this pull request Jan 11, 2023
Closed
@dipterix
Copy link

Sorry to bother everyone in this PR... @tfmoraes I wrote a patch in pure python here that works on Windows. Just tested : )
https://github.com/dipterix/rpyANTs/blob/main/inst/patches/win_patch.py

Please see my diagnosis here: #429

Basically the script works as "injection" that replaces ants.utils.get_pointer_string and ants.utils._int_antsProcessArguments. Instead of passing memory pointers to C++, we save the ANTsImage to temporary nifti file and pass that temporary path. I have tested both locally and on Github action and this method works fine.

The script was designed for my R package so there are some "weird" stuff. However, I believe you will have better solution to this problem. If you want to use this method, please remember to delete the temporary file once the C++ returns.

Al so thanks for compiling ANTsPy on windows. It works like a charm ;P

@cookpa cookpa mentioned this pull request Mar 16, 2023
Closed
@cookpa cookpa mentioned this pull request Jun 9, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tfmoraes @stnava @cookpa @dipterix