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.

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

Closed
48 tasks done
Zsailer opened this issue Jun 4, 2019 · 20 comments
Closed
48 tasks done

Sync Notebook and Jupyter Server. #53

Zsailer opened this issue Jun 4, 2019 · 20 comments

Comments

@Zsailer
Copy link
Member

Zsailer commented Jun 4, 2019

Opening this issue to track the porting of relevant Notebook PRs to Jupyter Server.

List to Port:

My strategy is to just manually move PRs that affect files in the Server. Is that the best strategy here? @SylvainCorlay @blink1073

@blink1073
Copy link
Contributor

As a last resort, sure. I'm wondering if Sylvain had some git-foo that he used last time. 😀

@Zsailer
Copy link
Member Author

Zsailer commented Jun 4, 2019

@SylvainCorlay, Sensei, teach us your git-foo ways! 😃

@kevin-bates
Copy link
Member

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.

@Zsailer
Copy link
Member Author

Zsailer commented Jun 8, 2019

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...

@lresende
Copy link
Member

lresende commented Jun 8, 2019

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.

@SylvainCorlay
Copy link
Contributor

I don't think that there is much magic we can do besides manually porting the PRs at this point.

@Zsailer
Copy link
Member Author

Zsailer commented Jun 11, 2019

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.

@Zsailer
Copy link
Member Author

Zsailer commented Jun 11, 2019

Also, if anyone wants to port a PR, go ahead! 😄 I'll start working through this list, but any help is appreciated.

@Zsailer
Copy link
Member Author

Zsailer commented Jun 11, 2019

Also, here's a list of notebook PRs that were merged after @SylvainCorlay did the initial fork. Many of these PRs are frontend specific.

@Zsailer
Copy link
Member Author

Zsailer commented Sep 24, 2019

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

  1. cherry-pick each of these changes in one major PR.
  2. starting tracking notebook repo for server-side changes.
  3. manually port PRs to server as they land in notebook.

Any better strategies?

@kevin-bates
Copy link
Member

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.

@Zsailer
Copy link
Member Author

Zsailer commented Sep 25, 2019

chunk things up given they were independent changes previously

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.

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.

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!

@kevin-bates
Copy link
Member

@Zsailer - I've updated the comment to indicate those gateway-related PRs handled in #92. There's also one gateway-related commit from #4449 that you'll find already done (be35a37).

@Zsailer
Copy link
Member Author

Zsailer commented Sep 25, 2019

Question for the group—should we squash merge the ported PRs? Or should we retain the full commit history?

@lresende
Copy link
Member

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 rebase and merge which would maintain the history.

@kevin-bates
Copy link
Member

I agree. I think moving forward, we should try to adopt a squash and merge approach for multi-commit PRs. But, as Luciano says, since these are essentially multi-PR PRs, where each PR is potentially multiple commits, I think it makes sense to not squash in this case.

@Zsailer
Copy link
Member Author

Zsailer commented Sep 25, 2019

Great, thanks everyone. Check out #96 for our plan to move forward.

@vidartf
Copy link
Member

vidartf commented Sep 26, 2019

I'm mostly against squashing due to it making git blame (especially here in github) a lot harder to navigate. However, I'm for it under these circumstances:

  • The individual commits change some code, then changes it back to the original value (reversion). In these cases squashes make for a cleaner history.
  • The total line changes are small (implying that it doesn't conflate changes that are logically different).

@Zsailer
Copy link
Member Author

Zsailer commented Sep 27, 2019

🎉 🎉 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.

@kevin-bates
Copy link
Member

Thanks for the great work Zach! Time to rock this thing! 🚀

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

No branches or pull requests

6 participants