Skip to content

Conversation

jakirkham
Copy link
Member

@jakirkham jakirkham changed the title Docker Readme Fixes Docker README Fixes Oct 8, 2015
@jakirkham
Copy link
Member Author

@funkyfuture, bringing to your attention.

jakirkham referenced this pull request in jakirkham/notebook Oct 8, 2015
…ted in a manner similar to the way a notebook would be normally.
README.md Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be rephrased to make clear that the data volume will be created internally and persist(!) if not overridden like in the example.

Copy link
Member Author

Choose a reason for hiding this comment

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

How do you mean persist here? Using --rm means the container will be cleaned up on exit.

Copy link
Contributor

Choose a reason for hiding this comment

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

your example may be well for demonstration purposes. but it's hardly useful for daily usage.

my point is: this is a documentation of the Docker image. that image includes a volume declaration. for non-demonstration-usages it is essential to mention this volume explicitly.

Copy link
Contributor

Choose a reason for hiding this comment

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

All notebooks from your session will be saved in the current directory. In reality, they are save inside the container at /notebook.

in the case of the example this is not true and a contradiction.

Copy link
Member Author

Choose a reason for hiding this comment

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

your example may be well for demonstration purposes. but it's hardly useful for daily usage.

I respectfully disagree. There are quite a few dependencies required by jupyter. Getting a nice working copy can sometimes be tricky. Being able to startup a docker container instead of debugging the build/install process is a huge asset. Not having to understand all of the ends and outs of docker in order to get jupyter up and running makes this worthwhile for the average end user.

If your point here is that docker is normally used on cluster deployments and the like, then I think that user will know enough about docker that they will have no problem changing these options to fit their needs.

my point is: this is a documentation of the Docker image. that image includes a volume declaration. for non-demonstration-usages it is essential to mention this volume explicitly.

Which it now is.

All notebooks from your session will be saved in the current directory. In reality, they are save inside the container at /notebook.
in the case of the example this is not true and a contradiction.

Not true. They are saved in that directory. While the container is up and running one could still find these using docker exec. The container is merely blown away on exit instead of merely being stopped.


I have tried to add a line to address the concern you have raised. If you think I have not done so, please be explicit about what you would like to see instead. I would like this to be resolved in a manner that we can both find satisfactory soon.

@jakirkham jakirkham force-pushed the docker_readme_fixes branch 2 times, most recently from 461f240 to 0b0ab15 Compare October 8, 2015 16:47
…ow it relates to the current working directory. [ci skip]
@jakirkham jakirkham force-pushed the docker_readme_fixes branch from 0b0ab15 to 3c0c52c Compare October 9, 2015 11:32
@funkyfuture
Copy link
Contributor

They are saved in that directory.

yes, that's the path inside the container. but with the example with a host-path mounted, in 'reality' the nbs are saved in that host-path. or, if no mounted volumes is given, they will be saved in an extra data volume, that is not in the container, but used by it.

i will provide a proposal for a wording after a nap.

@willingc
Copy link
Member

willingc commented Oct 9, 2015

@jakirkham @funkyfuture Thank you both for sharing your thoughts. Let's please remember that all documentation is about the end user. Those end users are going to vary in skill and use cases.

Clarity and concrete points are most helpful in the README. I recommend that we work toward the 80/20 rule. Try to cover 80% of the typical users in the README and link the other 20% to more detailed information in the project documentation. Thank you. 🐧

@jakirkham
Copy link
Member Author

Sorry this got really noisy. I had only meant to be helpful since I was already submitting a small patch and @funkyfuture made a good point. However, I think this was the wrong move in retrospect as I am clearly missing the mark as to what he is after and that isn't helping anyone. I would like to ask @funkyfuture to supply his own patch.

@willingc
Copy link
Member

willingc commented Oct 9, 2015

@jakirkham Oh no worries. I think that both you and @funkyfuture have helpful views here. I've opened an issue #571 since I believe that folks could benefit from the detail. 🍰

@funkyfuture
Copy link
Contributor

hm, i'd just undo cb080df.

but again, that is targeted only to users who read here and links the relevant docs about volumes in Docker. in developing it's absolutely reasonable to use --rm.

@minrk minrk modified the milestone: no action Dec 1, 2015
@coveralls
Copy link

coveralls commented Jul 26, 2017

Coverage Status

Changes Unknown when pulling 3c0c52c on jakirkham:docker_readme_fixes into ** on jupyter:master**.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants