feat(core): design of renku session #2529

vigsterkr · 2021-12-15T07:48:53Z

Description

Fixes #1991 #2394

vigsterkr · 2021-12-15T10:22:41Z

@SwissDataScienceCenter/poc this is an initial implementation of supporting a local interactive session. any feedback is more than welcome.

rokroskar

Nice! I really like this, it's very simple and clean. There are a couple of outstanding questions, with the error handling and UX around suggesting alternative paths in case of failure being a big one here, I think, since we have to assume we are dealing with users with very little docker experience.

design/003-interactive-session/003-interactive-session.md

rokroskar · 2022-01-20T08:36:16Z

design/003-interactive-session/003-interactive-session.md

+
+A command to start an interactive session.
+
+By default, the `Dockerfile` present in the given project is going to be used for building the docker image


I would say by default we look for the image on the hosted instance (if we can identify one). That would be quicker than building in most cases. We can identify the registry from the origin remote.

There is a lot of potential for failure here so we need to think about the error handling with user-friendly suggestions for what to do next. E.g.

check local docker images --> none exists, check remote

remote image lookup fails because image isn't there and no local image exists --> prompt to build one

remote image lookup fails (private project and no token) --> suggest to login

remote image lookup fails, (private project with token) --> suggest image build

image build fails, no docker --> link to docker install / documentation

image build fails, docker working but problem in Dockerfile --> suggest a base image? or 🤷

A related question is how do we actually identify which image we need? Simply by the commit sha? When the runtime doesn't change for many commits, we could just check which commit we need (i.e. when did the runtime dependencies change last - though we might not know all the paths in the repo that affect the runtime). Probably the safest is to go with the commit sha even if it results in too many image builds.

by using commit hash when the runtime env doesn't change the caching mechanism (at least in case of docker) will circumvent the unnecessary rebuilds. by default the code-base itself (of the project) is mounted into the running container, so it shouldn't affect the image building at all.

There is a lot of potential for failure here so we need to think about the error handling with user-friendly suggestions for what to do next. E.g.
check local docker images --> none exists, check remote
remote image lookup fails because image isn't there and no local image exists --> prompt to build one
remote image lookup fails (private project and no token) --> suggest to login
remote image lookup fails, (private project with token) --> suggest image build
image build fails, no docker --> link to docker install / documentation
image build fails, docker working but problem in Dockerfile --> suggest a base image? or 🤷

we should describe this logic or do you think we should always build and never fetch from the remote registry?

regarding the logic of image fetching/building: since now it is a plugin based solution, this is strictly up to the plugin what and how to do things. the most this RFC can do is to kind of write a suggestion how it should be handled, but honestly the plugin will do whatever it wants to do... note i'm not so sure what happens in case of notebook api when an image is defined...

design/003-interactive-session/003-interactive-session.md

rokroskar · 2022-01-20T08:39:38Z

design/003-interactive-session/003-interactive-session.md

+## Drawbacks
+
+One of the major drawback of the current design is the dependency on docker. Namely, to only support local
+interactive session if docker is available. Many HPC environments do not support docker, but they support


I think considering other runtimes is definitely a valid concern but with regards to HPC specifically, there are a lot of other hurdles (port-forwarding to compute nodes, for example). We might want to consider that a separate case altogether.

design/003-interactive-session/003-interactive-session.md

rokroskar · 2022-01-20T08:51:47Z

design/003-interactive-session/003-interactive-session.md

