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

Add an edit-mode option #421

Merged
merged 6 commits into from Oct 8, 2018

Conversation

@evertrol
Copy link
Contributor

evertrol commented Oct 1, 2018

This adds an option to run from a local repository in edit mode, where
changes in a running Docker container (for example, through a
notebook) are reflected in the local repository.

Implements the feature suggested in #357.

@evertrol

This comment has been minimized.

Copy link
Contributor Author

evertrol commented Oct 1, 2018

This may still require some tests, which are probably quite a bit more work than implementing the actual feature (given that changes in both a local directory and a container need to be verified).

@betatim

This comment has been minimized.

Copy link
Member

betatim commented Oct 1, 2018

Test will indeed require a bit of thought. Some ideas below.

What do you think of using the tests from #413 (https://github.com/jupyter/repo2docker/pull/413/files#diff-73b9b926dafec45bd12128b598a0dd45) as a starting point?

I was thinking if we can execute repo2docker from the "outside" (with most of the other tests we only get to run code in verify inside the container) then we can use a start file that is executed inside the container to make a change and check that the local directory was indeed changed.

@betatim

This comment has been minimized.

Copy link
Member

betatim commented Oct 1, 2018

In terms of things we need:

(if you want to you can move this list to the top comment, not sure if GitHub let's you tick boxes in my comment)

@evertrol

This comment has been minimized.

Copy link
Contributor Author

evertrol commented Oct 3, 2018

I've put the documentation as a FAQ (even if it isn't (yet)), since I'm not sure how well it fits as a how-to. And I didn't see documentation on the individual options (other than what argparse produces on the command line, obviously).

Also: I can't tick the check-box :-(

@evertrol

This comment has been minimized.

Copy link
Contributor Author

evertrol commented Oct 3, 2018

I seem to have hit a little snag: I can't bind an external volume to an internal volume using an internal variable. I'd like to bind to $HOME or ~, but that just get escaped inside client.containers.run(..., volumes=container_volumes, ...). I can get hardcode /home/jovyan for tests, but that is obviously not a solution when using user-supplied Docker files.

I've been looking at another way to work around this, or find a configuration option to change this, but no luck so far. Would anyone know how to do this?

(to be clear, I'd like to do:

client.containers.run(..., volumes={'/host/directory', {'bind': '$HOME', 'mode': 'rw'}}, ...)

)

@betatim

This comment has been minimized.

Copy link
Member

betatim commented Oct 3, 2018

I think we could resolve $HOME before making that call?

I'd be Ok with having the docs say "Editable mode is a convenience option that will mount the repository to /home/$USER, if you need to mount to a different location in the container please use --volumes." if we can't find a solution to this.

@evertrol

This comment has been minimized.

Copy link
Contributor Author

evertrol commented Oct 3, 2018

I have yet to figure out how to resolve $HOME before the container is run (or, otherwise, mount a volume afterwards).

But it looks like using self.volumes[os.path.abspath(args.repo)] = '.' actually works, because the working directory is picked up from the image and prepended in case of a relative directory:

            image_workdir = image['ContainerConfig']['WorkingDir']

            for k, v in self.volumes.items():
                container_volumes[os.path.abspath(k)] = {
                    'bind': v if v.startswith('/') else os.path.join(image_workdir, v),
                    'mode': 'rw'
                }

And the working directory generally appears to be set to $HOME:

WORKDIR ${HOME}

If we can assume this will always be the case for repo2docker containers, than the code as it is currently, works, and there is no need to resolve $HOME.

There may have to be a caveat pointing out this doesn't work for fully customized containers(*), similar to the caveat about mounting to another location in the container (the two caveats would probably go together).

(*) I don't actually know if that's true: can a user's Dockerfile remove/ignore the working directory setting? The failing test case as described below suggests it can.


The thing that bugs me, and led me on this chase to $HOME, is that a simple test based on the dockerfile/simple test, but with ['-E', 'testdir'] added to the arguments, fails: in this case, image['ContainerConfig']['WorkingDir'] is empty.

To resolve that issue, I could

  • try and tweak the test
  • add a WORKDIR ${HOME} to the Dockerfile used in the test, as in the base Dockerfile image.
  • something else?

