Unify #71

casperdcl · 2017-12-11T14:05:12Z

continued from #69:

sync with SIRF-SuperBuild:system-build
- sort out (python) tests
  - use $(SIRF_)PYTHON_EXECUTABLE adding PYTHON_EXECUTABLE to env_ccppetmr.*? SIRF-SuperBuild#60
  - closes Test Updates #12 (closed by System build SIRF-SuperBuild#38)
  - mac test issues (PyThreadState_Get)
- ~~fix USE_SYSTEM_HDF5=OFF~~
  - merge HDF5_DIR_fixes branch SIRF-SuperBuild#44
- sort out the mac build issues on travis (python/pip)
- ccache
  - build time (make) ~16min -> under 1min (:1st_place_medal: min)
  - Support settings multiple languges in one travis ci file travis-ci/travis-ci#4090
  - travis improvements SIRF-SuperBuild#17

closes SyneRBI/SIRF-SuperBuild#39

Ambitious

useSIRF-SuperBuild/docker/*.sh scripts
- SIRF/.travis.yml
- SIRF-SuperBuild/.travis.yml
- VM

codecov · 2017-12-11T14:38:57Z

Codecov Report

Merging #71 into master will increase coverage by 1.65%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master      #71      +/-   ##
==========================================
+ Coverage   55.51%   57.17%   +1.65%     
==========================================
  Files           2        2              
  Lines        1632     1548      -84     
==========================================
- Hits          906      885      -21     
+ Misses        726      663      -63

Impacted Files	Coverage Δ
src/xGadgetron/pGadgetron/pGadgetron.py	`49.87% <0%> (+0.17%)`	⬆️
src/xSTIR/pSTIR/pSTIR.py	`65.64% <0%> (+4%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 950078c...c25c73c. Read the comment docs.

casperdcl · 2017-12-12T09:33:34Z

yay it builds -.-

casperdcl · 2017-12-12T10:08:47Z

ok sarcasm aside, that ccache really helps. under 4min per job, most of which is not compiling.

casperdcl · 2017-12-12T21:10:06Z

sigh, waiting for https://www.traviscistatus.com/incidents/j772csl6n49j

KrisThielemans · 2017-12-13T06:53:11Z

great about ccache, I tried to do this in the CMake-recommended way SyneRBI/SIRF-SuperBuild#58 but this didn't seem to help. not sure why. I haven't tested that PR outside of Travis. comments welcome there.

KrisThielemans · 2017-12-13T06:55:45Z

Before you do lots more work on the actual "unification" can we discuss the plan? I'm not sure how it fits with what we had in mind for the VM, docker etc. Probably no time on Friday. next tcon?

casperdcl · 2017-12-13T12:34:32Z

@KrisThielemans turns out it was just travis lying, still 20min/build. Odd, since I get a much quicker build when I use ccache locally. It might be because of language as you said. travis-ci/travis-ci#4090

regarding python/pip (esp. on Mac): travis-ci/travis-ci#2312, pypa/pip#3813, pypa/pip#1668

KrisThielemans · 2017-12-17T10:13:02Z

sounds like another python version mixup.

casperdcl · 2017-12-17T12:48:37Z

still failing on mac though, even with tests using ${PYTHON_EXECUTABLE}

KrisThielemans · 2017-12-17T14:48:46Z

yes. In the log we see the conflict:

-- Found PythonInterp: /usr/local/bin/python2 (found suitable version "2.7.14", minimum required is "2") 
-- Found PYTHON_EXECUTABLE=/usr/local/bin/python2
-- Python version 2.7.14
-- Found PythonLibs: /System/Library/Frameworks/Python.framework/Versions/2.7/lib/libpython2.7.dylib (found suitable version "2.7.10", minimum required is "2")

we currently use

-DPYTHON_LIBRARY=$(python-config --prefix)/lib/libpython2.7.dylib -DPYTHON_INCLUDE_DIR=$(python-config --prefix)/include/python2.7"

but that isn't good (enough). According to the CMake doc, we need to do

find_package(PythonInterp)
find_package(PythonLibs)

which we do here. But then overriding the above variables is probably confusing. Might be better to override PYTHON_EXECUTABLE.

casperdcl · 2017-12-17T15:26:47Z

ah yes, but why do we override this on osx in the first place? can we not just rely on find_package like we do with linux?

KrisThielemans · 2017-12-17T22:23:45Z

the internet is full of reports by people struggling with CMake and Python on OSX. We got it to work with that particular combination, but that was because we didn't test the python executables from within CMake (and were (un)lucky to pick up the matching python when running the test from the script).

Possibly @bathomas and @rijobro can comment. They recently struggled quite a lot as well.

casperdcl · 2017-12-18T14:50:25Z

KrisThielemans · 2017-12-18T22:53:39Z

.travis.yml

+   - libfftw3-dev
+   - python-dev
+   - python3-dev
+   - python-tk


why include this? We don't need it as far as I know

we definitely need -dev and -tk, arguably not for both python and python3 (depending on the build matrix) but

it's neater to put both here rather than require sudo to apt-get the appropriate version

it's a quick download/install

it doesn't affect osx

What about the other dependencies, such as root-system-bin? Are they really all required? I just copied from the SuperBuild.

agree to install both python2 and 3 here. still mystified about -tk as I've never had to install this before (maybe it was there).

yes, get rid of root-system-bin. Could be used to enable ROOT support for STIR, but then it'd need other packages as well (and is sure to let to other compilation problems, so we won't do this now).

ok, building without root-system-bin now. python-tk is needed (https://travis-ci.org/CCPPETMR/SIRF/jobs/315938695) and would have been installed back when we had language: python. We need language: cpp now in order to get ccache to work (which is evidently much more important than caching pip)

let me know if you think anything else can be removed. it's about 1:30min to apt-get things at the moment.

KrisThielemans · 2017-12-18T22:57:16Z

.travis.yml

+ - popd
+ # get pip
+ - curl -0 https://bootstrap.pypa.io/get-pip.py -o get-pip.py
+ - python$PYMVER get-pip.py --user


no guarantee that this python will be the one found by cmake. would have to enforce it in the cmake call by setting PYTHON_EXECUTABLE. For STIR, I did it the other way round: first run CMake, then use whatever python it found.

just tried (https://travis-ci.org/CCPPETMR/SIRF/builds/318679147), but it's going to fail (osx) since the libs and executable versions are still mismatched, don't want to look into getting them to match (2.7.10 bin, 2.7.14 lib) - anyone else have any ideas?

KrisThielemans · 2017-12-18T23:01:46Z

src/xGadgetron/pGadgetron/tests/test1.py


-@author: Evgueni Ovtchinnikov
+{licence}


this is clearly cleaner. However, it means the license info isn't actually inside the file. I don't think this is according to Apache 2 instructions (and it assumes the file will not be distributed on its own).
Not very important for these tests of course, but I think best to keep as-was (feel free to argue!)

lawyers need to talk to programmers more to understand that "requiring" paragraphs of licence text per-file is insane, irrespective of standaloneness

making "reasonable efforts" per-file (such as this new version) is legally sufficient, regardless of whether that contradicts what any licence text says

this file does actually need and contain the licence text via the import. Running pydoc/help() on this would show the full licence text

because of the above dependency, this file could not work standalone without deliberate modification to remove any reference to licence, which would be clearly illegal

"requiring" paragraphs of licence text per-file is insane

see above

decent programmers do not do avoidable duplication. decent lawyers do not do avoidable repetition. codacy doesn't like code duplication. Even as it stands, this PR fixes some, but not all the duplication

see 1, 5 and 6

Happy to hear any counter arguments!

:-) seems you have strong opinions! I'm ok with this in these test files only. nowhere else though. sorry.

KrisThielemans · 2017-12-19T17:28:02Z

.travis.yml

@@ -125,6 +127,10 @@ before_install:
      pushd $HOME/Library/Python/$PYMVER*/bin
      export PATH="$PWD:$PATH"
      popd
+      # fix CMake variables
+      export BUILD_FLAGS="$BUILD_FLAGS -DPYTHON_LIBRARY=$(python-config --prefix)/lib/libpython2.7.dylib"


what happens if you just don't set PYTHON_LIBRARY etc, only PYTHON_EXECUTABLE?

KrisThielemans · 2017-12-19T17:33:14Z

regarding dependencies, could list only relevant boost packages (they are in the External_boost*), but I think we have better things to do in life.

- travis - PYMVER - SuperBuild: PYVER - MAKEFLAGS="-j 2" - rename *_TESTS -> *_PYTHON_TESTS - use PYTHON_EXECUTABLE

also remove root-system-bin

- remove all KEM recon until the end - make neighbourhood=3 in all the cells except where we need to change it - remove repetition of reconstruction: reuse image reconstructed in previous cell - use 21 subsets and 2 full iterations

Making the notebook faster SyneRBI#71:

casperdcl force-pushed the unify branch from 80ddf1c to db96b59 Compare December 11, 2017 14:07

casperdcl self-assigned this Dec 11, 2017

casperdcl added the help wanted label Dec 11, 2017

casperdcl force-pushed the unify branch 2 times, most recently from 5e7d5d7 to b640671 Compare December 11, 2017 19:25

casperdcl mentioned this pull request Dec 12, 2017

travis improvements SyneRBI/SIRF-SuperBuild#17

Closed

4 tasks

casperdcl added this to the v1.0.0 milestone Dec 12, 2017

casperdcl force-pushed the unify branch 2 times, most recently from 4d6ddef to 0b21b01 Compare December 12, 2017 19:36

casperdcl force-pushed the unify branch 2 times, most recently from 590d3be to 40b31f0 Compare December 13, 2017 12:08

SyneRBI deleted a comment from coveralls Dec 13, 2017

SyneRBI deleted a comment from codecov bot Dec 13, 2017

casperdcl force-pushed the unify branch 6 times, most recently from d3460c2 to 611d257 Compare December 13, 2017 17:14

SyneRBI deleted a comment from coveralls Dec 16, 2017

casperdcl force-pushed the unify branch from 9e8e0f7 to 20aa23d Compare December 17, 2017 01:16

casperdcl force-pushed the unify branch from 20aa23d to bdc3428 Compare December 17, 2017 12:23

KrisThielemans requested changes Dec 18, 2017

View reviewed changes

KrisThielemans reviewed Dec 19, 2017

View reviewed changes

casperdcl added 12 commits January 19, 2018 16:09

ccache

def4b51

cache pip

506e502

more tidy

9179c62

travis tidy

1cdb2ce

clean all tests

3c59fe4

test runner function name clash fix

2fd5de2

travis fix

1bda47f

MacOS fix

1642a04

language: python -> cpp for ccache (WIP)

89a7a91

misc fixes

2f5bdd3

- travis - PYMVER - SuperBuild: PYVER - MAKEFLAGS="-j 2" - rename *_TESTS -> *_PYTHON_TESTS - use PYTHON_EXECUTABLE

try to fix PYTHON_EXECUTABLE for osx

526e1ab

also remove root-system-bin

WIP

c25c73c

casperdcl force-pushed the unify branch from 5158753 to c25c73c Compare January 19, 2018 16:14

casperdcl merged commit c25c73c into master Jan 19, 2018

casperdcl mentioned this pull request Jan 19, 2018

CI: OSX #79

Closed

casperdcl deleted the unify branch January 19, 2018 17:51

johannesmayer pushed a commit to johannesmayer/SIRF that referenced this pull request Feb 21, 2022

Merge pull request SyneRBI#74 from danieldeidda/hkem-speedup

45efe0e

Making the notebook faster SyneRBI#71:

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify #71

Unify #71

casperdcl commented Dec 11, 2017 •

edited

Loading

codecov bot commented Dec 11, 2017 •

edited

Loading

casperdcl commented Dec 12, 2017 •

edited

Loading

casperdcl commented Dec 12, 2017 •

edited

Loading

casperdcl commented Dec 12, 2017

KrisThielemans commented Dec 13, 2017

KrisThielemans commented Dec 13, 2017

casperdcl commented Dec 13, 2017 •

edited

Loading

KrisThielemans commented Dec 17, 2017

casperdcl commented Dec 17, 2017

KrisThielemans commented Dec 17, 2017

casperdcl commented Dec 17, 2017

KrisThielemans commented Dec 17, 2017

casperdcl commented Dec 18, 2017

KrisThielemans Dec 18, 2017

casperdcl Dec 18, 2017

KrisThielemans Dec 19, 2017

casperdcl Dec 19, 2017

KrisThielemans Dec 18, 2017

casperdcl Dec 19, 2017 •

edited

Loading

KrisThielemans Dec 18, 2017

casperdcl Dec 18, 2017

KrisThielemans Dec 19, 2017

casperdcl Dec 19, 2017

KrisThielemans Dec 19, 2017

KrisThielemans commented Dec 19, 2017


		@author: Evgueni Ovtchinnikov
		{licence}

Unify #71

Unify #71

Conversation

casperdcl commented Dec 11, 2017 • edited Loading

Ambitious

codecov bot commented Dec 11, 2017 • edited Loading

Codecov Report

casperdcl commented Dec 12, 2017 • edited Loading

casperdcl commented Dec 12, 2017 • edited Loading

casperdcl commented Dec 12, 2017

KrisThielemans commented Dec 13, 2017

KrisThielemans commented Dec 13, 2017

casperdcl commented Dec 13, 2017 • edited Loading

KrisThielemans commented Dec 17, 2017

casperdcl commented Dec 17, 2017

KrisThielemans commented Dec 17, 2017

casperdcl commented Dec 17, 2017

KrisThielemans commented Dec 17, 2017

casperdcl commented Dec 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

casperdcl Dec 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KrisThielemans commented Dec 19, 2017

casperdcl commented Dec 11, 2017 •

edited

Loading

codecov bot commented Dec 11, 2017 •

edited

Loading

casperdcl commented Dec 12, 2017 •

edited

Loading

casperdcl commented Dec 12, 2017 •

edited

Loading

casperdcl commented Dec 13, 2017 •

edited

Loading

casperdcl Dec 19, 2017 •

edited

Loading