Remove cython md5 hashing since it breaks the build process by hmaarrfk · Pull Request #3254 · scikit-image/scikit-image

hmaarrfk · 2018-07-07T02:46:35Z

In developing #3253, I had to keep issuing commands:

rm */*/*.md5 */*/*.c 
git checkout -- skimage/restoration/unwrap_2d_ljmu.c skimage/restoration/unwrap_3d_ljmu.c

this is because the hashing doesn't consider pxd files.
This leads to the cython compiler doing really weird things and crashing.

Finally, cython will check itself if the files have changed, so this isn't necessary.

As a bonus, if you clump together c and c++ files seperately, cython will cythonize them in parallel using our infrastructure. This speeds things up greatly on multicore machines.

~~You have to hard code the number of threads to use, or maybe somebody knows the nproc command in python, but 8 is a safe number for now :/~~ Thanks @emmanuelle

Checklist

[It's fine to submit PRs which are a work in progress! But before they are merged, all PRs should provide:]

Clean style in the spirit of PEP8
Docstrings for all functions
Gallery example in ./doc/examples (new features only)
Benchmark in ./benchmarks, if your changes aren't covered by an
existing benchmark
Unit tests

[For detailed information on these and other aspects see scikit-image contribution guidelines]

References

[If this is a bug-fix or enhancement, it closes issue # ]
[If this is a new feature, it implements the following paper: ]

For reviewers

(Don't remove the checklist below.)

Check that the PR title is short, concise, and will make sense 1 year
later.
Check that new functions are imported in corresponding __init__.py.
Check that new features, API changes, and deprecations are mentioned in
doc/release/release_dev.rst.
Consider backporting the PR with @meeseeksdev backport to v0.14.x

codecov-io · 2018-07-08T21:57:34Z

Codecov Report

Merging #3254 into master will decrease coverage by 0.76%.
The diff coverage is 6.66%.

@@            Coverage Diff             @@
##           master    #3254      +/-   ##
==========================================
- Coverage   87.77%   87.01%   -0.77%     
==========================================
  Files         323      340      +17     
  Lines       27138    27430     +292     
==========================================
+ Hits        23820    23867      +47     
- Misses       3318     3563     +245

Impacted Files	Coverage Δ
skimage/restoration/setup.py	`26.31% <0%> (ø)`
skimage/filters/setup.py	`27.77% <0%> (ø)`
skimage/transform/setup.py	`31.25% <0%> (ø)`
skimage/morphology/setup.py	`27.77% <0%> (ø)`
skimage/_shared/setup.py	`33.33% <0%> (ø)`
skimage/feature/setup.py	`22.72% <0%> (ø)`
skimage/measure/setup.py	`27.77% <0%> (ø)`
skimage/segmentation/setup.py	`35.71% <0%> (ø)`
skimage/graph/setup.py	`33.33% <0%> (ø)`
skimage/_build.py	`26.66% <20%> (ø)`
... and 35 more

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 ef1d91a...7a3c154. Read the comment docs.

hmaarrfk · 2018-07-08T22:19:11Z

Other people thinking about parallel builds: matplotlib/matplotlib#1507

hmaarrfk · 2018-08-06T00:04:34Z

As a note, if you setup CCACHE correctly, then recompiling skimage takes almost no time.

soupault

LGTM!

soupault · 2018-08-12T19:56:01Z

skimage/_build.py

+                pyx_files[i] = pyxfile.replace('.pyx.in', '.pyx')

-    return md5_cached != md5_new.encode('utf-8')
+        cythonize(pyx_files, nthreads=8)


I'm not sure about hardcoding the number of threads. I mean this looks like a reasonable default, but I'm just not sure :).

Let me experiment with this number tonight. Cython might use ncores*2 automatically.

@hmaarrfk Doesn't look so - https://github.com/cython/cython/blob/a0bbb940c847dfe92cac446c8784c34c28c92836/Cython/Build/Dependencies.py#L923-L925

I added a note for it.

sciunto · 2018-09-12T05:26:00Z

This might (partially) solve #3387 don't you think?

sciunto · 2018-09-12T05:27:39Z

@hmaarrfk Could you rebase please? Could you indicate your command line for ccache as well?

stefanv

Is this a new feature in Cython? How does it know not to regenerate the C file?

stefanv · 2018-09-12T05:28:25Z

skimage/_build.py

+                pyx_files[i] = pyxfile.replace('.pyx.in', '.pyx')

-    return md5_cached != md5_new.encode('utf-8')
+        # Cython doesn't automatically chose a number of threads > 1


hmaarrfk · 2018-09-12T08:31:06Z

@stefanv beats me, this might be kinda like "I'm so new to cython I do not really remember when it didn't do that"

hmaarrfk · 2018-09-12T08:32:11Z

@sciunto I don't really know a reliable installation guide, can you try

sudo apt install ccache
export PATH="/usr/lib/ccache:$PATH"

type these manually to test

make clean
make
make clean 
make

hmaarrfk · 2018-09-12T08:34:24Z

I have this in my ~/.profile fille

export PATH="/usr/lib/ccache:$PATH"

hmaarrfk · 2018-09-12T08:35:29Z

➤ which gcc                                          
/usr/lib/ccache/gcc

➤ which g++                                          
/usr/lib/ccache/g++

hmaarrfk · 2018-09-12T08:52:07Z

How did python 3.7 finish so quickly? 15 mins vs 30+ mins.

sciunto · 2018-09-12T08:59:33Z

I gained 2 minutes (from 7 to 5) on my machine. I changed the path to reflect the difference of my distribution. (which g++ /usr/lib/ccache/bin/g++) The gain is not tremendous.

hmaarrfk · 2018-09-12T09:19:26Z

ccache should make gcc commands instant...

i7 laptop:

Cythonizing 1 at a time, no ccache 4m 12s
Cythonizing 1 at a time, with ccache 58s
Parallel cythonizing with ccache 29s (this branch)

hmaarrfk · 2018-09-12T09:23:46Z

Can you give the output of:

ccache --zero-stats 
make clean
make
ccache --show-stats

Mine is (once ccache is built of course)

cache directory                     /home/mark/.ccache
primary config                      /home/mark/.ccache/ccache.conf
secondary config      (readonly)    /etc/ccache.conf
cache hit (direct)                    47
cache hit (preprocessed)               0
cache miss                             0
called for link                       50
no input file                          1
files in cache                       180
cache size                         117.7 MB
max cache size                       5.0 GB

sciunto · 2018-09-12T10:07:06Z

Thanks for sharing.

Quite weird over here. My cache size was 2GB over 5GB. Cache hits were low and only preprocessed.
I cleared the cache. Now, the hits are only direct. However, a make clean; make still takes the same duration. Anyway, it might be a specific issue on my side.

hmaarrfk · 2018-09-12T12:33:18Z

it is not always perfect, and soemtimes finiky, but yeah, sometimes it works.

stefanv · 2018-09-12T17:55:16Z

Look at the time stamps of the generated files: ``` (py36) carbo:/tmp $ ls -al foo.c -rw-r--r-- 1 stefan stefan 159139 Sep 12 10:53 foo.c (py36) carbo:/tmp $ cython foo.pyx (py36) carbo:/tmp $ ls -al foo.c -rw-r--r-- 1 stefan stefan 159139 Sep 12 10:54 foo.c ```

hmaarrfk · 2018-09-12T18:02:37Z

@stefanv but we aren't calling cythonize directly, we are calling it through python. I think it behaves differently for me:

Try this command

python -c 'from Cython.Build import cythonize; cythonize("foo.pyx")'

hmaarrfk · 2018-09-12T18:10:50Z

@stefanv I think cython and cythonize are two different commands. Dam dam dam.............

cythonize

date; ls -lah foo.c; cythonize foo.pyx; ls -lah foo.c; python -c 'import Cython; print(Cython.__version__)'                                                                                                (owl) 
Wed Sep 12 14:08:09 EDT 2018
-rw-rw-r-- 1 mark mark 90K Sep 12 14:05 foo.c
-rw-rw-r-- 1 mark mark 90K Sep 12 14:05 foo.c
0.28.5

cython

date; ls -lah foo.c; cython foo.pyx; ls -lah foo.c; python -c 'import Cython; print(Cython.__version__)'                                                                                                   (owl) 
Wed Sep 12 14:10:31 EDT 2018
-rw-rw-r-- 1 mark mark 89K Sep 12 14:09 foo.c
-rw-rw-r-- 1 mark mark 89K Sep 12 14:10 foo.c
0.28.5

stefanv · 2018-09-13T00:00:35Z

To test this, you need to have it inside of a setup.py file: ``` from distutils.core import setup, Extension from Cython.Build import cythonize module1 = cythonize('foo.pyx')[0] setup (name = 'PackageName', version = '1.0', description = 'This is a demo package', ext_modules = [module1]) ``` But, you are right that, in combination with distutils, it remembers what has been built before and doesn't rebuild it. I would be curious to know where that information is stored! So, yes, +1 from me on this change then.

emmanuelle · 2018-09-30T17:10:36Z

For an automatic selection of the number of threads, use multiprocessing.cpu_count() (joblib code is useful to find such goodies :-))

hmaarrfk · 2018-09-30T18:59:09Z

Thanks @emmanuelle tests are now failing because of #3432

jni

Alright, this now may be holding up other PRs, so I'm merging. Thanks @hmaarrfk! Based on the discussion, this looks brilliant!

hmaarrfk · 2018-10-11T22:53:54Z

@jni, let me rebase.

hmaarrfk · 2018-10-12T00:00:19Z

Thanks for waiting @jni, this build stuff tends to break if we wait too long between test + merge.

sciunto · 2018-10-12T06:13:38Z

I merge for @jni.

jni · 2018-10-12T06:47:43Z

💯

sciunto · 2018-10-14T09:16:03Z

A comment regarding the discussion on CCACHE. I checked on a system with a i7 CPU, and it divided the duration by a factor 4. It might be that my old atom CPU is not good enough for such optimization.

hmaarrfk · 2018-10-14T12:55:15Z

Sorry you have an Atom 😢

hmaarrfk force-pushed the remove_md5_hashing branch from 5487ae2 to e52dfe7 Compare July 8, 2018 22:18

hmaarrfk mentioned this pull request Jul 8, 2018

Do not merge! Travis: explicitely run optional tests and QT test for OSX #3231

Closed

4 tasks

hmaarrfk force-pushed the remove_md5_hashing branch 3 times, most recently from d3a4cb5 to d17a82c Compare July 9, 2018 00:21

soupault added the ⚖️ Needs decision label Jul 29, 2018

hmaarrfk force-pushed the remove_md5_hashing branch from d17a82c to c2e4b4a Compare August 6, 2018 00:06

soupault approved these changes Aug 12, 2018

View reviewed changes

soupault added 🔧 type: Maintenance Refactoring and maintenance of internals and removed ⚖️ Needs decision labels Aug 12, 2018

soupault added this to the 0.15 milestone Aug 12, 2018

hmaarrfk force-pushed the remove_md5_hashing branch from c2e4b4a to 5f11cb4 Compare August 13, 2018 12:46

soupault added the status: mrg+1 label Aug 13, 2018

hmaarrfk changed the title ~~Remove cython md5 hashing as it isn't needed anymore~~ Remove cython md5 hashing since it breaks the build process Sep 12, 2018

stefanv reviewed Sep 12, 2018

View reviewed changes

hmaarrfk force-pushed the remove_md5_hashing branch 2 times, most recently from ccd97f9 to 0544afd Compare September 12, 2018 08:24

hmaarrfk force-pushed the remove_md5_hashing branch 3 times, most recently from a566694 to 4dd5152 Compare September 30, 2018 17:38

hmaarrfk force-pushed the remove_md5_hashing branch from 4dd5152 to 60e04bf Compare October 5, 2018 01:16

hmaarrfk mentioned this pull request Oct 11, 2018

[WIP] Add fused types, attempt to use in denoise #3469

Closed

9 tasks

jni approved these changes Oct 11, 2018

View reviewed changes

hmaarrfk added 3 commits October 11, 2018 18:54

Remove cython md5 hashing as it isn't needed anymore

f594f79

build: cythonize files in parallel.

a2eb962

Travis: show that ccache at the right time

7a3c154

hmaarrfk force-pushed the remove_md5_hashing branch from 60e04bf to 7a3c154 Compare October 11, 2018 22:54

sciunto merged commit 9dd29ae into scikit-image:master Oct 12, 2018

hmaarrfk deleted the remove_md5_hashing branch October 14, 2018 12:55

hmaarrfk mentioned this pull request Oct 19, 2019

sdist review (Oct 2019) #4261

Closed

2 tasks

grlee77 mentioned this pull request Aug 1, 2020

Investigate build system quirks #3387

Closed

Uh oh!

Conversation

hmaarrfk commented Jul 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

References

For reviewers

Uh oh!

codecov-io commented Jul 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

hmaarrfk commented Jul 8, 2018

Uh oh!

hmaarrfk commented Aug 6, 2018

Uh oh!

soupault left a comment

Choose a reason for hiding this comment

Uh oh!

soupault Aug 12, 2018

Choose a reason for hiding this comment

Uh oh!

hmaarrfk Aug 12, 2018

Choose a reason for hiding this comment

Uh oh!

soupault Aug 13, 2018

Choose a reason for hiding this comment

Uh oh!

hmaarrfk Aug 13, 2018

Choose a reason for hiding this comment

Uh oh!

sciunto commented Sep 12, 2018

Uh oh!

sciunto commented Sep 12, 2018

Uh oh!

stefanv left a comment

Choose a reason for hiding this comment

Uh oh!

stefanv Sep 12, 2018

Choose a reason for hiding this comment

Uh oh!

hmaarrfk commented Sep 12, 2018

Uh oh!

hmaarrfk commented Sep 12, 2018

Uh oh!

hmaarrfk commented Sep 12, 2018

Uh oh!

hmaarrfk commented Sep 12, 2018

Uh oh!

hmaarrfk commented Sep 12, 2018

Uh oh!

sciunto commented Sep 12, 2018

Uh oh!

hmaarrfk commented Sep 12, 2018

Uh oh!

hmaarrfk commented Sep 12, 2018

Uh oh!

sciunto commented Sep 12, 2018

Uh oh!

hmaarrfk commented Sep 12, 2018

Uh oh!

stefanv commented Sep 12, 2018 via email

Uh oh!

hmaarrfk commented Sep 12, 2018

Uh oh!

hmaarrfk commented Sep 12, 2018

cythonize

cython

Uh oh!

stefanv commented Sep 13, 2018 via email

Uh oh!

emmanuelle commented Sep 30, 2018

Uh oh!

hmaarrfk commented Sep 30, 2018

Uh oh!

jni left a comment

Choose a reason for hiding this comment

Uh oh!

hmaarrfk commented Oct 11, 2018

Uh oh!

hmaarrfk commented Jul 7, 2018 •

edited

Loading

codecov-io commented Jul 8, 2018 •

edited

Loading