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

Migrate start-notebook & start-singleuser to python #2006

Merged
merged 18 commits into from
Oct 17, 2023

Conversation

yuvipanda
Copy link
Contributor

@yuvipanda yuvipanda commented Oct 17, 2023

Based on

Stop using bash, haha 👍

from #1532.

If there's more apetite for this, I'll try to migrate start.sh and start-singleuser.sh as well - I think they should all be merged together. We can remove the .sh suffixes for accuracy, and keep symlinks in so old config still works. Since the shebang is what is used to launch the correct interpreter, the .sh doesn't matter.

Will help fix #1532, as I believe all those things are going to be easier to do from python than bash

TODO

  • Add .py extensions to the script
  • Add shim .sh files that just redirect us back
  • Document these .sh scripts

@yuvipanda
Copy link
Contributor Author

@mathbunnyru I wanted to ask if there's apetite for this kinda change before I put more work into this :)

@yuvipanda yuvipanda changed the title Migrate start-notebook.sh to bash Migrate start-notebook & start-singleuser to python Oct 17, 2023
@yuvipanda
Copy link
Contributor Author

I'll take a look at the test failures if this kinda migration is acceptable! I'm sure I can sort those out if needed.

@mathbunnyru
Copy link
Member

I like this PR a lot, just a few thoughts about the implementation:

  1. Let's add the .py extension to the added start-like scripts. It doesn't change much, but some utils (maybe pre-commit hooks) and editors definitely work better (at least not worse).
  2. Instead of symlinking to keep compatibility, I propose having simple shell files, which first echo a warning that calling is file is deprecated and then simply pass all the params to the corresponding .py file. This way we will be able to first replace a warning with a hard error in a few months and then completely delete the .sh files.
  3. Let's also document the warning about calling old .sh start scripts.
  4. I really hope you will have the courage to deal with start.sh when we merge this PR 🙂

Overall, great PR, thank you!

@yuvipanda yuvipanda force-pushed the start-notebook-python branch 2 times, most recently from c4f46de to 15f6833 Compare October 17, 2023 10:46
@yuvipanda yuvipanda marked this pull request as ready for review October 17, 2023 11:01
@mathbunnyru
Copy link
Member

I updated your branch (to make sure we test latest versions of everything).

@yuvipanda
Copy link
Contributor Author

Glad to hear you like it, @mathbunnyru! I've addressed all the points you mentioned and I think this is ready for review now.

I do personally prefer leaving out the .py suffix, but am alright leaving it in. I find that most tools are ok looking at the shebang for detection in those cases, but I'm sure that's not 100% of them.

I'm definitely interested in tackling start.sh as well! I actually wrote https://github.com/yuvipanda/jupyterhub-roothooks/ a while ago that provides some part of that functionality, so there's gonna be stuff for me to steal from there. I also spent today picking up a plan to replicate as exactly as possible bash's source semantics in python and I think I've a path there.

My goal would be to do this in such a way that all the existing tests pass, so no new functionality.

yuvipanda and others added 17 commits October 17, 2023 16:35
Based on

> Stop using bash, haha 👍

from jupyter#1532.

If there's more apetite for this, I'll try to migrate
`start.sh` and `start-singleuser.sh` as well - I think they should
all be merged together. We can remove the `.sh` suffixes for
accuracy, and keep symlinks in so old config still works. Since
the shebang is what is used to launch the correct interpreter,
the `.sh` doesn't matter.

Will help fix jupyter#1532,
as I believe all those things are going to be easier to do from
python than bash
-u can not be set by shebang, we must set the env var
instead
Co-authored-by: Ayaz Salikhov <mathbunnyru@users.noreply.github.com>
@yuvipanda
Copy link
Contributor Author

@mathbunnyru hah, we clashed as I was just about to rebase :)

Co-authored-by: Ayaz Salikhov <mathbunnyru@users.noreply.github.com>
import shlex
import sys

# If we are in a JupyterHub, we pass on to `start-singleuser.py` instead so it does the right thing
Copy link
Member

Choose a reason for hiding this comment

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

I appreciate it that you added comments to this file! I'm sure it will help future contributors to understand this file faster.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mathbunnyru you're welcome :)

@manics
Copy link
Contributor

manics commented Oct 17, 2023

I'm definitely interested in tackling start.sh as well! I actually wrote https://github.com/yuvipanda/jupyterhub-roothooks/ a while ago that provides some part of that functionality, so there's gonna be stuff for me to steal from there. I also spent today picking up a plan to replicate as exactly as possible bash's source semantics in python and I think I've a path there.

If you're modifying the behaviour of the start scripts #1528 might fit in too.

@yuvipanda
Copy link
Contributor Author

@manics I'm probably going to just try do a verbatim port, and leave the behavior modification to others in the future. Hopefully the fact that it's in python and not bash will make that easier.

@mathbunnyru mathbunnyru merged commit bceaead into jupyter:main Oct 17, 2023
56 checks passed
@yuvipanda
Copy link
Contributor Author

Yay, thank you for the quick review and merge, @mathbunnyru. And for the long set of tests that made this easy

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.

Improve logging in start.sh
3 participants