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

Include openblas as a runtime dependency on osx and linux. #62

Closed
wants to merge 3 commits into from

Conversation

jdblischak
Copy link
Member

Checklist

  • Used a fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

My attempt to fix the openblas issue for R recipes on Travis described in conda-forge/conda-forge.github.io#682

From @isuruf:

You'll have to add openblas as a run requirement to toolchain builds of r-base.

Also, note that problems linking to openblas are currently described in meta.yaml:

  # The current openblas-devel run exports
  # are overly restrictive IMHO, should be
  # open-ended, but actually because we
  # need to copy the shared library into this
  # package and change its SONAME to libRblas.so
  # we do not want a run dependency on openblas
  # at all (otherwise Matrix.so ends up with a
  # DT_NEEDED of libopenblas.so and cannot be used
  # against any old libRblas.so)
  ignore_run_exports:
    - libopenblas

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@mingwandroid
Copy link
Contributor

mingwandroid commented Dec 7, 2018

Please don't do this. We vendor openblas libraries deliberately instead so that defaults R is not incompatible with defaults numpy (via MKL).

Something else is wrong here. Please compare with defaults recipe.

Ping @msarahan, @mcg1969, @ocefpaf

@mingwandroid
Copy link
Contributor

My comment does not describe a problem. It explains why we vendor these libraries.

@mcg1969
Copy link

mcg1969 commented Dec 7, 2018

But there's nothing wrong with openblas and MKL living in the same directory, as long as a given ecosystem only links to one of them. The real mistake is to use the same variant metapackage for R and Python—that would force them to use the same BLAS whether you wanted to or not.

@isuruf
Copy link
Member

isuruf commented Dec 7, 2018

Issue is that R links to gsl and conda-forge gsl comes with openblas which makes R link to openblas as well. But conda solver likes to get r-base from conda-forge and gsl from defaults

@mingwandroid
Copy link
Contributor

libopenblas depends upon the mutex package afaik. Regardless since we vendor the libs and rename the SONAME this is at best pointless.

@mcg1969
Copy link

mcg1969 commented Dec 7, 2018

Here's a link to @mingwandroid's recipe for r-base. It's a meaty one!

https://github.com/AnacondaRecipes/r-base-feedstock/tree/master/recipe

@mingwandroid
Copy link
Contributor

mingwandroid commented Dec 7, 2018

Issue is that R links to gsl and conda-forge gsl comes with openblas which makes R link to openblas as well. But conda solver likes to get r-base from conda-forge and gsl from defaults

Thanks @isuruf, in this case we should either force the build to use defaults gsl or else remove the openblas headers so it doesn't get picked up. We really must be compatible with such an important package if at all possible.

@mingwandroid
Copy link
Contributor

Here's a link to @mingwandroid's recipe for r-base. It's a meaty one!

We've synced the recipes now, more or less! Just these tricky things to iron out..

@isuruf
Copy link
Member

isuruf commented Dec 7, 2018

@jdblischak, you can try changing GSL_CBLAS_LIB=-lgslcblas to GSL_CBLAS_LIB= in gsl.pc so that R doesn't link to openblas.

@mcg1969, those hacks are not necessary if you consider conda-forge/blas-feedstock#15. Please comment there.

@mcg1969
Copy link

mcg1969 commented Dec 7, 2018

Oh I'm deferring to @mingwandroid on the library-related matters.

@mingwandroid
Copy link
Contributor

mingwandroid commented Dec 7, 2018

Indeed @isuruf's new scheme seems great to me here. My only concern is around the index size (32 vs 64 bits) of different blas implementations.

The other reason I did it this way is to allow swapping back to R's default blas implementation trivially for benchmarking and correctness verification.

@isuruf
Copy link
Member

isuruf commented Dec 7, 2018

The other reason I did it this way is to allow swapping back to R's default blas implementation trivially for benchmarking and correctness verification.

I don't know anything about R's default blas implementation. Is that a separate library? If so, we can have a 5th implementation (apart from netlib, openblas, mkl, blis)

@mingwandroid
Copy link
Contributor

mingwandroid commented Dec 7, 2018

