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

stencila support #309

Merged
merged 5 commits into from Aug 1, 2018

Conversation

Projects
None yet
4 participants
@minrk
Member

minrk commented May 11, 2018

Supports stencila editor for DAR files

Additionally separates get_build_env from get_env to allow defining environment variables that do not affect build, which would force a rebuild from an earlier stage.

Developed at the eLife Innovation Sprint

TODO:

  • enable R runtime
  • remove hardcoded numpy/matplotlib installs in favor of real dependencies
  • remove hardcode of default archive name
  • merge upstream fixes for nbserverproxy
  • tests

@minrk minrk force-pushed the minrk:stencila branch 2 times, most recently from 2df740a to b28a8a0 May 11, 2018

@choldgraf

This comment has been minimized.

Collaborator

choldgraf commented May 11, 2018

This is awesome!

system, and the value is the destination file path inside the
container image.
"""
return super().get_build_script_files()

This comment has been minimized.

@betatim

betatim May 11, 2018

Collaborator

Do we need this if it just calls super()?

This comment has been minimized.

@minrk

minrk May 23, 2018

Member

Not at the moment. It used to have things, and it may again before we are done, so I haven't cleaned it out just yet. I'll remove this if it turns out not to be needed in the end.

"${NB_USER}",
# install nbserverproxy
r"""
pip install --no-cache https://github.com/minrk/nbserverproxy/archive/del-starting.tar.gz

This comment has been minimized.

@betatim

betatim May 11, 2018

Collaborator

Switch to a tag/commit on the official repo?

This comment has been minimized.

@minrk

minrk May 23, 2018

Member

Yes, now that it's been merged.

@betatim

This comment has been minimized.

Collaborator

betatim commented May 11, 2018

I was confused after reading the code because I was expecting it to look for dependencies to install or something like that. Does that happen somewhere?

Then I saw https://twitter.com/NokomeBentley/status/994831220512092162 and played with it a bit. Now I am thinking stencila is more like nteract-on-jupyter than a new build pack? /me is confused.com

@choldgraf

This comment has been minimized.

Collaborator

choldgraf commented May 12, 2018

@betatim FWIW, apparently nteract now runs on Jupyter as well https://blog.nteract.io/nteract-on-jupyter-53cc2c38290d

@betatim

This comment has been minimized.

Collaborator

betatim commented May 12, 2018

@betatim FWIW, apparently nteract now runs on Jupyter as well https://blog.nteract.io/nteract-on-jupyter-53cc2c38290d

That is what I meant with nteract-on-jupyter :) The way we added that to repo2docker was adding a new dependency. Which is why I am confused, because this is a build pack.

@minrk minrk referenced this pull request May 23, 2018

Open

Dependencies #7

@minrk minrk closed this May 23, 2018

@minrk minrk deleted the minrk:stencila branch May 23, 2018

@choldgraf

This comment has been minimized.

Collaborator

choldgraf commented May 23, 2018

@minrk any reason this was closed abruptly?

@minrk minrk restored the minrk:stencila branch May 24, 2018

@minrk minrk reopened this May 24, 2018

@minrk

This comment has been minimized.

Member

minrk commented May 24, 2018

@choldgraf oops. I think i may have accidentally deleted the branch. Reopened.

I've been working on https://github.com/minrk/nbstencilaproxy consolidating much of this.

In the end, I think the logic for stencila should be:

  1. detect/install dependencies as usual for any other buildpack
  2. locate DAR archive directory and set STENCILA_ARCHIVE_DIR
  3. if found, add/enable nbstencilaproxy server extension
  4. optionally, set default url to /stencila/ to avoid needing to set urlpath

It's only the STENCILA_ARCHIVE_DIR bit that can't be addressed by just adding a package to the dependencies in a regular repo of the other flavors. Maybe there's a way to do that?

Basically, this should be treated just like nteract as @betatim mentioned, but optional instead of in every env by default.

@choldgraf

This comment has been minimized.

Collaborator

choldgraf commented May 24, 2018

sounds good to me re: the logic!

@nuest nuest referenced this pull request Jun 22, 2018

Closed

Sprint wrap-up blog post #12

20 of 21 tasks complete
@nuest

This comment has been minimized.

Contributor

nuest commented Jun 22, 2018

@minrk How can I help to get this merged? I'd be happy to put in some time, but would appreciate your guidance on which tasks I don't need 10 times longer than you do ;-)

Is the task list above complete? AFAICS "enable R runtime" can be checked.

minrk added some commits May 11, 2018

@minrk minrk force-pushed the minrk:stencila branch from b28a8a0 to b13f5e6 Jul 21, 2018

rework stencila as an ‘optional frontend’
instead of a buildpack

stencila will be installed via `nbstencilaproxy` if a stencila manifest.xml is found

@minrk minrk force-pushed the minrk:stencila branch from b13f5e6 to 4aff6c5 Jul 21, 2018

@minrk minrk changed the title from [WIP] stencila support to stencila support Jul 21, 2018

@minrk

This comment has been minimized.

Member

minrk commented Jul 21, 2018

This actually works now! Instead of a StencilaBuildPack, stencila is handled in the BaseImage buildpack. It's managed as an 'additional frontend' setting up the nbstencilaproxy extension when it finds a stencila doc.

@choldgraf

This comment has been minimized.

Collaborator

choldgraf commented Jul 24, 2018

woot! so.... 🚢 ?

@minrk

This comment has been minimized.

Member

minrk commented Aug 1, 2018

I'm happy to ship it

@choldgraf choldgraf merged commit 7552361 into jupyter:master Aug 1, 2018

3 checks passed

codecov/patch 96.29% of diff hit (target 0%)
Details
codecov/project 54.32% (target 0%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@choldgraf

This comment has been minimized.

Collaborator

choldgraf commented Aug 1, 2018

then may the merge gods look favorably on your path, PR #309!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment