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.

Sign up for GitHub

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

Jump to bottom

PR: Add support for Jupyter-client >= 7 #16644

Merged
merged 10 commits into from
Nov 22, 2021
Merged

PR: Add support for Jupyter-client >= 7 #16644

merged 10 commits into from
Nov 22, 2021

Conversation

bnavigator
Copy link
Contributor

@bnavigator bnavigator commented Oct 26, 2021 •
edited by ccordoba12
Loading

Description of Changes

  • Wrote at least one-line docstrings (for any new functions)
  • Added unit test(s) covering the changes (if testable)
  • Included a screenshot or animation (if affecting the UI, see Licecap)

Follow-up on spyder-ide/spyder-kernels#328
Fixes #16762.

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct: @bnavigator

@bnavigator bnavigator changed the base branch from master to 5.x October 26, 2021 19:24
@pep8speaks
Copy link

pep8speaks commented Oct 26, 2021 •
edited
Loading

Hello @bnavigator! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-11-22 19:59:32 UTC

@ccordoba12
Copy link
Member

Hey @bnavigator, thanks a lot for your help with this! A couple of comments:

  1. You need to resync your branch in spyder-kernels with our subrepo here after moving it to 2.x there.
  2. In order to install jupyter-client 7, you need to edit our install.sh script and add the respective commands there (i.e. mamba install jupyter_client=7, etc) after the removal of spyder-kernels.

@bnavigator bnavigator marked this pull request as ready for review October 31, 2021 18:52
@bnavigator
Copy link
Contributor Author

I think this is ready.

I reverted the "test commits", which force jupyter_client 7 and clone the subrepo from spyder-ide/spyder-kernels#328. The checks on 46556ad show that it works. (The Mac App Bundle error looks unrelated)

@ccordoba12 ccordoba12 changed the title PR: override KernelManager._kill_kernel differently for jupyter_client >= 7 PR: Override KernelManager._kill_kernel differently for jupyter_client >= 7 Nov 1, 2021
@ccordoba12
Copy link
Member

I think this is ready.

Thanks @bnavigator for your help!

I reverted the "test commits", which force jupyter_client 7 and clone the subrepo from spyder-ide/spyder-kernels#328

I think you should remove them and leave commit 46556ad as the last one on this PR, so that we continue our testing with Jupyter-client 7.

(The Mac App Bundle error looks unrelated)

Please merge with the latest 5.x or rebase on top of it to get the fixes for that.

@ccordoba12
Copy link
Member

ccordoba12 commented Nov 1, 2021 •
edited
Loading

By the way, I'm a bit worried about issue jupyter/jupyter_client#715 because on Windows kernels take a long time to start (I've seen them take up to a minute to do it).

