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

add pyopenms #18522

Merged
merged 23 commits into from
Nov 28, 2019
Merged

add pyopenms #18522

merged 23 commits into from
Nov 28, 2019

Conversation

Leon-Bichmann
Copy link
Contributor

@Leon-Bichmann Leon-Bichmann commented Nov 7, 2019

Here we want to add a new conda package pyopenms, which builds the python bindings of the already available package openms.

Please review and merge if the tests pass.

Thanks!

  • I have read the guidelines for bioconda recipes.
  • This PR adds a new recipe.
  • AFAIK, this recipe is directly relevant to the biological sciences (otherwise, please submit to the more general purpose conda-forge channel).
  • This PR updates an existing recipe.
  • This PR does something else (explain below).

@timosachsenberg
Copy link
Contributor

timosachsenberg commented Nov 11, 2019

minor note: please adapt:

extra:
  identifiers:
    - biotools:openms
    - doi:10.1038/nmeth.3959

and add pyopenms nose test?

@bgruening
Copy link
Member

Shouldn't this be a sub-package of the main OpenMS recipe?

@timosachsenberg
Copy link
Contributor

Hi Björn, Thanks for chipping in. Good question. So far we kept the projects separate and we probably need to look into what this would imply. AFAIK in our pip installer, we left the tools out and only compiled the library for use with pyopenms. Is it possible to do it as two recipes now and potentially move to the sub-package later?
Or should we figure this out now?

@bgruening
Copy link
Member

Or should we figure this out now?

We can postpone this to later :)

@timosachsenberg
Copy link
Contributor

@Leon-Bichmann please check that all dependencies are installed.
see e.g. the error message in the CI:
Looking for numpy - not found

@timosachsenberg
Copy link
Contributor

there seem to be an issue with boost now.
was changing boost to boost-cpp correct?

@bgruening
Copy link
Member

boost has the boost-python stuff, boost-cpp is realy just the CPP part of boost.

@dpryan79
Copy link
Contributor

I think we're going to need to take a couple different tacks with this recipe. Firstly, we should just depend on openms, so we don't need to recompile the whole thing. We still need to run cmake, though, since otherwise env.py is never created in the python source code directory. After that, we should presumably be able to simply run pip install from within the python source code and have it work...or at least that's what perusing the code suggests. The python script and CMake files incorrectly force C++11 and some gnu flags that will likely cause problems with recent boost versions, but I assume those can be patched around. I'm attempting a variant of this locally and I'll see if it helps at all.

@dpryan79
Copy link
Contributor

Actually, I think it'll be easier to just manually make env.py than to use cmake, since otherwise we really need all of openms' dependencies.

@Leon-Bichmann
Copy link
Contributor Author

It would be great to get this into bioconda in order to include more recipes based on pyopenms #18701

@timosachsenberg
Copy link
Contributor

@hroest Leon and I are a bit lost here and would like to hear what you think would be the best way forward?
I am also not sure about " The python script and CMake files incorrectly force C++11 and some gnu flags that will likely cause problems with recent boost versions"
OpenMS (and also compiling the autowrapped C++ classes) requires C++11 but I guess this comment refers to the python only part?

@dpryan79
Copy link
Contributor

Most of our recipes are now build with C++17, so trying to compile with C++11 will have issues going forward, since it won't work with newer boost versions. I've gotten pyopenms to at least build now without needing to compile all of openms at the same time. There's a fair bit of hackiness involved in that, but it builds in half the time and at least partially passes testing (I'm not sure about the mulled testing yet).

@hroest
Copy link

hroest commented Nov 19, 2019

@hroest Leon and I are a bit lost here and would like to hear what you think would be the best way forward?

I think it is possible to depend on OpenMS but it is not the way we usually do our config and build since in the default config PYOPENMS flag is not enabled. For integration with our build system it would be easier to start from scratch. I think it would be easier to start from a build where OpenMS was built, run cmake again with the PYOPENMS option and let the build system do its work. I suggest to start from a openms build environment, rerun cmake and then do make pyopenms.

However, once the env.py is created and the files copied and in the right place, you should be able to run create_cpp_extension.py and then setup.py build if you really want to do that. I would not recommend this since if we need to make any change in our CMake files, you will have to replicate those manually in your solution.

I am also not sure about " The python script and CMake files incorrectly force C++11 and some gnu flags that will likely cause problems with recent boost versions"

as soon as you include OpenMS header files, you should enable C++11 flags

Most of our recipes are now build with C++17, so trying to compile with C++11 will have issues going forward, since it won't work with newer boost versions.

Which boost versions require C++17? Also this does not seem to be a pyOpenMS issue but an OpenMS issue in general - you can try and compile with C++17 and it may work out of the box, we have not tested it. If we get OpenMS to compile with C++17 then pyopenms should also work.