+other container formats, like [runC](https://github.com/opencontainers/runc). As a long term goal of this
+effort it would be good to consider support for different container engines, hence the design and implementation
+of the initial `renku session` should take this into consideration.
+


One general question - the idea here is to mount the local repo directory into the container. Our containers typically run as user 1000 with gid 1000 - what happens with the file ownership outside the container?

yep i was looking into this, specifically from windows perspective 🙃

rokroskar

This is looking great! Just a few clarifying comments. Sorry it took me a while 🙈

design/003-interactive-session/003-interactive-session.md

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

in order to support custom image for a session

vigsterkr · 2022-02-06T08:04:07Z

actually today i was thinking a bit more about this whole local vs remote story and realised that this is not the best approach, i.e. it shouldn't be distinguished like this. rather renku session sub-command should just have a support for various container engines---as plugins---and just as in case of workflow execute provider one would choose which one to use when running the command, for example:

renku session start --engine singularity

by default renku would provide two different plugins: docker and renkulab. all the others would be simply plugins as usual (e.g. singularity or youki).
Note i think the other problem of the current --remote design is that a docker can be remote as well! it all depends on the env variables settings.

maybe the --engine flag is not the best name. maybe we should be consistent and use --provider just as in case of workflows... none the less this would in a way unify the whole renku session across various container backends and 'locations'

rokroskar · 2022-02-07T13:44:56Z

I think --provider makes a lot of sense! I could imagine something like this for e.g. HPC clusters which would have very specific needs but it would be self-consistent if we name them all "providers". So I guess the RFC doc should then define the interface?

vigsterkr · 2022-02-07T22:46:35Z

I think --provider makes a lot of sense! I could imagine something like this for e.g. HPC clusters which would have very specific needs but it would be self-consistent if we name them all "providers". So I guess the RFC doc should then define the interface?

refactored, updated the RFC. now docker based interactive sessions are implemented as a plugin.

i'll add the ISessionProvider interface definition to the RFC ASAP

vigsterkr · 2022-02-08T06:16:50Z

mmm seems a good side-effect of this refactor that the docker plugin won't even show up if docker engine is not installed 🙃 just need to do some extra exception handling (see macos ci failure)

configuration

this way the plugin will always be present and the error will only be raised when actually trying to use a session command and the docker instance cannot be found.

design/003-interactive-session/003-interactive-session.md

renku/core/management/session/docker.py

olevski

@vigsterkr I tried to install this and run it on my mac. With a project I created with renku init. I am getting this message:

AttributeError: 'ImageNotFound' object has no attribute 'msg'

It also happens if I specify a wrong not-real image with --image

If I specify a real image then things work.

I know this is a WIP but just thought I would point this out.

Also see my comment about getting the default url from the project. I think that would be really useful.

renku/core/management/session/docker.py

olevski · 2022-02-08T11:13:40Z

Sorry for spamming. I noticed I was a few commits back on this branch when I was testing. A small clarification on the errors I am seeing.

renku session start results in:

AttributeError: 'APIError' object has no attribute 'msg'

renku session start --image fdsfdsf results in:

AttributeError: 'ImageNotFound' object has no attribute 'msg'

vigsterkr · 2022-02-08T11:20:48Z

Sorry for spamming. I noticed I was a few commits back on this branch when I was testing. A small clarification on the errors I am seeing.
* `renku session start` results in:
AttributeError: 'APIError' object has no attribute 'msg'
* `renku session start --image fdsfdsf` results in:
AttributeError: 'ImageNotFound' object has no attribute 'msg'

@olevski nw, added a fix for those comments

rokroskar

Some additional comments on the RFC.

Also, could we separate the implementation from the RFC? The PR discussion is muddled by the fact that both things are getting addressed at once imho and it would be easier for posterity if they were separate.

rokroskar · 2022-02-08T11:21:23Z

design/003-interactive-session/003-interactive-session.md

+an interactive session. By default two different providers are shipped with Renku CLI:
+ - docker
+ - renkulab
+The provider specific configuration values can be specified by using the `--config <config.yaml>` flag.


I should be able to persist them also in the project settings?

this is in line with the workflow execute provider logic.... of course these values could be stored anywhere... i dont have any strong opinion about this.

although i'm not so sure why would one want to store youki, singularity etc config values on a project level...these these key-value pairs meant for actually specifying some config value for the provider plugin, that i see more like a global setting not a project level setting. none the less, they could be persisted anywhere...

rokroskar · 2022-02-08T11:23:38Z

design/003-interactive-session/003-interactive-session.md

+
+A command to start an interactive session.
+
+By default, the `Dockerfile` present in the given project is going to be used for building the docker image


There is a lot of potential for failure here so we need to think about the error handling with user-friendly suggestions for what to do next. E.g.
check local docker images --> none exists, check remote
remote image lookup fails because image isn't there and no local image exists --> prompt to build one
remote image lookup fails (private project and no token) --> suggest to login
remote image lookup fails, (private project with token) --> suggest image build
image build fails, no docker --> link to docker install / documentation
image build fails, docker working but problem in Dockerfile --> suggest a base image? or 🤷

we should describe this logic or do you think we should always build and never fetch from the remote registry?

design/003-interactive-session/003-interactive-session.md

renku/core/management/session/docker.py

vigsterkr · 2022-02-15T08:20:52Z

closing in favour of #2687 #2688

github-actions bot added the documentation:pending label Dec 15, 2021

vigsterkr changed the title ~~design of renku session~~ feat(core): design of renku session Dec 15, 2021

vigsterkr force-pushed the session-rfc branch from 2fe3285 to 5800ebf Compare December 15, 2021 10:06

vigsterkr removed the documentation:pending label Dec 16, 2021

vigsterkr force-pushed the session-rfc branch from 5800ebf to 4cd9366 Compare December 16, 2021 08:27

vigsterkr changed the base branch from master to develop December 22, 2021 08:03

vigsterkr force-pushed the session-rfc branch from 4cd9366 to 2791b22 Compare January 12, 2022 14:10

rokroskar reviewed Jan 20, 2022

View reviewed changes

vigsterkr force-pushed the session-rfc branch from 22c2a4d to 54011e4 Compare January 24, 2022 13:06

vigsterkr force-pushed the session-rfc branch 2 times, most recently from be2e037 to 1ba476e Compare February 2, 2022 13:55

vigsterkr marked this pull request as ready for review February 2, 2022 13:56

vigsterkr requested a review from a team as a code owner February 2, 2022 13:56

Panaetius requested a review from rokroskar February 2, 2022 15:05

rokroskar requested changes Feb 3, 2022

View reviewed changes

design/003-interactive-session/003-interactive-session.md Outdated Show resolved Hide resolved

design/003-interactive-session/003-interactive-session.md Outdated Show resolved Hide resolved

design/003-interactive-session/003-interactive-session.md Outdated Show resolved Hide resolved

vigsterkr force-pushed the session-rfc branch from 1acead4 to 2b06990 Compare February 3, 2022 21:44

Viktor Gal and others added 9 commits February 5, 2022 09:06

design of renku session

8b80520

Update design/003-interactive-session/003-interactive-session.md

a01344e

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

add optional flag to specify a docker image for session start

b9f623c

handle when docker is not available

c90c1b7

add --remote documentation

4173ffb

add resource flags for renku session start

46f4b8b

add renku sesion open subcommand

35afb44

renku session list only lists active sessions for the given project

546e3b5

use labels for containers

fbb8652

in order to support custom image for a session

vigsterkr force-pushed the session-rfc branch from 62db192 to fbb8652 Compare February 5, 2022 08:28

fix import order errors and update poetry

48b26a5

refactor using plugins

8fa920f

Viktor Gal added 3 commits February 8, 2022 10:43

add proper function documentation and yaml parsing for provider

2d285e8

configuration

guard docker client creation

76ee82d

this way the plugin will always be present and the error will only be raised when actually trying to use a session command and the docker instance cannot be found.

set auth_token for the jupyterlab session

e90625e

Panaetius reviewed Feb 8, 2022

View reviewed changes

design/003-interactive-session/003-interactive-session.md Outdated Show resolved Hide resolved

renku/core/management/session/docker.py Show resolved Hide resolved

Viktor Gal added 2 commits February 8, 2022 11:51

address PR comments

3712e56

add wrapper class for an interactive session

df849dd

olevski reviewed Feb 8, 2022

View reviewed changes

renku/core/management/session/docker.py Outdated Show resolved Hide resolved

address PR comments

b888657

rokroskar requested changes Feb 8, 2022

View reviewed changes

olevski reviewed Feb 8, 2022

View reviewed changes

renku/core/management/session/docker.py Show resolved Hide resolved

add support for pinned image

c051506

vigsterkr closed this Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): design of renku session #2529

