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

Support HTTPS connection to Jupyter notebook #144

Merged
merged 3 commits into from
Jul 29, 2022

Conversation

danielhollas
Copy link
Contributor

@danielhollas danielhollas commented Jul 25, 2022

Basic aiidalab container comes with HTTP, but users might want to add HTTPS.
Here we just modify the code that tries to connect to Jupyter Notebook to determine whether it is ready.

I am currently playing with HTTPS for local development in our Docker image built on top of AiiDAlab-docker-stack, see ispg-group/aiidalab-ispg-docker-stack#6 if that is of interest.

TODO:

  • Tests. Seems like it would be tricky to test this properly, we would somehow need to patch the Jupyter notebook in a running container? Not sure it's worth investing too much time. In any case, to enable HTTPS, one simply needs to pass the key and certificate to jupyter-notebook in /opt/start-notebook.sh, see the PR linked above.
  • When the start command finishes, we still print http url, and if user tries to open the browser directly from cmdline it will open http and fail. I didn't figure out how to improve this since the polling function is an async @staticmethod so I did not not how to cleanly pass info from it without larger refactor.

Basic aiidalab container comes with HTTP,
but users might want to add HTTPS.
Here we just modify the code that tries to connect to
Jupyter Notebook to determine whether it is ready.
@codecov
Copy link

codecov bot commented Jul 25, 2022

Codecov Report

Merging #144 (c385ec5) into main (621f4ce) will decrease coverage by 0.75%.
The diff coverage is 55.55%.

@@            Coverage Diff             @@
##             main     #144      +/-   ##
==========================================
- Coverage   87.03%   86.27%   -0.76%     
==========================================
  Files           9        9              
  Lines         879      889      +10     
==========================================
+ Hits          765      767       +2     
- Misses        114      122       +8     
Flag Coverage Δ
py-3.10 86.16% <55.55%> (-0.76%) ⬇️
py-3.7 85.69% <52.94%> (-0.78%) ⬇️
py-3.8 86.11% <55.55%> (-0.76%) ⬇️
py-3.9 86.23% <55.55%> (-0.76%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
aiidalab_launch/__main__.py 79.41% <ø> (ø)
aiidalab_launch/instance.py 87.40% <55.55%> (-2.60%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 621f4ce...c385ec5. Read the comment docs.

@csadorf
Copy link
Member

csadorf commented Jul 26, 2022

The aiidalab-launch tool is designed for local invocation not necessarily for serving the service via a proxy over the wider network. I must admit that I am having a bit of a hard time following the use case, maybe you can elaborate a little bit?

Either way, I have no problem with accepting a slight patch to the tool if it unblocks the effort as long as it does not increase the complexity disproportionally.

Is this PR ready for review or comment or are you still experimenting? In the latter case I'd suggest to change it into draft stage, otherwise, please request my review and I will have a closer look at the implementation.

@danielhollas
Copy link
Contributor Author

@csadorf this is ready for review.

Well, having https in local environment is good for testing it out, and also when using mkcert and issuing your own root certificate you can completely get away from the annoying browser warning when you're accessing http site (on Firefox I am using the strict only-http mode).

Besided local development, I have another server where I am essentially serving Aiidalab application within our VPN for members of our group. Here again having HTTPS is very much desirable. Also I am not yet ready to dive into full-blown production deployment so using aiidalab-launch makes sense for me. Let me know if I can clarify further.

@csadorf csadorf self-requested a review July 27, 2022 06:44
Copy link
Member

@csadorf csadorf left a comment

Choose a reason for hiding this comment

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

It looks this PR introduces a few unrelated changes to the logging behavior and verbosity. Would you mind removing those or alternatively justify why they should be part of this change set? Thank you!

@danielhollas
Copy link
Contributor Author

Yes, sorry about that. I added justification in comments but am happy to submit those in separate PR.

However, there is no "functional" change to logging behaviour, I just changed a logging level of a couple of statement to make the '-vv' level more useful.

@csadorf
Copy link
Member

csadorf commented Jul 27, 2022

Yes, sorry about that. I added justification in comments but am happy to submit those in separate PR.

However, there is no "functional" change to logging behaviour, I just changed a logging level of a couple of statement to make the '-vv' level more useful.

Ok, fair enough. Feel free to leave the adjustments in the PR.

@csadorf csadorf self-requested a review July 27, 2022 14:53
aiidalab_launch/instance.py Outdated Show resolved Hide resolved
aiidalab_launch/instance.py Outdated Show resolved Hide resolved
aiidalab_launch/instance.py Outdated Show resolved Hide resolved
aiidalab_launch/instance.py Outdated Show resolved Hide resolved
@csadorf
Copy link
Member

csadorf commented Jul 28, 2022

  • Tests. Seems like it would be tricky to test this properly, we would somehow need to patch the Jupyter notebook in a running container? Not sure it's worth investing too much time. In any case, to enable HTTPS, one simply needs to pass the key and certificate to jupyter-notebook in /opt/start-notebook.sh, see the PR linked above.

You wouldn't need to patch a running container, we could adjust it before the start. But I agree, this is esoteric enough that the effort is likely not justified. If you come up with a good solution to test it, I'd of course welcome it.

  • When the start command finishes, we still print http url, and if user tries to open the browser directly from cmdline it will open http and fail. I didn't figure out how to improve this since the polling function is an async @staticmethod so I did not not how to cleanly pass info from it without larger refactor.

I'd suggest to simply return it and then toggle an attribute on the AiidaLabInstance class. If you like I can implement that, just let me now.

@danielhollas
Copy link
Contributor Author

I'd suggest to simply return it and then toggle an attribute on the AiidaLabInstance class. If you like I can implement that, just let me now.

I didn't know how to return from an async function, but it was quite easy in the end. 😄 We now print the correct protocol both at the and of the start command and also in the output of the status command. 🎉

image

@csadorf csadorf merged commit 3d3f8e9 into aiidalab:main Jul 29, 2022
@danielhollas danielhollas deleted the https-support branch July 29, 2022 11:17
@csadorf
Copy link
Member

csadorf commented Jul 29, 2022

@danielhollas Thanks for the contribution! I just released this feature in https://github.com/aiidalab/aiidalab-launch/releases/tag/v2022.1015 .

@danielhollas
Copy link
Contributor Author

Thanks so much @csadorf ! : 💖

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.

None yet

2 participants