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

Conda build #56

Merged
merged 7 commits into from
Aug 12, 2022
Merged

Conda build #56

merged 7 commits into from
Aug 12, 2022

Conversation

hansenms
Copy link
Member

@hansenms hansenms commented Aug 9, 2022

This PR adds a conda build pipeline, which will publish (on CI) to the ismrmrd anaconda channel.

It turns out that none of the tests (in tests/) were ever run and a few of them were broken (related to metadata in ismrmrd images, which has to be XML). In order to get some testing done for the conda package, the tests have been fixed and they are run (with nosetests) in the test stage of conda build.

The Azure DevOps pipeline was also removed, since it is not used.

@hansenms hansenms requested a review from johnstairs August 10, 2022 00:27
@hansenms
Copy link
Member Author

@roopchansinghv can I get you to have a look at this. If you look at the conda/run_test.sh script, it only runs the nosetests when the platform is Linux. For some reason, it is failing on the mac, but I don't have a mac for testing. It would be really great if you could take a look.

conda/package.sh Outdated Show resolved Hide resolved
@roopchansinghv
Copy link
Contributor

I did not have 'nose' installed by default in my Gadgetron conda environment, but once I did, I got:

(gadgetron-macos) [0] :~/my_root/src/ismrmrd-python/tests 11:07:04 % git status
On branch hansenms/conda-build
Your branch is up to date with 'origin/hansenms/conda-build'.
(gadgetron-macos) [0] :~/my_root/src/ismrmrd-python/tests 11:07:14 % nosetests
~/my_root/src/ismrmrd-python/tests/test_acquisition.py:172: SyntaxWarning: "is not" with a literal. Did you mean "!="?
  assert acquisition.version is not 0, \
............................~/my_root/anaconda3/envs/gadgetron-macos/lib/python3.9/site-packages/ismrmrd/image.py:189: PendingDeprecationWarning: The default behavior of this function is currently column-major which is inconsistent with numpy using row-major by default. In a future version this will be changed. Please switch to setting transpose in this function to false to switch to the new behavior.
  warnings.warn(
..............~/my_root/anaconda3/envs/gadgetron-macos/lib/python3.9/site-packages/ismrmrd/image.py:189: PendingDeprecationWarning: The default behavior of this function is currently column-major which is inconsistent with numpy using row-major by default. In a future version this will be changed. Please switch to setting transpose in this function to false to switch to the new behavior.
  warnings.warn(
...~/my_root/src/ismrmrd-python/tests/test_image.py:70: SyntaxWarning: "is not" with a literal. Did you mean "!="?
  assert image.version is not 0, \
~/my_root/src/ismrmrd-python/tests/test_image.py:137: SyntaxWarning: "is" with a literal. Did you mean "=="?
  assert image.channels is 1, \
~/my_root/src/ismrmrd-python/tests/test_image.py:153: SyntaxWarning: "is" with a literal. Did you mean "=="?
  assert image.channels is 1, \
~/my_root/src/ismrmrd-python/tests/test_image.py:169: SyntaxWarning: "is" with a literal. Did you mean "=="?
  assert image.channels is 16, \
..................~/my_root/src/ismrmrd-python/tests/test_waveform.py:34: SyntaxWarning: "is not" with a literal. Did you mean "!="?
  assert waveform.version is not 0, \
.........
----------------------------------------------------------------------
Ran 72 tests in 16.791s

OK

Lots of warnings - but it looked like things passed.

I'll take a swing at starting with a completely bare environment for this, and see what that gets me.

@hansenms
Copy link
Member Author

@roopchansinghv could you try on my branch to disable the check for Linux in run_test.sh and then execute the package.sh script to see if it builds and runs the tests. On the mac os build machines, the build succeeds, but the module fails to load in the test environment. It is pretty much impossible to debug for me without a mac.

@roopchansinghv
Copy link
Contributor

So when I ran package.sh on macOS with the check for 'Linux' disabled in run_test.sh, I am getting:

======================================================================
ERROR: Failure: ModuleNotFoundError (No module named 'ismrmrd')
----------------------------------------------------------------------

and a bunch of python traceback errors around it.

So it seems the machinery here is not actually installing the conda package on macOS. Am I understanding that correctly, or am I missing something basic?

@hansenms
Copy link
Member Author

@roopchansinghv that is the error that I get too on the build agent. I don't know if this means that the package itself is no good or if it is "just" the build machinery. I was hoping you could maybe help me debug. Could you:

  1. Use the package script to produce a package.
  2. Create a new conda environment where you install the runtime dependencies (listed in the meta.yml) file.
  3. Install the package you created (there should be a tar.bz2 file that you can just conda install).
  4. Run python and import ismrmrd and see if it works

If it works we can maybe conclude that it is the conda build stuff on mac that has some issue and we can leave the Linux check in there if it also fails through the manual process we might need to dig in more.

Either way, your help would really be appreciated here.

@roopchansinghv
Copy link
Contributor

Apologies this is taking me a while to debug. I am on site this week, so besides actually dealing with matters on the ground here, there are some shenanigans between NIH and outside sites' certificates that I need to work around ...

From what I've been able to tell this morning, I don't think the conda package is even successfully built, as I am unable find it anywhere, either in the ismrmrd-python git repo folder or sub-folders, or in the build cache for my conda environment.

@roopchansinghv
Copy link
Contributor

So a bit more headway ...

During the conda build process, a ismrmrd-python-1.12.0-0.tar.bz2 file does get created in conda/build_pkg, and persists there till the tests start running. When the tests start, the conda package disappears, and the tests fail soon after.

However, if I grab a copy of ismrmrd-python-1.12.0-0.tar.bz2, create an environment based on the contents of conda/meta.yml, the tests still fail with the same error as above, i.e. ERROR: Failure: ModuleNotFoundError (No module named 'ismrmrd')

Turns out, the .egg file in the .bz2 conda package is just copied into ${CONDA_PREFIX}/lib/python3.10/site-packages and not properly unzipped. Once I unzipped the .egg file there, an ismrmrd folder was created. Then, I could run the tests properly, and launching python, then running import ismrmrd worked ...

Seems there are a couple of things to deconstruct here, but I'll try to figure out what's happening on macOS here.

@hansenms
Copy link
Member Author

@roopchansinghv thanks so much for diffing in. What is your take here, should we a) wait for you to dig a bit more, b) merge as is with later follow-up fix for mac os, or c) disable macos for now to start shipping a package and then get macos package later?

@roopchansinghv
Copy link
Contributor

roopchansinghv commented Aug 12, 2022

I would recommend omitting macos-latest from the buiild-matrix strategy for now, and also remove the check for "Linux" in run_test.sh.

In everything I've been able to review so far, the issue is either the conda package build itself, or installing the built package. But when I work through these manually, and run the tests on my Mac, they pass and run fine.

Therefore, I would commit changes right now that would "just" need the macos-latest entry to be added again to the build strategy matrix. Of course, there might be a couple of other tweaks to get everything working, but I am hoping those will be relatively minor.

When the conda build and test chain fails, the created (and broken) packages go into ${CONDA_PREFIX}/conda-bld/broken - FYI

@hansenms hansenms merged commit 2cdf828 into master Aug 12, 2022
@hansenms hansenms deleted the hansenms/conda-build branch August 12, 2022 19:02
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 this pull request may close these issues.

3 participants