-
Notifications
You must be signed in to change notification settings - Fork 315
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
Sync Notebook and Jupyter Server. #53
Comments
As a last resort, sure. I'm wondering if Sylvain had some git-foo that he used last time. 😀 |
@SylvainCorlay, Sensei, teach us your git-foo ways! 😃 |
Assuming this is the place to talk about NB PRs we want in server.. The Gateway related PRs are needed: jupyter/notebook#4161, jupyter/notebook#4431 and the yet-to-be-merged jupyter/notebook#4576. These will enable the ability to manage kernels on computer cluster edge nodes, separated from the notebooks themselves. I also believe jupyter server should introduce async kernel management, so (minor, but merged) PR jupyter/notebook#4412 should probably be taken, but we should strongly consider the yet-to-be-merged PR jupyter/notebook#4479 (which depends on jupyter client PR jupyter/jupyter_client#428). These last two enable the ability to change management class hierarchies from sync to async and I would propose we have the default be async (the PR uses sync as the default since it was targeting Notebook). Actually, this topic should probably become a separate issue. If necessary, I'd be happy to help triage various notebook PRs for their applicability into server. Thanks. |
Absolutely. I will definitely make sure to port these PRs over. I think the intention is to port anything that touches server-side code in the notebook. Before you spend too much time triaging notebook PRs, let's see if @SylvainCorlay has any tricks up his sleeve... |
I believe @SylvainCorlay mentioned in the workshop that @Carreau has a tool that helps identifies all prs that touch a set of files or something like that which could help us identify the changes that need to be moved. |
I don't think that there is much magic we can do besides manually porting the PRs at this point. |
Okay, I think it's time to start building a list of PRs to port. I'll edit the top post with a running checklist. If anyone wants to begin triaging PRs in notebook and listing them here, I'll update the checklist as we go. |
Also, if anyone wants to port a PR, go ahead! 😄 I'll start working through this list, but any help is appreciated. |
Also, here's a list of notebook PRs that were merged after @SylvainCorlay did the initial fork. Many of these PRs are frontend specific. |
I've updated the top comment to include the full list of merged PRs in notebook that touch server_side code. Many of the changes are small. What's the best strategy to move forward? My plan was to
Any better strategies? |
Thanks Zach. I think steps 2 and 3 are fine. Step 1 is probably the real question. One massive PR seems like it could be a nightmare if issues arise. I'm wondering if it would be better to chunk things up given they were independent changes previously. Perhaps a "series" of 3 to 5 PRs? Would that approach be helpful for checkpointing progress? I'm curious if you found changes that spanned into what would be the "nb classic" server extension? Btw, the various Gateway PRs are addressed in #92. I'll update the comment tomorrow - checking the applicable entries (with a reference to #92) if you're okay with that. |
I think you're right. Let's break it up. I'm planning to move through these pretty fast, so I'll open multiple batches of PRs. I didn't track any PRs that were nbclassic specific. I'll probably rebase jupyter/notebook#4653, allowing us to start fresh on tracking PRs for nbclassic.
Great! #92 is what inspired me to build the above checklist. I'd love to stay on top of notebook PRs moving forward. Thanks for doing this great work! |
Question for the group—should we squash merge the ported PRs? Or should we retain the full commit history? |
I personally really don't like the option to see the PR history in master, it makes it very messy, particularly as a lot of people add committs to a pr like 'fix typo' or 'another try'... Also, if someone is really interested in the pr lifecycle, comments, history, this is still available on the pr. Having said that, in this particular case, this is cherry-picking multiple commits, so I would just |
I agree. I think moving forward, we should try to adopt a |
Great, thanks everyone. Check out #96 for our plan to move forward. |
I'm mostly against squashing due to it making
|
🎉 🎉 I think we've successfully synced jupyter_server with the classic notebook server (huge thank you to @kevin-bates for reviewing these PRs)! 🎉 🎉 I'll be tracking notebook to make sure we port PRs here as they land in notebook. |
Thanks for the great work Zach! Time to rock this thing! 🚀 |
Opening this issue to track the porting of relevant Notebook PRs to Jupyter Server.
List to Port:
get_secure_cookie
kwargs jupyter/notebook#3778allow_remote_access
when ip is '*' jupyter/notebook#4139FilesHandler.get()
a decorated coroutine. Closes #4869 jupyter/notebook#4891My strategy is to just manually move PRs that affect files in the Server. Is that the best strategy here? @SylvainCorlay @blink1073
The text was updated successfully, but these errors were encountered: