Skip to content

!B [#7852] fix for _rrule maximum recursion depth exceeded on multiprocessing usage #7854

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

Merged
merged 5 commits into from
Jan 20, 2017

Conversation

ahcub
Copy link
Contributor

@ahcub ahcub commented Jan 17, 2017

issue description:
#7852

Copy link
Member

@efiring efiring left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I see (I think): you are using multiprocessing, which is pickling, requiring the special access method to the special methods.

@ahcub
Copy link
Contributor Author

ahcub commented Jan 17, 2017

yes, that is correct

@codecov-io
Copy link

codecov-io commented Jan 17, 2017

Current coverage is 62.10% (diff: 50.00%)

Merging #7854 into master will increase coverage by <.01%

@@             master      #7854   diff @@
==========================================
  Files           174        174          
  Lines         56065      56067     +2   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          34820      34823     +3   
+ Misses        21245      21244     -1   
  Partials          0          0          

Powered by Codecov. Last update fa95b58...c189bf9

@ahcub
Copy link
Contributor Author

ahcub commented Jan 17, 2017

Should I add a new test for this code change?

@efiring
Copy link
Member

efiring commented Jan 17, 2017

Yes, a test would be good. Maybe in test_pickle.py?

@ahcub
Copy link
Contributor Author

ahcub commented Jan 17, 2017

I struggle a bit with creation of test for this case without pandas
do you know how I can make it easily?
not sure that adding pandas dependency is a correct approach here...

@NelleV
Copy link
Member

NelleV commented Jan 17, 2017

Can you create a mock object whose API is close to the one from pandas?

(pandas should not be added as a dependency).

@efiring
Copy link
Member

efiring commented Jan 17, 2017

I don't think anything should depend on Pandas, or needs to here. If the problem is pickling an rrulelocator, it should be possible to test it very directly by analogy with some other tests in the test_pickle.py module.

@dopplershift
Copy link
Contributor

I agree--mocking Pandas seems a bit heavy-handed, if not wrong. If the problem is pickling, test pickling.

@tacaswell
Copy link
Member

We have some tests that run with pandas, you can just use it. Follow the pattern from

def test_pandas_iterable():

@QuLogic
Copy link
Member

QuLogic commented Jan 18, 2017

You could, but it's not really necessary to use pandas to reproduce the problem. Something like this is enough to trigger the recursion:

>>> import matplotlib.dates
>>> import pickle
>>> r = matplotlib.dates.rrulewrapper(2)
>>> r
<matplotlib.dates.rrulewrapper object at 0x7f3c9de18470>
>>> pickle.loads(pickle.dumps(r))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  ...
  File "/home/elliott/code/matplotlib/lib/matplotlib/dates.py", line 716, in __getattr__
    return getattr(self._rrule, name)
RecursionError: maximum recursion depth exceeded while calling a Python object

It just needs to be wrapped up in the test functions and given better names and such.

@ahcub
Copy link
Contributor Author

ahcub commented Jan 18, 2017

awesome, QuLogic, thank you for the example!
I will add a test now

@ahcub
Copy link
Contributor Author

ahcub commented Jan 19, 2017

added the test
4e08061

@ahcub
Copy link
Contributor Author

ahcub commented Jan 19, 2017

does anybody know how a new test could lead to failure of those checks?
it's a bit strange that code coverage decreased..

@jenshnielsen
Copy link
Member

Part of the failure is our PEP8 checker, You have a blank line containing whitespaces. I suggest fixing that and let the tests rerun. I think the other failure is a random fluke

except RecursionError:
print('rrulewrapper pickling test failed')
raise

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You still have a white space issue a this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, I didn't notice that
committed a change

@NelleV NelleV merged commit 5881e72 into matplotlib:master Jan 20, 2017
@NelleV
Copy link
Member

NelleV commented Jan 20, 2017

Thanks for the patch!

@QuLogic QuLogic added this to the 2.1 (next point release) milestone Jan 20, 2017
@tacaswell
Copy link
Member

👍 to backporting, but it needs a bit more work as this 'fixes' the problem via

In [27]: r.__getstate__
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-27-9cc7b4e1bb10> in <module>()
----> 1 r.__getstate__

/home/tcaswell/src/p/matplotlib/lib/matplotlib/dates.py in __getattr__(self, name)
    713         self._construct.update(kwargs)
    714         self._rrule = rrule(**self._construct)
--> 715 
    716     def __getattr__(self, name):
    717         if name in self.__dict__:

AttributeError: type object 'object' has no attribute '__getattr__'

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.

8 participants