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

build: Dockerfile and docker-compose to launch notebook with pygenn resolves genn-team/genn#546 #548

Merged
merged 2 commits into from
Oct 24, 2022

Conversation

Stevinson
Copy link
Contributor

Run pyGeNN models from a docker image. docker-compose is used to launch a jupyter notebook with files from the local filesystem.

Run GeNN models from a docker image that launches a jupyter notebook
@Stevinson Stevinson changed the title build: Dockerfile and docker-compose to launch notebook with pygenn build: Dockerfile and docker-compose to launch notebook with pygenn resolves genn-team/genn#546 Oct 20, 2022
@Stevinson Stevinson mentioned this pull request Oct 20, 2022
3 tasks
Copy link
Contributor

@neworderofjamie neworderofjamie left a comment

Choose a reason for hiding this comment

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

Thank you so much for your hard work on this - as a docker n00b, there's just a few things I'd like explained


COPY . ${HOME}

RUN adduser --disabled-password --gecos "" $USERNAME
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just use root as containers are kinda throwaway? My knowledge of docker is pretty crap so I could very well be missing something...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It’s something I was taught was best practice so have got into the habit of doing. I believe this is because:

  1. Certain execution environments block containers running as root
  2. Force container immutability
  3. Limits the processes that can be executed and who can execute them

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, that is really good to know, thanks

Copy link
Member

@jamesturner246 jamesturner246 Oct 21, 2022

Choose a reason for hiding this comment

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

Running the container as root (from the host) could be bad, sure, but I can't see any issue being root inside the container. Do you have an example of root inside containers being blocked by providers? The mutability argument, I'm not sure how relevant that is, since all state is lost once the container is killed anyway. As for limiting processes, sure, it stops the user running e.g. apt inside the container as interactive, but is that a good thing?

Copy link
Member

Choose a reason for hiding this comment

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

Btw, I don't mean to be pedantic. I'm no expert, and I'm very interested to hear yours and others' reasons for and against!

Copy link
Member

@jamesturner246 jamesturner246 Oct 21, 2022

Choose a reason for hiding this comment

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

Ahh.... I see the issue with containers starting as root now.....

I just pwnt my box with a container that begins as root! :)

https://flast101.github.io/docker-privesc/

@@ -0,0 +1,28 @@
version: "3.4"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't quite see why docker compose is required....I thought it was intended to combine together multiple containers whereas, couldn't what you do here be achieved with a single docker run incantation?

Also, more fundamentally, the way I imagine the GeNN docker image being used is that it would be hosted on Dockerhub (probably produced by CI when we make releases/merge master) so users wouldn't have to checkout our git repro and hence wouldn't have access to the docker compose script outside of the container.

Again, I may be missing things....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, it facilitates having multiple containers interacting together. I only used it as I find it helpful to set up port bindings, volumes, health checks, etc in a configurable way that is easy to use (and was used only to launch jupyter).

However, if it is not going to be useful for extendability then maybe, as you say, a command such as docker run -p 127.0.0.1:8888:8888/tcp -v '$(CURDIR)'/pygenn/notebooks:/root/pygenn/notebooks pygenn jupyter notebook --ip 0.0.0.0 --no-browser is better suited. I have added this to the makefile (as make docker-jupyter) and can delete the docker-compose.yml?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just for saving such brutally long command lines, docker compose seems like a good idea (especially as it's optional).

@neworderofjamie neworderofjamie changed the base branch from master to docker_stevinson October 21, 2022 10:54
@neworderofjamie neworderofjamie self-requested a review October 21, 2022 10:56
Copy link
Contributor

@neworderofjamie neworderofjamie left a comment

Choose a reason for hiding this comment

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

I am going to fiddle a bit more (hence the change of target) but, unless my colleague @jamesturner246 has any further concerns, I'm happy to accept this (and will add the relevant hacktoberfest label). Thank you again for your help with this!

Copy link
Member

@jamesturner246 jamesturner246 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 good. A couple of points that might be worth implementing - maybe @neworderofjamie also has preferences on them. Thanks!

@@ -69,3 +69,11 @@ clean:
@# Delete libGeNN
@rm -f $(LIBGENN)
@rm -f $(BACKEND_LIBS)

Copy link
Member

Choose a reason for hiding this comment

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

It might be cool and convenient to have a makefile rule to setup an interactive pygenn container with -it, and bind host pwd to some directory (e.g. 'workspace') in the working directory of the container. Just for people who don't want to use jupyter.

build_steps/Dockerfile Show resolved Hide resolved
@@ -13,3 +13,8 @@ PyGeNN wraps the C++ GeNN API using SWIG, allowing GeNN to be used either direct
- Navigate to the GeNN directory and build GeNN as a dll using ``msbuild genn.sln /t:Build /p:Configuration=Release_DLL`` (if you don't have CUDA installed, building the CUDA backend will fail but it should still build the CPU backend).
- Copy the newly built DLLs into pygenn using ``copy /Y lib\genn*Release_DLL.* pygenn\genn_wrapper``
- Build the Python extension with setup tools using ``python setup.py develop`` command

Copy link
Member

Choose a reason for hiding this comment

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

Should probably add pygenn/README.md documentation for the docker-jupyter option, plus for the interactive container rule mentioned above, if implemented.

@neworderofjamie neworderofjamie merged commit 130b600 into genn-team:docker_stevinson Oct 24, 2022
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.

4 participants