MAINT: add ability to specify recursionlimit #7843
Conversation
We're getting a test failure on OSX and Python 3.6 where parsing reaches the recursion limit: matplotlib#7799 Upping the recursionlimit resolves the error. Add command line argument to tests to up the recursion limit for the test run.
|
How high do you have to raise the limit? And why would the problem be specific to python 3.6? Does it have a lower default limit? On 3.5, OS X, Anaconda, it is 1000. Would it make sense to increase the default in tests.py in addition to, or instead of, adding the option? |
|
I think it's always 1000, by default. Maybe it would be good to raise the default limit, I was worried that we might be concealing other problems by doing that. I have no idea why Python 3.6 generates this error, maybe it is because of different dict ordering on Python 3.6? |
|
Am I understanding properly that this has been merged to v2.x? |
|
This is the equivalent fix to the one that has been merged for v2.x - but the code being modified has changed so much that the v2.x fix won't apply, and this one needs to be merged for master. |
|
I am slightly worried that this is going to conflict with the merging v2 into master strategy. |
|
To be more clear, merging branches into master is a practice that concerns me for many reason, this particular one included. I think the workflow should be to backport patches to v2.0, so that patches that conflicts can be dealt with by a new PR, which is the workflow you've taken. I don't know how having two patches for the same bug on different branches is going to affect us. It is probably just going to be a question of fixing the conflict when merging v2.0.x into master, but the question is whether we want to merge the patch twice, or just fix the conflict when merging v2.0.x into master. A more traditional workflow would not have put us into that situation. |
You could forward port as well, if you prefer. The merging 2.0.x into master may conflict, then you'll have to resolve by hand and push directly. I personally don't like that. But that will only give you one conflict indeed. Personally, I'm going to guess and not pronounce myself for the rest of the Jupyter team as we have grown a lot. Backporting is much easier for blame and bisect. Though our N.x branches are usually short-lived, and we are pretty strict on what is "backported". The other reason to "backport" is that users tend to issue PRs against master, so it keeps the workflow identical and surprise-less for everyone. Pr against master, and backport if deemed necessary. It also put a slightly higher threshold on the energy necessary for backporting (which has to be explicit) leading to less "accidental new features" being backported. Anyway, If you want me the add a backport/forward port variation to my backport bot to accommodate your use case I will be happy to do so. It can likely fw/backport to many branch, and push directly instead of submitting PRs. |
|
I actually think it is better to create PRs, as it allows to tests the backport. Because we are in the transition between nose and pytest for example, backporting (or forward porting) may be "clean", but still lead to failures. |
|
@NelleV Can we move this discussion to the mailing list? |
Current coverage is 62.27% (diff: 100%)@@ master #7843 diff @@
==========================================
Files 174 174
Lines 56051 59294 +3243
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 34808 36927 +2119
- Misses 21243 22367 +1124
Partials 0 0
|
|
I have done the merge up from 2.0.x so this is good to merge. |
|
Thanks @matthew-brett and sorry for all the confusion. |
We're getting a test failure on OSX and Python 3.6 where parsing reaches the
recursion limit: #7799
Upping the recursionlimit resolves the error.
Add command line argument to tests to up the recursion limit for the
test run.