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

[FAST_BUILD] Migrate runhooks to python #2007

Closed
wants to merge 40 commits into from

Conversation

yuvipanda
Copy link
Contributor

@yuvipanda yuvipanda commented Oct 17, 2023

This can't be merged without also migrating start.sh, but opening this primarily to
show the implementation of source. I think that is the biggest barrier to migrating start.sh
to python, and this approach solves it quite neatly - am pretty proud of it :D

You can test it by creating a folder like this:

test
test/01-set-variable.sh
test/02-get-variable.sh

And the contents of the files are:

$ cat test/01-set-variable.sh 
export HELLO=WORLD
$ cat test/02-get-variable.sh 
echo "What is the HELLO to? It is to $HELLO"

And then run:

$ ./run-hooks.py test
Running hooks in: test as 501 gid: 20
Sourcing shell script: test/01-set-variable.sh
Sourcing shell script: test/02-get-variable.sh
What is the HELLO to? It is to WORLD
Done running hooks in: test

you can see that 01-set-variable exports a variable, which is then read by 02-get-variable!

We can turn this into a unit test later

This is built on top of #2006, see yuvipanda/docker-stacks@start-notebook-python...yuvipanda:docker-stacks:runhooks-py for just the run-hooks change

yuvipanda and others added 18 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 <[email protected]>
- Primarily, implement the `source` method to emulate behavior
  of source command in bash
- This won't actually be effective until start.sh is rewritten as
  well
@yuvipanda
Copy link
Contributor Author

@mathbunnyru take a look at this for the source implementation :) To me this was the part of start.sh that needed solving, and I think this does a pretty good job. What do you think of this approach?

@mathbunnyru
Copy link
Member

That's a nice trick, and I love it!
Fortunately, I added testing for run-hooks only a couple months ago, so as longs as the tests are ok, we should be fine 🙂
I have a few ideas and will write them, when we merge previous PR and this one will be easier to read/comment/change.

@mathbunnyru
Copy link
Member

mathbunnyru commented Oct 17, 2023

@yuvipanda I merged the previous PR, please rebase/merge/create new PR for start.sh and run-hooks.py (whichever you choose and makes git diff nice and small is ok for me).

@yuvipanda
Copy link
Contributor Author

@mathbunnyru I've merged that in here.

Since start.sh uses source to call run-hooks, we will have to migrate start and run-hooks together. This PR migrates run-hooks, but of course start has more going on. I'll try work on it later this week, but next week is more likely.

@yuvipanda
Copy link
Contributor Author

But do tell me your ideas, I'll revisit here wherever possible

@manics
Copy link
Contributor

manics commented Oct 17, 2023

What happens with bash functions that are defined in one hook and used in another?

@mathbunnyru
Copy link
Member

I agree with @yuvipanda on @manics issue.

Generally speaking, we haven't promised the order in which shell scripts are sourced/called (the current implementation is in order and tests use this though).
I think, if you have a.sh and b.sh, it's not that difficult to move the function to some common shell file and source it from both a.sh and b.sh. And, to be honest, I think there are not so many people using this.

@mathbunnyru
Copy link
Member

mathbunnyru commented Oct 17, 2023

@yuvipanda also, when you will be porting start.sh, take a look at plumbum.
We already use it in this repo for testing and tagging, so it won't increase the number of technologies used in this repo.

This package is not added in the images, but I am pretty sure it's tiny and without lots of additional dependencies.
And it makes calling executables easier and more pretty-looking in Python.

@yuvipanda
Copy link
Contributor Author

I'm pacing myself on this one, and spent another 15 minutes on it today addressing some of the review feedback :) start.sh is the big one and to start next, but will take it slow.

@mathbunnyru mathbunnyru changed the title Migrate runhooks to python [FAST_BUILD] Migrate runhooks to python Nov 4, 2023
@mathbunnyru
Copy link
Member

Added a test for unset: #2022

@mathbunnyru
Copy link
Member

Added a test for changing variable value: #2023

@@ -1,46 +1,6 @@
#!/bin/bash
# Copyright (c) Jupyter Development Team.
# Distributed under the terms of the Modified BSD License.
# echo "WARNING: Use run-hooks.py instead"
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, we have to comment this line, otherwise, tests fail, because they do not expect warnings.

@mathbunnyru
Copy link
Member

@yuvipanda I added some tests for run-hooks and rewrote the test file a little bit, to make adding new tests easier.
I believe these tests will make merging this PR easier (when you are done with start.sh).
I only added tests that should work after this PR as well (so, no functions), but now we check unset works and double export with value change also works fine.

@mathbunnyru
Copy link
Member

@yuvipanda do you have plans to continue working on this? 🙂

@mathbunnyru
Copy link
Member

Closing this PR, as there is no activity here

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.

3 participants