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

Cython support #326

Merged
merged 88 commits into from
Oct 2, 2014
Merged

Cython support #326

merged 88 commits into from
Oct 2, 2014

Conversation

thesamovar
Copy link
Member

This isn't ready to merge yet, but since it's getting there I thought it would be useful to change to a pull request. Fixes #27.

Things to do:

  • Create all templates
  • Make all templates efficient
  • In depth testing
  • Fix crashing on quit.

thesamovar and others added 28 commits July 3, 2014 00:23
cell magic class with a few minor modifications - will probably be
better to use this than cython_inline
- Basically working but need support for functions
@thesamovar
Copy link
Member Author

Currently Cython runs crash on exit. Probably something to do with memory management.

@mstimberg
Copy link
Member

I'll look into the 'found executable' thing but last time I looked I don't think I found a way to do it.

Maybe we should indeed wrap it then to "swallow" all the output.

For the compilation time I'm not sure there is a solution: Cython just takes longer than weave.

It really takes significantly longer though, I don't quite understand why. It isn't the additional cythonize step, it is really the g++ compilation that takes way longer. But you are probably right that we can't do much about it :-/

Is there any way to make travis not have to build everything from scratch? That's the real time killer, right?

You mean the dependencies? We use anaconda and binary packages for that now, that takes only about ~40s in total. It is really the Cython test suite that takes an incredible amount of time. On my work computer, the test suite running weave with nothing pre-compiled takes about 2mins, Cython takes ~15mins. On travis everything takes about 3 times as long and we have a hard limit of 50min for the total test run.

@thesamovar
Copy link
Member Author

Ah I see, I thought it was because we had a small amount of time due to the dependencies.

I'm not sure there is anything we can do to make it run faster unless the compilation options are slowing it down? If so, we could possibly remove optimisation flags during testing? Maybe that would speed up the compilation?

@mstimberg
Copy link
Member

I'm not sure there is anything we can do to make it run faster unless the compilation options are slowing it down? If so, we could possibly remove optimisation flags during testing? Maybe that would speed up the compilation?

Good idea, I'll try running it with -O0 and see how long this takes.

@thesamovar
Copy link
Member Author

I had a little look at techniques for speeding up compilation and that's the only one we have control over. The other suggestions are things like 'use parallel make' and 'use a RAM disk' which we can't do on Travis.

Incidentally the generated code for a Cython extension is around 10k lines whereas a weave one is on the order of 1k lines, so it might just be unavoidable.

@mstimberg
Copy link
Member

Would using cython.inline change anything? Either way, I changed the testing to use -O0 and that does indeed lead to faster compilation, on my machine it takes about half the time now. I also added code that should suppress all output on stdout during the compilation, does that make the doctest pass for you on Windows? It does not seem to work correctly on Python 3 (judging from the log on travis), but maybe that's not a big deal for now.

Even with this change, the compilation on travis is not guaranteed to work, on Python3 it seems to take around 50min, sometimes a bit more, sometimes a bit less... I'll outline a strategy to deal with this in #145 -- but I think we should include the relevant bits in this branch, #145 is more general.

@mstimberg mstimberg mentioned this pull request Oct 1, 2014
3 tasks
@thesamovar
Copy link
Member Author

cython.inline just does something more or less equivalent to what we have (but less flexible).

The tests all pass now except for spatial neuron (values more or less completely different).

I guess it should work on Python 3 before we merge: it's one of the main reasons for implementing Cython support was to have Python 3 support for a compiled target.

@mstimberg
Copy link
Member

The tests all pass now except for spatial neuron (values more or less completely different).

Um, this is on your Windows machine, I assume? Since I fixed the bug in d9ff130, the spatialneuron tests have been passing on my machine and on travis...

I guess it should work on Python 3 before we merge: it's one of the main reasons for implementing Cython support was to have Python 3 support for a compiled target.

Maybe that was a bit unclear: Cython does work on Python3 (at least when the test suite manages to finish in 50min...), it's only the "suppress output" bit that does not seem to work. Users on Python 3 might therefore see warnings during compilation.

@mstimberg mstimberg mentioned this pull request Oct 1, 2014
@thesamovar
Copy link
Member Author

OK the Cython tests all pass now. I guess I failed updating my copy somehow.

I don't mind users seeing the warnings during compilation but if it's going to stop the tests from passing on travis it's a problem for our current workflow.

Actually, I get some occasional test failures but they seem to be intermittent and minor. One I got was a tiny value mismatch in one test in weave. Another was a 'Found executable ...' in the doctest for TimedArray which only came up when I ran all the tests, not just the Cython tests.

So what's the plan? Keep on working on improving the speed of the tests before we merge or go ahead? And is there anything left to do for Cython rather than tests?

Marcel Stimberg added 2 commits October 2, 2014 12:34
…ing the `SkipTest` approach) and test them (together with the doctests) separately from the other tests.
@mstimberg
Copy link
Member

Ok, I implemented most of what I proposed in #145, the reduced test suite is now completing within a reasonable time frame.

About the warnings during compilation: even though it is not strictly-speaking true, I now included all doctests in the "codegen-independent" part of the test suite and run it under numpy. Therefore the warnings shouldn't interfere with the doctests, the only place where we really check the output. I think this is justified, since the doctests are only meant to show the syntax, we do the thorough testing in the normal tests.

I also created a new branch with an .travis.yml to run the long test suite: https://travis-ci.org/brian-team/brian2/builds/36866479
I'm not quite sure yet what the best long-term solution to do the long testing. For now, the following should work whenever we want to test a new branch thoroughly:

git branch new_branch_for_long_test
git cherry-pick 9c9d2818
git push new_branch_for_long_test

From my side there's nothing else I plan to do in this branch, if both the test suites pass, we are ready to merge.

@thesamovar
Copy link
Member Author

OK and everything continues to pass on windows too. Time to do the merge!

thesamovar added a commit that referenced this pull request Oct 2, 2014
@thesamovar thesamovar merged commit 850a2c7 into master Oct 2, 2014
@thesamovar thesamovar deleted the cython_codegen2 branch October 2, 2014 13:52
@mstimberg
Copy link
Member

You didn't wait for the long tests to finish :) Anyway, they are looking good so far and I don't see why they should fail.

@thesamovar
Copy link
Member Author

Oh I misunderstood! I ran them on my machine at least. ;)

mstimberg pushed a commit that referenced this pull request Oct 6, 2014
mstimberg pushed a commit that referenced this pull request Oct 6, 2014
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.

Add Cython as a codegen target
2 participants