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

Openblas detection and test #173

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Openblas detection and test #173

wants to merge 1 commit into from

Conversation

miroi
Copy link
Contributor

@miroi miroi commented May 30, 2016

let us see if the CI test will pass...

@miroi
Copy link
Contributor Author

miroi commented May 31, 2016

current problem is that libopenblas packages are not behaving well on testing virtual servers, have to find out more ...

@miroi
Copy link
Contributor Author

miroi commented May 31, 2016

@miroi
Copy link
Contributor Author

miroi commented May 31, 2016

ah, I see no openblas package for osx, suggesting to skip this test for OSX

@miroi miroi closed this May 31, 2016
@miroi miroi reopened this May 31, 2016
@miroi miroi closed this May 31, 2016
@miroi miroi reopened this Jun 2, 2016
@miroi miroi force-pushed the openblas branch 4 times, most recently from fcf1482 to a5bbc59 Compare June 3, 2016 11:11
@miroi
Copy link
Contributor Author

miroi commented Jun 3, 2016

Finally I got tests green ! Ready for merge.

@miroi
Copy link
Contributor Author

miroi commented Jun 3, 2016

well, the 'bloody' part of this change was playing with travis-ci configurations, to see, which packages go well with the 'offending 'libopenblas package.

@bast
Copy link
Member

bast commented Jun 4, 2016

Thanks! However, I don't think the code duplication with the separate test script is necessary and good. Either the code needs to be generalized back into one file or the code duplication be abstracted into a module. We definitely want to avoid code repetition. D.R.Y.

@miroi
Copy link
Contributor Author

miroi commented Jun 5, 2016

what is "D.R.Y." ?

ah, I found it , it means don't repeat yourself (DRY)

- thanks to
  https://github.com/BVLC/caffe/blob/master/cmake/Modules/FindOpenBLAS.cmake
- added dedicated openblas test, which has the same testing source the same as for the blas test
  (fc_openblas in test_openblas.py)
- taken care of ENABLE_OPENBLAS variable
- provide openblas package installation for .travis.yml in 2 extra
  platforms (thanks to Dominic Jodoin <[email protected]>)
- closes dev-cafe#172
- .travis.yml : openblas and blas can be together, but not with
  lapack,atlas packages !
  and atlas !
- test/test_openblas.py : test both blas and openblas in new test
- test/test.py : test all excep openblas
- reduced number of installed packages for blas+openblas to get
  travis-ci test passed
- fc_blas unified with fc_openblas, no need to duplicate the code
- WARNING: testing on travis-ci is shaky, sometimes is passes, sometimes
  not
@bast
Copy link
Member

bast commented Jun 15, 2016

I cannot integrate this as a PR. There is code repetition (test_openblas.py contains code that is repeated in test.py). Also we cannot just copy-paste CMake code from other projects without proper attribution. I will pull this into a branch and clean it up there. Also tests fail and I don't know why. Need to look at it.

@miroi
Copy link
Contributor Author

miroi commented Jun 15, 2016

hi, at least please try to restart tests, and see if they pass;

ad: test_openblas.py - I need another mixture of packages, this is how they advised it from travis

sorry for not putting proper reference to the module's source; but can we modify it ?

@bast
Copy link
Member

bast commented Jun 15, 2016

We can modify the source (BSD license) but we must attribute/acknowledge the source. Also I have restarted tests and they still fail for some mysterious reason. I will pull this into my fork and then close the PR and clean it up there. But thanks for the work on this.

@miroi
Copy link
Contributor Author

miroi commented Jun 15, 2016

Hi, libopenblas package somehow hurts composition of packages, if you get rid of it, tests should pass. But I would try some newest linux platform in travis-ci to see what openblas hurts/does not hurt.

@bast
Copy link
Member

bast commented Jul 11, 2016

I have cleaned up your code on the openblas branch in the central repo
but the OpenBLAS test still fails: https://travis-ci.org/coderefinery/autocmake/builds/143905558.
Can you please have a look why it fails? Otherwise I cannot integrate it.

@bast
Copy link
Member

bast commented Jul 11, 2016

If the code works on your computer but fails on Travis for some reason that we cannot solve, we could also get rid of the test. The code will get tested by the community and if there are issues, we react.

@miroi
Copy link
Contributor Author

miroi commented Jul 11, 2016

It seems that travis-ci is not able to set up healthy virtual testing servers;

here it hangs on (long) boost test

To cope with this I would set up another testing server, let say circle-ci, and see.

test/test.py::test_boost_libs 
No output has been received in the last 10 minutes, this potentially indicates a stalled build or something wrong with the build itself.
The build has been terminated

@bast
Copy link
Member

bast commented Jul 11, 2016

I am happy with Travis and do not want to add another CI service since this will increase complexity and IMO add little benefit. If the Boost test turns out to be a problem, we can always remove the test.

@miroi
Copy link
Contributor Author

miroi commented Dec 11, 2016

Well, openblas test

https://travis-ci.org/coderefinery/autocmake/builds/135607059

depends how travis-ci 'pre-cooks' the virtual server with the openblas package:

https://github.com/coderefinery/autocmake/blob/openblas/.travis.yml#L70

This is the case where we don't have control on how non-native packages will behave....

@miroi
Copy link
Contributor Author

miroi commented Dec 12, 2016

Xianyi is suggesting building openblas from the source,

https://groups.google.com/d/msg/openblas-dev/pnQX-lHf6Wc/UfLG74ePDgAJ

would you, Radovan, agree with this solution ? No more openblas packages, but rather build OpenBLAS from the scratch - from the source.

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.

2 participants