-
Notifications
You must be signed in to change notification settings - Fork 18
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
setup: migrate to jupyter server 2 #450
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #450 +/- ##
==========================================
+ Coverage 76.96% 77.25% +0.28%
==========================================
Files 12 12
Lines 1385 1407 +22
Branches 236 236
==========================================
+ Hits 1066 1087 +21
- Misses 276 277 +1
Partials 43 43
☔ View full report in Codecov by Sentry. |
Any chances this will be merged soon? We would like to run UI server as a JupyterLab extension in our JupyterHub deployment and the current constraint on |
Yes, it's ready for review now, and not a big change. It should be merged pretty soon. |
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.
Looks good at first glance. I haven't tested it yet.
@mahendrapaipuri, we should be able to get this reviewed soonish, however, this feature is scheduled for release along with Cylc 8.2.0 so won't be released until some other work is completed. If you want to get going with Jupyter Server 2 and Cylc 8.1, we might be able to provide you with a development branch you could install from to save waiting. Let us know if that would help. |
Cheers @oliver-sanders!! Well, we are not in a rush. I will keep track of this PR and once it gets merged I can test using the |
Great, just be aware you'll need to install cylc-flow from master too. |
0f45005
to
1446760
Compare
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.
Still need to update to >=3.8
Or should it be >=3.8,<3.10
?
Line 43 in 1446760
python_requires = >=3.7 |
sorry, didn't read properly, done. |
1446760
to
c49bb31
Compare
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.
LGTM, I've done a basic test of running the UI after pip installing on this branch
|
[Update] Apologies, this is a little more complex than it initially looked due to changes in authorisation and jps 2.7. |
32936f7
to
621c482
Compare
Ok, this is now looking ok but still needs a little work. This PR now upgrades us, both to Jupyter Server v2 and to newer versions of Jupyter Hub which has required some re-working of the authorisation which will need to be carefully examined and tested. The authorisation model requires recent versions of both for multi-user access, but I haven't flushed out the full ranges yet. The good news is that recent Jupyter Hub versions have added a scope to authorise the starting of other users servers so this improves multi-user functionality greatly. Along with the documentation PR:
TODO:
|
621c482
to
edcdd28
Compare
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.
Done another review. No comments at the moment, pending changes
edcdd28
to
20c0149
Compare
Got the tests passing. It wasn't possible to keep the unauthenticated tests because the jupyter_server test framework token authenticates all requests, turning this off requires in-depth hacking and doesn't really test the implementation properly. The unauthorised userprofile test has been removed as the userprofile no longer requires authorisation since it only gives you information about yourself. |
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.
Tested cylc gui
and cylc hub
, both working
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.
LGTM, tested as working.
Closes #411
Migrate from Jupyter Server 1-2.
There has been a rejig of the
get_current_user
logic so this should be accompanied by a security check of authorisation/authentication.Check List
CONTRIBUTING.md
and added my name as a Code Contributor.setup.cfg
(andconda-environment.yml
if present).CHANGES.md
entry included if this is a change that can affect users?.?.x
branch.