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

Jupyter server #230

Merged
merged 29 commits into from
Aug 31, 2021
Merged

Jupyter server #230

merged 29 commits into from
Aug 31, 2021

Conversation

oliver-sanders
Copy link
Member

@oliver-sanders oliver-sanders commented Jul 29, 2021

closes #219, #218, #45
unblocks #137
addresses #171
siblings: cylc/cylc-flow#4331, cylc/cylc-ui#711

I think this also closes (testing required to confirm):

Summary & Highlights

  • 🚀 Jupter Server replaces our bespoke Tornado server.
  • 😄 Nice simplification of UIS codebase.
  • 🎁 This means we get lots of lovely Jupyter goodies and tighter integration into Jupyter land.
  • 💪 Jupyter servers can run multiple Jupyter Server extensions (e.g. you can run the Cylc UIS and Lab on the same server).
  • 🤗 This opens up the possibility of tighter integration between the Cylc UIS and Juoyter Lab.
  • 🧑‍🤝‍🧑 We now have two modes, single and multi user.
    • 🧑‍🦱 single-user uses token authentication, no authorisation, no need to run the hub.
    • 🧑‍🤝‍🧑 multi-user functionality provides authorisation, requires the hub.
  • 🧮 There is the potential to wrap the single-user mode a a native-like application using electron or similar.

CLI

Everything can be run using the jupyter commands:

# launch standalone UIS (single-user mode)
# [automatically sources Cylc configuration files]
jupyter-cylc

# launch the hub (multi-user mode)
# [you must point hub at the Cylc configuration files or edit your user/site Jupyter config]
jupyterhub

# the command the hub spawns
# [automatically sources Cylc configuration files]
jupyter-cylchubapp

Existing jupyter server installations can be configured to run the Cylc UIS extensions, example config:

c.ServerApp.jpserver_extensions = {
        "cylc.uiserver": True,
}

For tighter integration with Cylc, improved usability for those unfamiliar with the Jupyter ecosystem and command whitelisting purposes there are Cylc equivalents to these commands:

# all Cylc commands source the cylc configurations out of the box

# launch single-user
cylc gui

# launch multi-user
cylc hub

# command the hub spawns
cylc hubapp

Caveats

  • jupyter_server: use server to serve static pages #231
    [edit] now resolved!

    Issue to do with redirects from static endpoint (e.g. the UI) to the hub which are largely broken on this branch at the moment.

    This has the side-effect that it makes it look like you can access UI Servers you are not authorised to simply because the web application loads without any login redirect or authorisation warning. However, the data endpoints are all protected.

  • Should get_secure_cookie set token_authenticated property? jupyter-server/jupyter_server#566
    [edit] now partially resolved, if the user is a string but not "anonymous" then it is a token.

    We are pinned to jupyter_server==1.10.1 until this issue is resolved as in order to determine when a connection is token authenticated we are currently relying on a bug which has since been fixed.

Follow On

Misc

Ditch the setup.py file, we can do everything we need using setup.cfg which is much nicer 🎉, no more arbitrary code execution! We should copy this change to our other repos in due course.

Also adopted the new(ish) standard pyproject.toml file.

Requirements check-list

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Appropriate tests are included (unit and/or functional).
  • Appropriate change log entry included.
  • Docs: architecture reference docs cylc-doc#285
  • Created an issue at cylc-uiserver conda-forge repository with version changes (if you changed dependencies in setup.py, see recipe/meta.yaml).

* convert UIS to a jupter server extension application
* put extension behind cylc app
* deal with namespace package issues
* allow args to be passed through to jupyter_server
* this is for the workflow owner not the authenticated user
* tighter integration with jupyter_server
* less bespoke static logic
@codecov-commenter
Copy link

codecov-commenter commented Jul 29, 2021

Codecov Report

Merging #230 (65be593) into master (47096f4) will increase coverage by 1.98%.
The diff coverage is 83.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #230      +/-   ##
==========================================
+ Coverage   73.32%   75.30%   +1.98%     
==========================================
  Files          10       11       +1     
  Lines         776      729      -47     
  Branches      125      121       -4     
==========================================
- Hits          569      549      -20     
+ Misses        183      155      -28     
- Partials       24       25       +1     
Impacted Files Coverage Δ
cylc/uiserver/hubapp.py 72.72% <72.72%> (ø)
cylc/uiserver/workflows_mgr.py 90.90% <73.33%> (+0.69%) ⬆️
cylc/uiserver/resolvers.py 32.97% <75.00%> (-0.71%) ⬇️
cylc/uiserver/data_store_mgr.py 67.44% <80.00%> (+1.54%) ⬆️
cylc/uiserver/handlers.py 88.33% <80.00%> (+7.03%) ⬆️
cylc/uiserver/app.py 86.95% <86.95%> (ø)
cylc/uiserver/__init__.py 100.00% <100.00%> (ø)
cylc/uiserver/schema.py 76.66% <100.00%> (ø)
cylc/uiserver/scripts/gui.py 100.00% <100.00%> (ø)
cylc/uiserver/scripts/hub.py 100.00% <100.00%> (ø)
... and 8 more

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 47096f4...65be593. Read the comment docs.

* setup.py -> setup.cfg
* pytest.ini -> pyproject.toml
* pypi classifiers
* revert to decorator approach
* standardise flake8 setup with cylc flow
* fix violations
@oliver-sanders oliver-sanders self-assigned this Jul 30, 2021
@oliver-sanders oliver-sanders added this to the cylc-uiserver 0.6.0 milestone Jul 30, 2021
This was referenced Jul 30, 2021
CHANGES.md Outdated Show resolved Hide resolved
Copy link
Contributor

@datamel datamel left a comment

Choose a reason for hiding this comment

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

Still having problems getting the config to load.

'hub/config.py'

I think should be changed to jupyter_config.py.

I also unfortunately am getting the same _xsrf error on graphiql with all your latest branches checked out.

cylc/uiserver/app.py Outdated Show resolved Hide resolved
@datamel
Copy link
Contributor

datamel commented Aug 23, 2021

Getting 404s on the auto pop-up loading cylc gui.

@oliver-sanders
Copy link
Member Author

Thanks both:

  • Mel spotted a mistake which explains why the hub is picking up the wrong config file, sorry.
  • Yes, I renamed UIServer to CylcUIServer as that makes it a bit less ambiguous as a server extension, this means the traitlet namespace has changed too.
  • I'm going to try and replicate the xsrf issues seen when running in hub mode.
  • I'm going to try and bump us up to the latest Jupyter Server release, you may need to manually (re)install jupyter_server at the specified version as pip install does not always behave as you would expect it to in this regard.

@datamel
Copy link
Contributor

datamel commented Aug 24, 2021

Getting 404s on the auto pop-up loading cylc gui.

This issue has self-fixed for me with a re-checkout of all branches, cleaning cookies and re-install. 🤷‍♀️ Not sure what caused it but can't replicate fortunately!

@oliver-sanders
Copy link
Member Author

oliver-sanders commented Aug 24, 2021

Bumped the Jupyter Server version.

I also unfortunately am getting the same _xsrf error on graphiql with all your latest branches checked out.

But a request to graphql to request mutations info is failing I think?

I just cannot reproduce this.

Getting 404s on the auto pop-up loading cylc gui.

This issue has self-fixed for me with a re-checkout of all branches

This shouldn't be possible, though it could be caused by having missmatched branches, in the server log you should see two messages for the Cylc extension, one saying it's linked, one saying it's loaded. If either step fail there should be an error message.

@datamel
Copy link
Contributor

datamel commented Aug 24, 2021

  • I'm going to try and replicate the xsrf issues seen when running in hub mode.

In case it helps with the replicating, I have found a reliableway to reproduce...

  • Run cylc hub, load the ui as normal and run a query, eg
  workflows {
    id
  }
}

This runs as normal, with no _xsrf complaints..

  • then go to cylc hub and log out of the ui. Pop up the developer console and look at the network. Log back in.
  • You should see 403 for subscriptions and graphql, with output in the ui-server output of
    403 POST /user/.../cylc/graphql (::1): '_xsrf' argument missing from POST and Token stored in cookie may have expired
    As I mentioned in team meet, I think this is not a major problem, I think this problem actually self-corrects.

* Closes cylc#231
* Provide our own static handler rather than using Jupyter Server because:
  * Our static files are contained in sub dirs, Jupyter Server does not
    support this for static files.
  * We are not using the same template/static resource approach.
  * We would have to edit URLs in the UI to point at `static/cylc/*`.
  * We are opening these static files to multiple users so need to push
    them through our authorisation code.
* Adding authentication here provides login redirection and does all the
  cookie / token stuff.
* Add an endpoint that provides the equivalent to `cylc version --long`
* This replaces the `hello` handler used during development.
* As it doesn't engage any extra logic it may be useful for debugging
  UI/UIS issues.
* It may also be useful for debugging environment issues.
* Document handlers.
@oliver-sanders
Copy link
Member Author

oliver-sanders commented Aug 24, 2021

Ok, the last few commits have:

  • Responded to outstanding feedback.
  • Bumped the Juptyter Server version onto the latest and greatest.
  • Fixed jupyter_server: use server to serve static pages #231 (the first caveat) which should resolve the intermittent 403 issues reported earlier.
  • Replaced /cylc/hello with /cylc/version (nice to have a minimal endpoint for debugging).
  • Broken the tests dammit.
  • Updated the description and crossed out the caveats.

Needs some heavy multi-user testing before sign off.

Copy link
Contributor

@datamel datamel left a comment

Choose a reason for hiding this comment

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

The problems that I was getting have settled with the last few commits.
The issue with the 403 after logging out and back in (with the _xsrf error) still occurs but self corrects. I think this is acceptable behaviour as it does not affect functionality.
The button for Cylc Hub on the ui should be disabled for jupyter-cylc, on click it currently gives a 404 (I've opened up a follow up pr in the ui for this - cylc/cylc-ui#737).
There is a remaining typo where implanted should be implemented, I think but this should not affect merge, I can fix on the auth pr.
Bravo @oliver-sanders 🥇 - lovely functionality to have!

Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

Looks good to me! Couple comments but I think they are not blockers. Feel free to push the merge button @oliver-sanders (just so you can read the comments before 👍 )

CHANGES.md Outdated Show resolved Hide resolved
cylc/uiserver/app.py Show resolved Hide resolved
@kinow kinow merged commit 4161229 into cylc:master Aug 31, 2021
@kinow
Copy link
Member

kinow commented Aug 31, 2021

Two approvals, CI happy, @oliver-sanders resolved all feedback. LGTM 😬

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UIS: use jupyter server
4 participants