For the blas stuff many of us have overlapping experience, both on the AD team and also conda-forge people. These big changes are always tricky though, but I figure we should probably get the compiler migration out of the way before deciding what to do with blas.

FYI, we also have trouble with openblas not being fork-safe because libgomp is not fork safe which forced defaults to build openblas without libgomp and are considering fairly radical solutions to that, including using clang/llvm-openmp on Linux instead of GCC/libgomp.

Before we panic and give ourselves a load more work to do I'd like to know the performance impact of not using libgomp with openblas.

@isuruf
Copy link
Member

isuruf commented Dec 7, 2018

FYI, we also have trouble with openblas not being fork-safe because libgomp is not fork safe which forced defaults to build openblas without libgomp and are considering fairly radical solutions to that, including using clang/llvm-openmp on Linux instead of GCC/libgomp.

I don't think that's radical. libgomp, libomp and libiomp all should be ABI compatible. (We can do the same thing as the blas variant I proposed). In any case, it's not relevant to this PR and let's not discuss it here.

@jdblischak
Copy link
Member Author

you can try changing GSL_CBLAS_LIB=-lgslcblas to GSL_CBLAS_LIB= in gsl.pc so that R doesn't link to openblas.

@isuruf Where is GSL_CBLAS_LIB defined? I couldn't find when I searched this feedstock or the R source code. Are you suggesting I submit a patch to the gsl-feedstock?

@mingwandroid
Copy link
Contributor

I don't know anything about R's default blas implementation. Is that a separate library? If so, we can have a 5th implementation (apart from netlib, openblas, mkl, blis)

Yes it's built as separate DSOs then moved aside. You could make it a fifth implemention if you were to fudge the SONAME. We'd like to avoid the mutex from preventing MKL and rblas being installed at the same time. With your new scheme would we need the opposite of a mutex? My head is fairly muddled at present so apologies if I'm raising dumb concerns!

Anyway. I think in the short term avoiding an openblas dep would be ideal if conda-forge are willing?

@isuruf
Copy link
Member

isuruf commented Dec 7, 2018

@jdblischak, it's defined in $PREFIX/lib/pkgconfig/gsl.pc file. For now, let's change that in this feedstock during build.

@mingwandroid
Copy link
Contributor

Does gsl still not autoload its blas implementation? I remember it was done at runtime to deliberately allow different implementations, in which case modifying the .pc is the right thing to do regardless of this discussion.

@mingwandroid
Copy link
Contributor

This can be checked with readelf -d of course. I'm banned from using computers for a while!

@jdblischak
Copy link
Member Author

Anyway. I think in the short term avoiding an openblas dep would be ideal if conda-forge are willing?

It's unclear to me what all the options are. Currently there are multiple conda-forge R recipes that can't build on for macOS on Travis, and likely to be more. Options include:

  1. Add openblas as a runtime dependency to r-base
  2. Add openblas as a runtime dependency on macOS for individual R package recipes
  3. Some other trick to get R and openblas to work correctly?

@mingwandroid
Copy link
Contributor

Depending upon llvm-openblas on macOS only is what I've been doing so far.

@jdblischak
Copy link
Member Author

Depending upon llvm-openblas on macOS only is what I've been doing so far.

@mingwandroid Thanks for the suggestion! Unfortunately I don't understand openblas well enough to understand how to implement it. Are you suggesting a change to the r-base recipe or individual R package recipes? How do I depend on llvm-openblas? I couldn't find a package by that name on Anaconda Cloud. I'm trying to avoid adding openblas as a runtime dependency to individual R recipes, but I don't have a clear alternative to implement.

@mingwandroid
Copy link
Contributor

Sorry that's my muddled head. I meant llvm-openmp. I've never needed to link to openblas directly. No R package should either since that's implemented via r-base.

I think if you fix gsl.pc as @isuruf suggested (and maybe also remove some the blas headers before running configure) then all should be good.

@isuruf isuruf mentioned this pull request Dec 8, 2018
@isuruf isuruf closed this in #63 Dec 8, 2018
@mingwandroid
Copy link
Contributor

Thank you @jdblischak and @isuruf!

dbast added a commit to dbast/staged-recipes that referenced this pull request Dec 8, 2018
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.

5 participants