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

Mount notebooks into a mounted fs volume to avoid data loss #873

Closed
wants to merge 1 commit into from
Closed

Mount notebooks into a mounted fs volume to avoid data loss #873

wants to merge 1 commit into from

Conversation

timothyjlaurent
Copy link
Contributor

The current way this is done:

docker run -p 8888:8888 -it --rm $USER/assignments

creates an ephemeral container due to the --rm flag. In the first notebook you are doing very expensive operations (gunziping) several GBs. All of this data will be lost when the container shuts down. Using the filesystem as a mounted volume, allows subsequent sessions to have access to the files created (eg pickled data). This is especially important because this is course material so want to avoid time consuming gotchas for the students.

@tensorflow-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@timothyjlaurent timothyjlaurent changed the title Mount notebooks into a volume to avoid data loss with --rm flag Mount notebooks into a mounted fs volume to avoid data loss Jan 25, 2016
@timothyjlaurent
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@vincentvanhoucke
Copy link
Contributor

@craigcitro any gotcha going down that route?
Should I also be advertising not using --rm for users of the pre-built Docker container?

@craigcitro
Copy link
Contributor

No, these both seem pretty reasonable. Two thoughts:

  1. The --rm thing is definitely a smart move -- I tend to spend more time debugging docker images than using them right now. For this case, though, it does make it a bit too easy to lose notebooks.
  2. Mounting the notebooks dir as a volume also makes sense. This is one where mounting it ("I want to make changes that get saved") and not mounting it ("I'm playing around with something and it's a throwaway") both make sense, so maybe some text explaining when you would or wouldn't want the option? As a minor thing, no need to cd -- just pass the subdir path instead of $(pwd)?

@vincentvanhoucke
Copy link
Contributor

@timothyjlaurent thanks for the fix. Would you mind changing the doc for the hosted container as well and removing the 'cd'?

@timothyjlaurent
Copy link
Contributor Author

Ok @vincentvanhoucke I updated the readme.


To avoid losing work between sessions in the container, it is recommended that you mount the `tensorflow/examples/udacity` directory into the container:

docker run -p 8888:8888 -v </path/to/tesorflow/examples/udacity>:/notebooks -it --rm $USER/assignments
Copy link
Contributor

Choose a reason for hiding this comment

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

s/tesorflow/tensorflow/

@timothyjlaurent
Copy link
Contributor Author

I fixed the typo

@vincentvanhoucke
Copy link
Contributor

LGTM @vrv

@vrv
Copy link

vrv commented Jan 26, 2016

Thanks! Squash the commits and we'll merge.

The current way this is done:

```
docker run -p 8888:8888 -it --rm $USER/assignments
```

creates an ephemeral container due to the `--rm` flag. In the first notebook you are doing very expensive operations (gunziping) several GBs. All of this data will be lost when the container shuts down. Using the filesystem as a mounted volume, allows subsequent sessions to have access to the files created (eg pickled data). This is especially important because this is course material so want to avoid time consuming gotchas.

update readme Re ephemeral vs mounted notebooks dir
@timothyjlaurent
Copy link
Contributor Author

Commits have been squashed.

There is one caveat to this PR -- if the user needs to rebuild the docker image -- all the files underneath will need to be added to the context for the build. This is several GB for the first assignment so it takes a LONG time.

@craigcitro
Copy link
Contributor

We aren't running any docker build commands from the top of the tree, though, are we? (Just the one docker run?)

@vincentvanhoucke
Copy link
Contributor

Merged. Thanks!

@timothyjlaurent
Copy link
Contributor Author

Well in the readme it says:

 Building a local Docker container
 ---------------------------------

     cd tensorflow/examples/udacity
     docker build -t $USER/assignments .

If you use that same directory to mount the notebooks then it will fill up with data files. Then if you need to remake the image all of that will have to be put in the context. When I had to do this, I just moved the data directories temporarily to .. and then back in.

So maybe we should recommend copying the udacity dir to some work dir location and then mount that into the container or mention the issue with context and explain that they should move extraneous files elsewhere while rebuilding the image.

@vincentvanhoucke
Copy link
Contributor

I'm not sure why you, as a class user, would ever want to build the container?
These instructions are for us, when we want to update the hosted containers.

@timothyjlaurent
Copy link
Contributor Author

The course materials instructed me to build the image:

image

If there is an image I could pull, that would definitely be better, but there is no mention of this image in the course materials as far as I can see.

@vincentvanhoucke vincentvanhoucke removed their assignment Jan 26, 2016
@vincentvanhoucke
Copy link
Contributor

Arpan, there should be no need for anyone to build a container. Was the doc updated?

@tensorflow-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

1 similar comment
@tensorflow-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@timothyjlaurent
Copy link
Contributor Author

If there is a hosted image that should be used for the class, the command should look something like this:

docker run --rm -v <path to notebooks>:/notebooks tensorflow/udacity

@vincentvanhoucke
Copy link
Contributor

@timothyjlaurent Access to the hosted version is described on the first line in the document you edited:
docker run -p 8888:8888 -it --rm b.gcr.io/tensorflow-udacity/assignments
Maybe I should separate the rest of this readme so that it stands out more.

@timothyjlaurent
Copy link
Contributor Author

Oh I see it now that you point it out. Maybe the course should be updated to instruct users to use that image rather than build their own.

@timothyjlaurent
Copy link
Contributor Author

Also now that I see the first line (although it wasn't shown in the course material)
This should probably also mount the working directory as a volume.
It takes maybe 20-30 minutes to decompress the data files. It also loads all this data into memory -- I had to upgrade my docker machine VM to have 8GB of RAM to avoid running out of memory. All of this should be disclosed to the student.

@vincentvanhoucke
Copy link
Contributor

@timothyjlaurent we pushed some changes this weekend that should help with memory.

tarasglek pushed a commit to tarasglek/tensorflow that referenced this pull request Jun 20, 2017
Updated calls to '..._cross_entropy_with_logits' to add arguments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants