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

feat(core): notebook_session provider #2880

Merged
merged 27 commits into from Jun 7, 2022
Merged

Conversation

olevski
Copy link
Member

@olevski olevski commented May 3, 2022

Description

Launching notebooks sessions with the cli.

Fixes #2635

Type of change

Added a new session provider. It will work with both anonymous and registered users. As long as the renku deployment supports anonymous sessions.

@olevski olevski requested a review from a team as a code owner May 3, 2022 07:26
renku/ui/cli/session.py Show resolved Hide resolved
renku/core/session/notebook_service.py Outdated Show resolved Hide resolved
renku/core/session/notebook_service.py Outdated Show resolved Hide resolved
renku/core/session/notebook_service.py Outdated Show resolved Hide resolved
renku/core/session/notebook_service.py Outdated Show resolved Hide resolved
renku/core/session/notebook_service.py Outdated Show resolved Hide resolved
renku/core/session/notebook_service.py Outdated Show resolved Hide resolved
renku/core/session/notebook_service.py Outdated Show resolved Hide resolved
renku/core/session/notebook_service.py Outdated Show resolved Hide resolved
renku/core/session/notebook_service.py Outdated Show resolved Hide resolved
@olevski olevski force-pushed the notebook-session-provider branch from cb0e853 to b95cb50 Compare May 20, 2022 09:21
@olevski olevski force-pushed the notebook-session-provider branch from e60e9f5 to 938b037 Compare May 20, 2022 09:46
@olevski olevski force-pushed the notebook-session-provider branch from b89700c to 71231b2 Compare May 20, 2022 12:20
@olevski olevski force-pushed the notebook-session-provider branch from 71231b2 to 9282fe2 Compare May 20, 2022 12:24
@olevski olevski force-pushed the notebook-session-provider branch from a6d9046 to 8bd779b Compare May 20, 2022 12:40
@olevski olevski requested a review from Panaetius May 20, 2022 12:47
@olevski
Copy link
Member Author

olevski commented May 23, 2022

p.s. I think that the documentation-pending label can be removed since the use of the renku session cli command has been documented in this PR. But then I am not sure what is the exact meaning of the label and maybe it should stay on this PR. Let me know if there is something I can do here for the docs or anything else.

@Panaetius
Copy link
Member

p.s. I think that the documentation-pending label can be removed since the use of the renku session cli command has been documented in this PR

That's the exact purpose of that label. Remind people to check if they added documentation. And once they did, they can remove the label.

@rokroskar
Copy link
Member

There seems to be an issue with creating the image name:

renku session start -p notebook_service                                                                                     
The container image 'registry.renkulab.io/k.roskar/jupytext:a83f00b' does not exists. Would you like to build it? 

It should be looking for registry.renkulab.io/rok.roskar/jupytext:a83f00b

@rokroskar
Copy link
Member

I did

renku clone https://renkulab.io/gitlab/rok.roskar/jupytext.git
cd jupytext
renku login --git renkulab.io
renku session start -p notebook_service

Error: Cannot start session via the notebook service because {"messages":{"error":"Cannot start server rok-2erosk-jupytext-ac25ab2f, because project jupytext does not exist, branch master does not exist, commit a83f00bc4f3d151358322b2799460f830b63e083 does not exist, image registry.renkulab.io/rok.roskar/jupytext:a83f00b does not exist or cannot be accessed"}}

The image exists (I can pull it) as do the branch and the commit. It seems to be a problem with the fact that I did renku login --git - when I skip --git it works.

@rokroskar
Copy link
Member

rokroskar commented Jun 1, 2022

two more comments:

  1. instead of having renku session open go to <renku-url>/sessions/<session> could we instead go to <renku-url>/projects/<namespace>/<project>/sessions/show/<session>? The latter takes you to the framed notebook with a bit more of the renkulab context around it. The user can click on "open" to get the session in the full tab afterwards, but there is no way to go in the other direction as of now.
  2. Can we call the provider something other than notebook_service? I think renkulab would be something the users would understand better, no one really knows what the notebook service is imho except for the developers.

