-
Notifications
You must be signed in to change notification settings - Fork 147
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
Integrating native-proxy #501
base: main
Are you sure you want to change the base?
Conversation
for more information, see https://pre-commit.ci
Hi @jwindgassen , thanks for this contribution! Is the idea to replace jhsingle-native-proxy (so that code can be shared), or will that project continue on? If the latter then could there still be duplication of code? (will this need to periodically get resynced with jhsingle-native-proxy?) Or should shared code be factored out into something separate for each project to use? If there is documentation on using this, could that be integrated as well? |
That is a good question. I do not know about the current state of the original project. The newest commit was already made a few months ago, but I do not think it is abandoned. I will contact the original developer soon and ask him about the current status and his opinion on this. But the proxies used by it are almost identical to the proxies here, and (I think) they originally were a copy of an old version of the ones in this package. The Documentation for it is currently in the ReadMe, but could be added to the docs here, given that we decide to add this feature. |
for more information, see https://pre-commit.ci
…tion and check_origin
This sounds like a great idea. I would suggest to create some kind of checklist of the features that are ported from jhsingle-native-proxy and what's not during the course of this pull request (or a series of pull requests). We're using jhsingle-native-proxy heavily in jhub-apps would love to move to just using |
@ryanlovett I talked to the original developer. He welcomes the merging of his feature into Jupyter Server Proxy. There are currently no further plans for @aktech Sure. Here is a list with the features I have currently ported or plan to do so:
That's at least everything I can think of now. If you have other ideas or require something more, let me know and I will add it to the list :) |
@jwindgassen Thanks very much for asking! It sounds like this has the potential to consolidate development in the future. |
for more information, see https://pre-commit.ci
The `oauth_callback` requests were handled by the ProxyHandler, effectively causing the request to ping-pong between JupyterHub Login and `/oauth_callback`
@ryanlovett @aktech I have been working on the feature over the last few weeks, and I am now happy to announce, that the code is working 🎉 I'm welcoming anyone to test the feature on their JupyterHub instance for testing. Please let me know about any problems or errors you encounter when doing so 🙂 How to useFor testing, I like to use voila. The command to execute might look like this: What else do you need from my side, besides the code, to be satisfied with this PR? I am currently writing a page for the docs, which I will commit soon. |
@jwindgassen Thanks for the update! I'll try to test this week locally. How might a hub administrator typically configure use of this feature? For example would they set every user to launch voila via standalone proxy from Is the intent of standalone to essentially re-use the hub's auth, spawner, user storage, etc. but limit what apps users can invoke because it specifies just the one? (since jupyter server + jupyter-server-proxy enables users to launch an arbitrary number of proxied apps) |
In essence, yes. But since you need to overwrite |
Fascinating, thanks! More of an aside, but how are you customizing the spawner to launch the different applications? Edit: oh, is it jhub-apps as mentioned earlier? |
No. We have created our own custom Spawner. But is similar to this. We have overwritten the start method, which will submit a new Job to our cluster. And inside the start script, we start jupyterhub singleuser at the end. But now you mention it, jhub-apps might synergyze quite well with the standalone feature. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The command line arguments are quite complicated, e.g. having to parse maps.
The alternative is more work, but if we were to refactor ServerProcess to be a Traitlets Configurable
jupyter-server-proxy/jupyter_server_proxy/config.py
Lines 31 to 49 in 41e588a
ServerProcess = namedtuple( | |
"ServerProcess", | |
[ | |
"name", | |
"command", | |
"environment", | |
"timeout", | |
"absolute_url", | |
"port", | |
"unix_socket", | |
"mappath", | |
"launcher_entry", | |
"new_browser_tab", | |
"request_headers_override", | |
"rewrite_response", | |
"update_last_activity", | |
"raw_socket_proxy", | |
], | |
) |
I think we could take advantage of Traitlets ability to automatically create all CLI args, and we'd have the benefit of being able to configure jupyter-standaloneproxy using an arbitrarily complex file.
What do you think? I'm happy to investigate converting ServerProxy to Traitlets.
"command", nargs="+", help="The command executed for starting the WebApp" | ||
) | ||
|
||
args = parser.parse_args() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a mismatch between the options supported by Jupyter Server Proxy
https://github.com/jupyterhub/jupyter-server-proxy/blob/v4.4.0/jupyter_server_proxy/config.py#L34-L47
and those available here. Obviously some options aren't relevant, but for example raw_socket_proxy
is.
Thanks for the suggestion, I really like that idea. This should also solve the issue with keeping CLI Arguments up with any new options added to the proxies. I will look into it :) |
I've made a start in #507 |
So I used your branch to create the CLI via traitlets. I'm no expert with traitlets, but I managed to get it working: jwindgassen@2d9eb5b I had to play around with aliases and the extra_args a bit, but I wanted to keep the CLI reasonably unchanged to before. If you have a better idea or otherwise comments on my changes over there, let me know! :) |
Sorry for the delay. Since this is a new addition to Jupyter server proxy I think we should prioritise long term maintenance over just replicating the previous CLI- if there's a better way to do things we can use it as an opportunity to refactor. We also don't need to do everything in one go, for example it's fine to initially focus on creating a functional standalone proxy along that only supports standard traitlets configuration, and add additional flags in a follow-up PR. |
I'm fine with how the CLI looks right now, so I'm happy to switch to this once your PR has been accepted. I am also almost done with Tests and Docs, they should be ready by the end of the week. |
for more information, see https://pre-commit.ci
Hey @jwindgassen Thanks a lot for working on this and for the ping. I'll play with it this week to provide some feedback. |
1f8855b
to
e4741d0
Compare
Ok, I have now also added Docs and Tests for the new feature. Writing proper tests for Login and Activity is a bit more difficult, since I would need to spawn a JupyterHub instance to gain full access to its API. For now, I limited it to only testing our code, since the classes I import from JupyterHub are tested over there. I also added a section to the docs, mostly targeted to developers, where I explain the different sections of the code and what features I needed to implement to make this work smoothly. If you think we need more tests for specific cases or want something to be explained in the docs in more detail, let me know. |
Hey everyone.
I was recently made aware, of the desire to be able to create standalone proxies, similar to how it is done in jhsingle-native-proxy (see #1).
This would be immensely advantageous for us, so I started porting the code here recently.
There is still a lot to do and I needed to remove/comment out a few of the original features, but it is already fundamentally working as is. I am opening this PR to let you know of this and get an opinion on a few bits here and there. I will continue to improve on it in the next weeks.
In the meantime, any comments and ideas are welcome :)