I've gotten pyopenms to at least build now without needing to compile all of openms at the same time. There's a fair bit of hackiness involved in that, but it builds in half the time and at least partially passes testing (I'm not sure about the mulled testing yet).

sounds good, see my concerns above about manually hacking the build system. Why is it so important to depend on openms instead of starting from scratch or from the same build environment?

@dpryan79
Copy link
Contributor

While boost doesn't explicitly require C++17, the recent (at least 1.70.0) packages on conda-forge have been compiled that way so you'll have issues getting the symbols correct if you force C++11. My only real concern with rebuilding openms is that anyone trying to install both openms and pyopenms in the same environment will then run into problems due to file collisions or slightly different requirements due to the packages getting built at somewhat different times. Explicitly depending on openms resolves that issue.

recipes/pyopenms/meta.yaml Outdated Show resolved Hide resolved
@dpryan79
Copy link
Contributor

@bioconda-bot please update

@dpryan79
Copy link
Contributor

At the very least it's building on Linux now, so there will be a test package.

@dpryan79
Copy link
Contributor

@bioconda-bot please fetch artifacts

@BiocondaBot
Copy link
Collaborator

Package(s) built on CircleCI are ready for inspection:

Arch Package Repodata
linux-64 pyopenms-2.4.0-py27h9de70de_0.tar.bz2 repodata.json

You may also use conda to install these:

  • For packages on linux-64:
conda install -c https://84302-42372094-gh.circle-artifacts.com/0/tmp/artifacts/packages <package name>

Docker image(s) built:

Package Tag Install with docker
pyopenms 2.4.0--py27h9de70de_0
showcurl "https://84302-42372094-gh.circle-artifacts.com/0/tmp/artifacts/images/pyopenms%3A2.4.0--py27h9de70de_0.tar.gz" | gzip -dc | docker load

@dpryan79
Copy link
Contributor

Can someone ensure the linux package works? We can then just skip the OSX build for now.

@Leon-Bichmann
Copy link
Contributor Author

@dpryan79 thx for getting this forward!

I'll give the unix package a try.

@Leon-Bichmann
Copy link
Contributor Author

ok nice, everything seems to go fine and it works for me on unix

@dpryan79
Copy link
Contributor

dpryan79 commented Nov 28, 2019

Shall we skip OSX for now and merge this in?

BTW, the errors on OSX are:

06:45:16 BIOCONDA INFO (ERR)     pyopenms/pyopenms_6.cpp:191428:13: error: no viable overloaded '='
06:45:16 BIOCONDA INFO (ERR)       __pyx_t_1 = __pyx_v_self->inst.get()->total_abundances;
06:45:16 BIOCONDA INFO (ERR)       ~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
06:45:16 BIOCONDA INFO (ERR)     /Users/distiller/project/miniconda/conda-bld/pyopenms_1574863788533/_build_env/bin/../include/c++/v1/map:911:10: note: candidate function not viable: no known conversion from 'map<UInt64, [...], less<unsigned long long>, allocator<pair<const unsigned long long, [...]>>>' to 'const map<unsigned long, [...], less<unsigned long>, allocator<pair<const unsigned long, [...]>>>' for 1st argument
06:45:16 BIOCONDA INFO (ERR)         map& operator=(const map& __m)
06:45:16 BIOCONDA INFO (ERR)              ^
06:45:16 BIOCONDA INFO (ERR)     /Users/distiller/project/miniconda/conda-bld/pyopenms_1574863788533/_build_env/bin/../include/c++/v1/map:938:10: note: candidate function not viable: no known conversion from 'map<UInt64, [...], less<unsigned long long>, allocator<pair<const unsigned long long, [...]>>>' to 'map<unsigned long, [...], less<unsigned long>, allocator<pair<const unsigned long, [...]>>>' for 1st argument
06:45:16 BIOCONDA INFO (ERR)         map& operator=(map&& __m)
06:45:16 BIOCONDA INFO (ERR)              ^
06:45:16 BIOCONDA INFO (ERR)     /Users/distiller/project/miniconda/conda-bld/pyopenms_1574863788533/_build_env/bin/../include/c++/v1/map:970:10: note: candidate function not viable: no known conversion from 'SampleAbundances' (aka 'map<unsigned long long, double>') to 'initializer_list<value_type>' (aka 'initializer_list<pair<const unsigned long, double> >') for 1st argument
06:45:16 BIOCONDA INFO (ERR)         map& operator=(initializer_list<value_type> __il)
06:45:16 BIOCONDA INFO (ERR)              ^

That strikes me as a cython bug on OSX.

@Leon-Bichmann
Copy link
Contributor Author

From my side we could skip OSX.

@Leon-Bichmann
Copy link
Contributor Author

Is there a way to check whether cython versions are the same for unix and OSX ?

Maybe that was the issue here.

@dpryan79
Copy link
Contributor

Looking at the build logs it was 0.29.14 in both cases.

@dpryan79
Copy link
Contributor

@bioconda-bot please merge

@BiocondaBot
Copy link
Collaborator

I will attempt to upload artifacts and merge this PR. This may take some time, please have patience.

@BiocondaBot BiocondaBot merged commit d9e220d into bioconda:master Nov 28, 2019
@jpfeuffer
Copy link
Contributor

Concerning the macOS error, for potential future bug-fix PRs I have a question @dpryan79 :
How is your build.sh supposed to obtain the config.h file from the OpenMS build tree which has to be usually configured by CMake. It defines macros for things like type sizes (which are a possible reason for the failure).
Was it maybe cached from earlier runs? And maybe cached wrongly on mac? Just some ideas.

@dpryan79
Copy link
Contributor

dpryan79 commented Dec 3, 2019

Presumably there was already a config.h in the source tree that got used. If it was inappropriate for OSX then that'd certainly cause issues, though type sizes should be mostly the same between Linux and OSX these days since they're the same architecture.

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.

7 participants