Any thoughts on this?

@betatim

This comment has been minimized.

Copy link
Member

betatim commented Oct 4, 2018

You could inspect image['ContainerConfig']['Env'] to find $HOME. This would make it a bit harder to reason about which directory inside the container will be used as it would be "Relative to WorkingDir if set, otherwise relative to $HOME if set, if neither is set relative paths aren't supported"

I think it is fine to assume that WorkingDir is set and error out if not. The general philosophy for repo2docker is that if you use your own Dockerfile you are an expert and know what you are doing. We offer very little support to you when things don't work beyond "do you really need to make your own Dockerfile?". Conversely we try to support the things a lot of users want to do natively so that they do not have to write a Dockerfile.

So I would keep the current behaviour and add a test that a Dockerfile with WorkingDir set works as expected.

@evertrol evertrol force-pushed the evertrol:add-editable-mode branch 2 times, most recently from 45d24f1 to 58c3bb2 Oct 4, 2018
@evertrol

This comment has been minimized.

Copy link
Contributor Author

evertrol commented Oct 4, 2018

I've added two tests:

  • a test that creates a file inside the container, for which it is checked that it can be viewed by the host (with its contents verified)
  • a test that creates a file on the host, while the container is running. The existence of that file is then verified inside the container (though not its contents).

The five-line fixture can be replaced by an import once #413 gets merged.


Additionally, I have converted the FAQ from Markdown to reStructuredText, since I wanted an admonition note, which is not possible with Markdown. Perhaps this change should be taken out of this PR and put into its own PR.

Lastly, I was looking for a CHANGES file to add this change, but there isn't any. Perhaps the repository is considered the CHANGES file, but generally, I find a separate file easier to read and spot (important) changes between versions, or bug-fixes.
Perhaps it's something to consider? Though I notice (sadly) few (notable) Python projects actually have such a file.

evertrol added 2 commits Oct 1, 2018
This adds an option to run from a local repository in edit mode, where
changes in a running Docker container (for example, through a
notebook) are reflected in the local repository.

Implements the feature suggested in #357
@evertrol evertrol force-pushed the evertrol:add-editable-mode branch from 16ee909 to a44340c Oct 5, 2018
@betatim

This comment has been minimized.

Copy link
Member

betatim commented Oct 8, 2018

Having a ChangeLog would be welcome at least by me. We haven't been very disciplined so far so if you oulc get us started that would be great.

Do you have a preferred format? My suggestion would be to start with a style a bit like https://github.com/scikit-learn/scikit-learn/blob/master/doc/whats_new/v0.19.rst#version-0191.

Changing from MD to rst would be ideal in a separate PR so we can keep reformatting separate from content changes.

evertrol added 4 commits Oct 4, 2018
This test only verifies the option one way: a change inside the
container (a new file) is reflected in the local host repository
outside the container.

A further test where a modification at the host level is reflected in
the container, is still neede.
This will test whether an externally (host) created file is seen
inside the container. This means starting the container, creating a
temporary file on the host, then running a simple verification command
(`ls`) to detect the file. Once the temporary file is removed on the
host side, it should be removed in the container as well.
The dockerfile test is used by test_editable, but should not be used
by the generated tests. Removing the verify script will prevent it
being run as part of the generated tests.
@evertrol evertrol force-pushed the evertrol:add-editable-mode branch from f425737 to 754edd0 Oct 8, 2018
This was referenced Oct 8, 2018
@evertrol

This comment has been minimized.

Copy link
Contributor Author

evertrol commented Oct 8, 2018

@betatim Thanks for all the feedback.

  • the faq.md to faq.rst change has been taken out, and is now its own PR
  • a suggested change log format is now submitted as a PR for comments
@betatim

This comment has been minimized.

Copy link
Member

betatim commented Oct 8, 2018

I'll merge this now to get this cool feature in there.

@betatim betatim merged commit a0e4dff into jupyter:master Oct 8, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@neuromusic

This comment has been minimized.

Copy link

neuromusic commented Nov 15, 2018

sorry for taking so long to say "thanks" but this is awesome! going to test it out ASAP

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.