PR: Monkey patch Jedi 0.10.0 for numpydoc #4121
Conversation
|
Thanks a lot @bcolsen fir giving us a hand with this! |
|
@ccordoba12 If it passes testing, It's ready for review. |
|
You need to remove the pins I introduced on PR #4104 for setup.py and meta.yaml |
|
Subscribing myself to this bug, since this would enable code completion to work on Debian with the packaged |
|
Just as a test run, I cherry-picked a squashed version of this PR on top of the current Many thanks @bcolsen. |
| @@ -22,10 +22,11 @@ def apply(): | |||
|
|
|||
| See [1] and [2] module docstring.""" | |||
| from spyder.utils.programs import is_module_installed | |||
| if is_module_installed('jedi', '=0.9.0'): | |||
| if (is_module_installed('jedi', '=0.9.0') or | |||
| is_module_installed('jedi', '=0.10.0')): | |||
There was a problem hiding this comment.
Please change this to
if (is_module_installed('jedi', '>=0.9.0'):There was a problem hiding this comment.
I remember that I wrote it that way so that when Jedi 0.11.0 comes out my patching won't thrash it(like it did when 0.10.0 came out). I am also hoping not to have to patch 0.11.0 :-)
I can still make the change if you think that's best.
There was a problem hiding this comment.
Ok, fair enough ;-) Please leave it as it is then.
| import jedi | ||
| else: | ||
| raise ImportError("jedi %s can't be patched" % jedi.__version__) | ||
| raise ImportError("jedi not =0.9.0 or 0.10.0, can't be patched") |
There was a problem hiding this comment.
raise ImportError("jedi not >=0.9.0, can't be patched")
| @@ -106,8 +108,8 @@ def _search_return_in_numpydocstr(docstr): | |||
| return found | |||
|
|
|||
|
|
|||
| @memoize_default(None, evaluator_is_first_arg=True) | |||
| def find_return_types(evaluator, func): | |||
| #@memoize_default() | |||
There was a problem hiding this comment.
So this should be removed or not?
There was a problem hiding this comment.
I left it there to show that I disabled caching for that function to make monkey patch work.
I had to change the way I'm monkey patching and I couldn't get the caching to work. Luckily, I didn't notice a difference in the run speed.
I am able to cache with the upstream patch.
I can give you more details on the problem later today.
There was a problem hiding this comment.
Please add a comment above this line mentioning this reason.
|
I also left another couple of (very simple) comments ;-) |
7c06e5b to
113f5ef
Compare
|
I fixed the caching....I just changed it back to the way it was in 0.9.0 and that still works in 0.10.0 If it passes the tests it's ready to go. |
Conda packages are still at 0.9.0
|
@bcolsen, I added Jedi 0.10.0 to CircleCI and, as you can see, our tests are failing there (the other CIs are still using Jedi 0.9.0 because they can't be modified so easily, so please only look at the results in CircleCI). Besides, I'd like to propose you to drop support for 0.9 and only focus on 0.10. The patches for 0.9 and 0.10 are very different, and since we won't be able to test both versions, I'd prefer to drop support for the oldest one. That'd also simplify your work ;-) |
|
@ccordoba12 Are you satisfied with the latest changes from @bcolsen ? If so, could you ack it (even if pushed later), so I can start cherry-picking the final fix to the Debian package. Thanks. |
|
I'll give it a final review on Wednesday. |
|
Any news? |
|
At least, can I have confirmation that all commits up to f9fe24d can be squashed and cherry-picked to v3.1.2 or v3.1.3 to get code completion working with Jedi 0.10? Thanks. It is getting very unlikely |
|
@ghisvail, tests are passing, so I'd say yes. However, I'd like to give this one a little bit of manual testing before merging. I'll promise you I'll do that this weekend ;-) |
|
As far as testing is concerned, I have used a package I built locally with |
|
Ok, merging this one then :-) Thanks a lot @bcolsen! |
|
@ccordoba12 Unfortunately, this PR does not apply cleanly on top of |
|
3.1.3 fixes a lot of segfaults introduced in 3.1.0 because of some odd focus bugs, so I'd really prefer if you use it instead of 3.1.2. |
|
It would help me build my case if you could provide a separate listing containing only these fixed segfaults on Linux. The more comprehensive my request is, the more likely it will be accepted. Thanks. |
|
Please take a look at our Changelog. Those segfaults affected all our supported OSes. |
Since you personally reviewed most, if not all, the PR listed in this change log, you are best placed to identify the most critical issues. Even if it is just the top 3 or 5. |
|
So you want to use only certain patches? |
|
I mean, it's a bugfix release, so why not package the full release instead? |
No.
That's the plan. I'll provide some more context. Debian is in freeze, meaning new versions of software no longer migrate automatically to what is going to become the next stable release. Instead, updates of packages should be focusing on addressing release critical bugs affecting the frozen packages (such as failures to build, security issues, crashes...). Updates to packages can migrate if an unblock request is granted from the release team, who will manually review the proposed changes and assess the necessity of such update (here to Hence myself asking whether you could make a selection of the most critical bugs fixed by |
|
Ok, thanks for the thorough explanation. I'll provide you with the highlights of 3.1.3 this weekend. |
|
Did you have a chance to look at it? We now have version 3.1.3 + this fix in Debian experimental and would like to draft the unblock request quite soon. Thanks. |
|
The general picture: I consider 3.1.3 the first version in the 3.1 series that is stable enough to be used by the general public. All previous versions were semi-broken because of important bugs and I don't recommend to use them. Critical bugs fixed in 3.1.3 (I don't link them with
All these issues were caused by a couple of focus bugs introduced by our junior developers. It was not their fault really, we simply didn't know that changing focus policies for the main window and the application will have those terrible effects. To avoid this in the future, we have expanded our test suite as much as we can in 3.1.3 and 3.1.4. |
|
Thanks for providing the list of critical bugs, here is what I found:
Not reproducible on
Not reproducible on
Reproducible on
Not reproducible on
Reproducible on Debian, but not your usual use case of the var explorer tbh. To sum up, that gives me 2 bugs out of 5 which are reproducible with the current package, including one for an atypical use case. That does not give me much of a case but I'll try. |
|
I hope my opinion counts too :-) I mean, there are other annoying bugs (like wrong auto-indentation, no new breakpoints in the IPython console while debugging and broken maximizing-the-current-pane button) and I don't want to keep telling people: please update because those things were fixed already. I have to say that that really bothers me as a maintainer: we're doing our duty of releasing bugfix versions, but they are not being picked up by Debian. At least those versions should be easily backported to the stable Debian branch. Sigh, unfortunately I know that's a lost battle with Debian.... |
I voiced your opinion, and referenced all your supporting material, in the following request for unblock. The rest is up to the release team at this point.
They are. The software is currently up-to-date in the
We will provide backports of bugfix releases in a |
|
Ok, thanks for the good news! |
|
The unblock request has been accepted. Debian Stretch will have 3.1.3 + this fix + #4311 for now. If 3.1.4 gets released soon enough, I might try to request another unblock again. |
This works now...but not ready for review yet. I think I need some PEP8
It's a bit more of a mess than I would like but I have a plan to get this upstream faster.