Skip to content

Conversation

ghost
Copy link

@ghost ghost commented Jan 4, 2023

@GPUtester
Copy link
Collaborator

Can one of the admins verify this patch?

Admins can comment ok to test to allow this one PR to run or add to allowlist to allow all future PRs from the same author to run.

@mrocklin
Copy link
Member

mrocklin commented Jan 5, 2023

Hrm, this is interesting. It feels wrong to me for a library to change a value like this. I'm surprised to learn that Jedi does it. cc'ing IPython dev @Carreau to see if he has recommendations/thoughts.

@ghost
Copy link
Author

ghost commented Jan 5, 2023

Yes, I was also surprised, you can see the relevant code here: https://github.com/davidhalter/jedi/blob/3602c10916a29d19e7b5b89cfc6d8970b8bb488e/jedi/api/__init__.py#L48

It seems like it would make more sense to increase the limit only around specific code that requires it (I guess the _analysis function is probably what this is for) and reset it afterwards.

Probably most code doesn't depend on the recursion limit being handled properly, and I think this may not be a problem on other OSes due to larger default stacks, so I expect this hasn't been a high-priority issue for anyone.

@mrocklin
Copy link
Member

mrocklin commented Jan 5, 2023

If we're going to do this then I'd suggest two things:

  1. Do it in IPython, not in Dask
  2. Capture the value before the import and then set that value after the import so that we're sensitive to the context in which we find ourselves.

@ghost
Copy link
Author

ghost commented Jan 5, 2023

I think if there's a "right" place to fix this, it's probably in Jedi, since this is essentially Jedi leaking global state to everything that imports it.

I don't know much about Jedi, though, so someone who's familiar with it would need to figure out how to make this more targeted.

@davidhalter
Copy link

(Author of Jedi here)

I probably agree that Jedi should deal with this. I will have to investigate how expensive setting the recursion limit is. This should ideally not be done at all, but for Jedi this is kind of needed unfortunately. We could however try setting this only upon entering type inference or something similar. It's however not that easy to do that, because Jedi has tons of entry points.

I never really thought that this could be an issue, because Jedi in its original form was thought to do static analysis and would therefore most likely not run arbitrary software next to it in the same process. Once we started doing the Python Shell/IPython integration, this was obviously not true anymore.

So, don't expect results soon, but this is definitely something that we should change.

@davidhalter
Copy link

I wrote a bit more in davidhalter/jedi#1902 (comment)

While I think this is definitely an issue on Jedi's side, I want to understand one thing:

You just want to have a RecursionError instead of a stack overflow in this particular case, right?

@Carreau
Copy link
Contributor

Carreau commented Jan 6, 2023

I can try to add a workaround in IPython as well and add a context manager to set the recursion limit when we enter/exit jedi, my main concern is I believe the type inference of completion is a generator, so we may need to enter/exit for each item which may have a cost, if like @davidhalter said setting the recursion limit has a cost.

locally sysetrecursionlimit/getrecustionlimit take ~15ns.

It does not seem to be smart and skip setting it if it's already the same value, though is fairly strait forward so I'm not sure it can grow to much slower.

@davidhalter
Copy link

That's pretty fast, it makes definitely sense to set it in Jedi in that case.

@mrocklin
Copy link
Member

mrocklin commented Jan 6, 2023

Thank you @Carreau and @davidhalter for engaging here. Feel free to keep engaging on this PR for conversation, but I'm probably going to close this for now. This is, I think, clearly an upstream issue and it looks like upstream is handling it. Thank you all.

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.

test_tokenize_object_with_recursion_error() triggers stack overflow on Windows due to IPython imports
5 participants