feat(core): design of renku session #2529

vigsterkr commented Dec 15, 2021 •

edited

vigsterkr commented Dec 15, 2021

rokroskar left a comment

rokroskar Jan 20, 2022

vigsterkr Jan 24, 2022

rokroskar Feb 8, 2022

vigsterkr Feb 8, 2022

rokroskar Jan 20, 2022

rokroskar Jan 20, 2022

vigsterkr Feb 3, 2022

rokroskar left a comment

vigsterkr commented Feb 6, 2022

rokroskar commented Feb 7, 2022

vigsterkr commented Feb 7, 2022

vigsterkr commented Feb 8, 2022

olevski left a comment

olevski commented Feb 8, 2022

vigsterkr commented Feb 8, 2022

rokroskar left a comment

rokroskar Feb 8, 2022

vigsterkr Feb 8, 2022

vigsterkr Feb 8, 2022

rokroskar Feb 8, 2022

vigsterkr commented Feb 15, 2022


		A command to start an interactive session.

		By default, the `Dockerfile` present in the given project is going to be used for building the docker image

feat(core): design of renku session #2529

feat(core): design of renku session #2529

Conversation

vigsterkr commented Dec 15, 2021 • edited

Description

vigsterkr commented Dec 15, 2021

rokroskar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rokroskar left a comment

Choose a reason for hiding this comment

vigsterkr commented Feb 6, 2022

rokroskar commented Feb 7, 2022

vigsterkr commented Feb 7, 2022

vigsterkr commented Feb 8, 2022

olevski left a comment

Choose a reason for hiding this comment

olevski commented Feb 8, 2022

vigsterkr commented Feb 8, 2022

rokroskar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vigsterkr commented Feb 15, 2022

vigsterkr commented Dec 15, 2021 •

edited