So, we'll have to wait until it's fixed (there's a PR for that already). After that is part of an official release, I think we can merge this.

@ccordoba12 ccordoba12 changed the title PR: Override KernelManager._kill_kernel differently for jupyter_client >= 7 PR: Add support for Jupyter-client >= 7 Nov 1, 2021
@bnavigator
force install jupyter_client=7 in Github CI
969e45b
@bnavigator
git subrepo clone --branch=client7 --force git@github.com:bnavigator/…
5cb91b0 
…spyder-kernels.git external-deps/spyder-kernels

subrepo:
  subdir:   "external-deps/spyder-kernels"
  merged:   "e009d4ca4"
upstream:
  origin:   "git@github.com:bnavigator/spyder-kernels.git"
  branch:   "client7"
  commit:   "e009d4ca4"
git-subrepo:
  version:  "0.4.3"
  origin:   "???"
  commit:   "???"
@bnavigator
import asyncio
e6b28cc
@bnavigator
remove removed .kernel.poll() calls
66ae4e0
@bnavigator
make testing with jupyter_client=7 permanent
eb9c9b8
@bnavigator
Copy link
Contributor Author

I think you should remove them and leave commit 46556ad as the last one on this PR, so that we continue our testing with Jupyter-client 7.

I am not sure if I understand you correctly. Is eb9c9b8 the way you intended it?

@ccordoba12
Copy link
Member

ccordoba12 commented Nov 1, 2021 •
edited
Loading

I am not sure if I understand you correctly. Is eb9c9b8 the way you intended it?

Yeah, that's idea. Since version 7 is so different from the previous ones, we need to be aware when new releases break us.

@bnavigator
Copy link
Contributor Author

Sure. But it means you definitely need spyder-kernels updated. And you probably want to use your own branch instead of the PR eventually.

@ccordoba12
Copy link
Member

I understand your concern now. That's why:

  1. We have a subrepo for spyder-kernels here, which is a copy of its repo that we sync with git for our respective branches here (2.x with 5.x and master with master). Our tests run with that subrepo, not with the installed version.
  2. We release a new version of spyder-kernels a day or two before releasing a new one of Spyder.

bnavigator
bnavigator commented Nov 1, 2021
View reviewed changes
external-deps/spyder-kernels/.gitrepo Outdated
branch = 2.x
commit = ac40350c9260e2e5d73f0fc3f8a9cc15da8d8a03
parent = 01421c2ae26c7a3861fc4c912f295f9f2b2c1c1f
branch = client7
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean, you probably don't want to merge while this is referenced.

Copy link
Member

@ccordoba12 ccordoba12 Nov 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Right, I'll merge your corresponding PR in spyder-kernels first, then ask you to resync our subrepo here, then merge.

The process is explained in our contributing guide.

@ccordoba12 ccordoba12 mentioned this pull request Nov 7, 2021
Closed
@ccordoba12 ccordoba12 added this to the v5.2.1 milestone Nov 7, 2021
@ccordoba12 ccordoba12 modified the milestones: v5.2.1, v5.2.0 Nov 22, 2021
@ccordoba12
git subrepo clone --branch=client7 --force https://github.com/bnaviga…
89d4e89 
…tor/spyder-kernels.git external-deps/spyder-kernels

subrepo:
  subdir:   "external-deps/spyder-kernels"
  merged:   "1d139d378"
upstream:
  origin:   "https://github.com/bnavigator/spyder-kernels.git"
  branch:   "client7"
  commit:   "1d139d378"
git-subrepo:
  version:  "0.4.3"
  origin:   "https://github.com/ingydotnet/git-subrepo"
  commit:   "2f68596"
@ccordoba12
Merge branch '5.x' into client7
d49b92a
@ccordoba12
Copy link
Member

ccordoba12 commented Nov 22, 2021 •
edited
Loading

@bnavigator, I updated your PR in spyder-kernels and this one in order to merge them for Spyder 5.2.0 (to be released on Wednesday). I was not planning to do that, but our tests in spyder-kernels are constantly failing now with jupyter_client 6.1.12, so this is a necessity.

Sorry for the rush and hope it's ok for you.

@ccordoba12
git subrepo clone (merge) --branch=2.x --force https://github.com/spy…
59b5fd3 
…der-ide/spyder-kernels.git external-deps/spyder-kernels

subrepo:
  subdir:   "external-deps/spyder-kernels"
  merged:   "ed74365a0"
upstream:
  origin:   "https://github.com/spyder-ide/spyder-kernels.git"
  branch:   "2.x"
  commit:   "ed74365a0"
git-subrepo:
  version:  "0.4.3"
  origin:   "https://github.com/ingydotnet/git-subrepo"
  commit:   "2f68596"

[ci skip]
ccordoba12
ccordoba12 approved these changes Nov 22, 2021
View reviewed changes
Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Thanks @bnavigator! I really appreciate your help with this.

@ccordoba12 ccordoba12 merged commit 1f6d44f into spyder-ide:5.x Nov 22, 2021
ccordoba12 added a commit that referenced this pull request Nov 22, 2021
@ccordoba12
Merge from 5.x: PR #16644
cad08b0 
Fixes #16762
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ccordoba12 ccordoba12 ccordoba12 approved these changes

Assignees

@bnavigator bnavigator

Labels
None yet
Projects
None yet
Milestone
v5.2.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@bnavigator @pep8speaks @ccordoba12