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

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

Merged
merged 18 commits into from
Oct 17, 2023
Merged

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

merged 18 commits into from
Oct 17, 2023

Conversation

yuvipanda
Copy link
Contributor

@yuvipanda yuvipanda commented Oct 17, 2023 •
edited
Loading

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
@yuvipanda
Migrate start-notebook.sh to bash
d1ff010 
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
@yuvipanda
Rename start-notebook.sh to start-notebook
ed70463
@yuvipanda
Cleanup start-notebook a little
00e751e
@yuvipanda
Fix typo
ea06768
@yuvipanda
Migrate start-singleuser as well
14a1e8c
@yuvipanda
Remove unused import
40f37d7
@yuvipanda
Run symlink commands as root
3364294
@yuvipanda
Combine repetitive RUN commands
bb98d5e
@yuvipanda
Remove multiple args to env
9e71839 
-u can not be set by shebang, we must set the env var
instead
@yuvipanda @mathbunnyru
Fix conditional inversion
67302e3 
Co-authored-by: Ayaz Salikhov <[email protected]>
@yuvipanda
Fix how start-singleuser is exec'd
1804a8c
@yuvipanda
Actually call jupyterhub-singleuser in start-singleuser
527b756
@yuvipanda
Pass through any additional args we get
9f7bdb3
@yuvipanda
Put .py suffix on the start-* scripts
37d5b15
@yuvipanda
Add .sh shims for the start-* scripts
81c67ef
@yuvipanda
Document start-notebook.sh and start-singleuser.sh
219cf36
@yuvipanda
Partially test start-notebook.sh
d6519ac
@yuvipanda
Copy link
Contributor Author

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

@yuvipanda @mathbunnyru
Reflow warning docs
99410dc 
Co-authored-by: Ayaz Salikhov <[email protected]>
mathbunnyru
mathbunnyru reviewed Oct 17, 2023
View reviewed changes
images/base-notebook/start-notebook.py
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 yuvipanda mentioned this pull request Oct 17, 2023
Closed
@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

@benz0li benz0li mentioned this pull request Oct 23, 2023
Closed
@chris-bateman chris-bateman mentioned this pull request Oct 31, 2023
Open
9 tasks
@mathbunnyru mathbunnyru mentioned this pull request Dec 2, 2023
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mathbunnyru mathbunnyru mathbunnyru approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve logging in start.sh
3 participants
@yuvipanda @mathbunnyru @manics