@olevski
Copy link
Member Author

olevski commented Jun 2, 2022

It seems to be a problem with the fact that I did renku login --git - when I skip --git it works.

What is the purpose of the --git flag? From what I could tell it just uses a slightly different remote name that gets routed some special way through the gateway or something? The explanation from the --help says Log in to gitlab too.. But a bit more context in this case would be definitely useful.

Co-authored-by: Rok Roškar <roskarr@ethz.ch>
@rokroskar
Copy link
Member

rokroskar commented Jun 2, 2022

What is the purpose of the --git flag?

it replaces the remote with one that goes through the gateway so a JWT can be used

@olevski
Copy link
Member Author

olevski commented Jun 2, 2022

@Panaetius I added a section in the file you mentioned so that things appear in the documentation.

@rokroskar I addressed your comments:

  • now the provider (both internally and externally) is called renkulab
  • the issues with wrong/incomplete project names have been resolved (it was all due to lstrip)
  • the session is opened in the iframe view when the user is registered, I could not use the iframe view for anonymous sessions because I need to pass the anonymous user ID in the URL but the UI does not seem to accept this in the iframe view (in the iframe view the UI just relies on the anonymous ID being present in a cookie I think).

Let me know if you need anything else.

@rokroskar
Copy link
Member

Thanks @olevski! It's almost there - I still get this (after logging in with renku login --git:

$ renku session start -p renkulab
Error: Cannot start session via the notebook service because {"messages":{"error":"Cannot start server rok-2erosk-jupytext-ac25ab2f, because project jupytext does not exist, branch master does not exist, commit a83f00bc4f3d151358322b2799460f830b63e083 does not exist, image registry.renkulab.io/rok.roskar/jupytext:a83f00b does not exist or cannot be accessed"}}

Works like a charm if using the normal remote.

@olevski
Copy link
Member Author

olevski commented Jun 2, 2022

Does anyone know how to make this docs linting error go away? I do not even have bullet lists in line 38 at renku.ui.cli.session.

Warning, treated as error:
/opt/hostedtoolcache/Python/3.7.13/x64/lib/python3.7/site-packages/renku/ui/cli/session.py:docstring of renku.ui.cli.session:38:Bullet list ends without a blank line; unexpected unindent.
Error: Process completed with exit code 2.

EDIT: I figured this out and fixed it. It was the indentation in bullet lists that span more than one line in the file.

@olevski
Copy link
Member Author

olevski commented Jun 2, 2022

@rokroskar I cannot replicate this problem on my end, this is what I did:

  • pipx install <renku-python-repo-dir> --force
  • forked your jupytext renku project
  • renku clone https://renkulab.io/gitlab/tasko.olevski/jupytext.git
  • renku login --git renkulab.io
  • renku session start -p renkulab

And it worked.

Can you please add a print statement after line 327 in renku/core/session/renkulab.py i.e. just like print(payload) and tell me what you get there?

@olevski
Copy link
Member Author

olevski commented Jun 2, 2022

In addition to testing with a fork of your jupytext repo I also tested with the jupytext repo directly @rokroskar. That also works.

EDIT: nvm I get the error you reported.

@olevski
Copy link
Member Author

olevski commented Jun 2, 2022

@rokroskar I fixed the error you mentioned. I used lstrip in two places and I thought I fixed both of them but I hadn't. Now it should all work.

@olevski
Copy link
Member Author

olevski commented Jun 2, 2022

Ok all the comments and linting errors have been addressed.

Copy link
Member

@rokroskar rokroskar left a comment

Choose a reason for hiding this comment

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

Looks great, thanks @olevski!

@olevski olevski merged commit f554f19 into develop Jun 7, 2022
@olevski olevski deleted the notebook-session-provider branch June 7, 2022 07:25
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.

Create a renkulab interactive session provider
3 participants