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

Allow user to set an install location for modules #55

Merged
merged 8 commits into from Apr 17, 2019

Conversation

pcuenca
Copy link
Contributor

@pcuenca pcuenca commented Apr 15, 2019

The preferred package install location can be set using the %install-location magic command, which points to the destination directory for compiled Swift modules. This allows the user to compile and install packages in a notebook (using the %install directives), and then import them in a different notebook without recompiling.

The compiled packages are built as dependencies of a jupyterInstalledPackages target. Note that compilation generates a dylib that must be dlopened before pre-built packages can be imported. For this reason, the .so file is also copied inside the module install location, and loaded when the Swift process is started.

A lock is taken while building and installing modules, to prevent conflicts between notebooks running simultaneously.

Limitations:

  • Build artifacts are not cached. When restarting a notebook, comment or skip installation directives (but keep %install-location) to prevent the recompilation of packages previously built.
  • jupyterInstalledPackages.so must depend on the modules that are going to be later used, possibly in a different notebook. This means that all packages should be built in the same notebook session.

pcuenca and others added 4 commits April 14, 2019 02:10
The preferred package install location can be set using the
%install-location magic command, which points to the destination
directory for compiled Swift modules. This allows the user to compile
and install packages in a notebook (using the %install directives), and
then import them in a different notebook without recompiling.

The compiled packages are built as dependencies of a
`jupyterInstalledPackages` target. Note that compilation generates a
dylib with that target, that must be `dlopen`ed before pre-built
packages can be imported. For this reason, the `.so` file is also
copied inside the package install location, and lazily opened when the
Swift process is started.

Limitations:
- Build artifacts are not cached. When restarting a notebook, comment
or skip installation directives (but keep `%install-location`) to
prevent the recompilation of packages previously built.
Run install_location_install first, then verify that the packages are
available for install_location_use.
Copy link
Contributor

@marcrasi marcrasi left a comment

Choose a reason for hiding this comment

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

Thanks, this seems like it will be very convenient to use!!

One principle that I think is very important is: Notebooks should not depend on external state (as far as possible). This way, people can download and run notebooks without needing to set anything up.

This change doesn't quite follow this principle, because it lets you write notebooks that require packages to be installed in a certain directory.

I think it'll be easy to update this PR to make notebooks self-contained: Just reuse the build artifacts, and call swift build every time. I've plated with swift build a bit, and it seems pretty good at recognizing when dependencies have changed and only rebuilding what is necessary. So we'll still get a large package installation speedup.

swift_kernel.py Outdated Show resolved Hide resolved
swift_kernel.py Outdated Show resolved Hide resolved
swift_kernel.py Show resolved Hide resolved
test/tests/notebooks/install_location_install.ipynb Outdated Show resolved Hide resolved
test/tests/notebooks/install_location_install.ipynb Outdated Show resolved Hide resolved
@pcuenca
Copy link
Contributor Author

pcuenca commented Apr 17, 2019

I think it'll be easy to update this PR to make notebooks self-contained: Just reuse the build artifacts, and call swift build every time.

Sure. That was actually the approach I first tried in pcuenca@81574b5. I can confirm the speedup. The problem of that revision was the whole concept of shared environment, I like the idea of allowing the user to define the location that we are doing here.

@marcrasi
Copy link
Contributor

Sure. That was actually the approach I first tried in pcuenca/swift-jupyter@81574b5. I can confirm the speedup. The problem of that revision was the whole concept of shared environment, I like the idea of allowing the user to define the location that we are doing here.

Yes, I also like allowing the user to define the location. My suggestion is to put the intermediate build artifacts there too, and reuse them if there are already any artifacts in that user-defined location.

This reverts commit 118dd8b.

I could have rebased and force-pushed, but I'm not sure about the PR
policies in place.
The Jupyter notebook library target will be rebuilt every time. However,
SPM properly determines the dependencies that need to be rebuilt, so no
unnecessary download / compilation steps should be necessary.

Regenerating the target is much better for consistency, and ensures the
notebook does not rely on external state.

In order to implement this change, an additional container directory
has been created, and that container is symlinked to the user-specified
location when appropriate.
@pcuenca
Copy link
Contributor Author

pcuenca commented Apr 17, 2019

I think I addressed all the issues in my last three commits.

@marcrasi
Copy link
Contributor

Oh no, test failure. I think it's because this branched off an older version of master where the tests don't pass any more. I will try to push a merge commit to this PR and rerun the tests.

@marcrasi marcrasi merged commit 8a90200 into google:master Apr 17, 2019
@pcuenca pcuenca deleted the package-install-location branch April 18, 2019 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants