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

Updates for Stencila #457

Merged
merged 26 commits into from Nov 9, 2018

Conversation

Projects
None yet
5 participants
@nuest
Contributor

nuest commented Oct 30, 2018

  • detect required contexts automatically
  • install R if needed
  • update dep versions
  • do not do anything if binder/ is present
  • update docs
  • more strictly test Stencila installation in verify

Note that the docs contain links to two example repositories (https://github.com/nuest/stencila-py and https://github.com/nuest/stencila-r) who have not been transfered yet to the binder-examples org. If you'd welcome the changes in this PR, we can sort that out.

@nuest nuest changed the title from [WIP] Updates for Stencila to Updates for Stencila Oct 30, 2018

@nuest nuest referenced this pull request Oct 30, 2018

Closed

Sprint wrap-up blog post #12

20 of 21 tasks complete
@@ -518,14 +556,28 @@ def get_assemble_scripts(self):
))
except FileNotFoundError:
pass
if self.stencila_manifest_contexts:
for context in self.stencila_manifest_contexts:
if(context == 'py' or context == 'pyjp'):

This comment has been minimized.

@minrk

minrk Oct 30, 2018

Member

Could both of these be true? If so, maybe if {'py', 'pyjp'}.intersection(self.stencila_manafest_contexts): to only trigger this once.

This comment has been minimized.

@nuest

nuest Oct 30, 2018

Contributor

Yes, a document can contain both py and pyjp code chunks. Neat trick, I must get more used to truthiness in Python :-).

This comment has been minimized.

@nuest

nuest Oct 30, 2018

Contributor

Regardin triggering: This would have been evaluated only once though because of the internal self._stencila_manifest_contexts, right? Or would that be once be class instance, not once per session?

Show resolved Hide resolved docs/source/config_files.rst Outdated
Show resolved Hide resolved repo2docker/buildpacks/r.py Outdated
@@ -64,21 +71,22 @@ def detect(self):
unless a `requirements.txt` is present and we do not want to require the
presence of a `requirements.txt` to use R.
Instead we just check if runtime.txt contains a string of the form
`r-<YYYY>-<MM>-<DD>`
Instead we check the options described in the class comment above.

This comment has been minimized.

@betatim

betatim Oct 30, 2018

Collaborator

I'd delete this line if we don't want to duplicate the contents of the doc string for the class. Ideally we'd be able to come up with a nice short summary about all the various ways to trigger the R buildpack but I currently can't think of one :-(

This comment has been minimized.

@nuest

nuest Oct 30, 2018

Contributor

Deleted. Your call, really. I guess the "nice short summary" means it can't be automated.

Do you think there should be a developer-facing variant of the config files docs?

This comment has been minimized.

@betatim

betatim Oct 31, 2018

Collaborator

Some developer facing docs would be good. Maybe building on https://repo2docker.readthedocs.io/en/latest/architecture.html?

@@ -19,7 +19,14 @@ class RBuildPack(PythonBuildPack):
date snapshot of https://mran.microsoft.com/timemachine
from which libraries are to be installed.
2. An optional `install.R` file that will be executed at build time,
2. A `DESCRIPTION` file signaling an R package, or

This comment has been minimized.

@betatim

betatim Oct 30, 2018

Collaborator

Maybe we should split this discussion into two things: how we detect that R needs installing and how to specify which libraries should be installed.

A more eloquent version of:

Setup R + RStudio if runtime.txt or DESCRIPTION or jats thing

Libraries will be installed if specified in install.R or as dependencies in DESCRIPTION
Show resolved Hide resolved repo2docker/buildpacks/r.py Outdated
Show resolved Hide resolved repo2docker/buildpacks/base.py Outdated
Show resolved Hide resolved repo2docker/buildpacks/base.py Outdated
Show resolved Hide resolved repo2docker/buildpacks/base.py Outdated
Show resolved Hide resolved repo2docker/buildpacks/r.py Outdated
@betatim

This comment has been minimized.

Collaborator

betatim commented Oct 30, 2018

Is it easy to add a test to check that stencila can run the document in the repository? As a way to check all the plumbing is connected properly.

betatim and others added some commits Oct 30, 2018

Fix wording in config_files.rst
Co-Authored-By: nuest <daniel.nuest@wwu.de>

@nuest nuest changed the title from Updates for Stencila to [WIP] Updates for Stencila Oct 30, 2018

@nuest

This comment has been minimized.

Contributor

nuest commented Oct 30, 2018

@betatim Thanks so much for the thorough review! Here's my to-do list:

  • add a check that Stencila can "run the document" - @nokome, any hint for this? Is there something like knitr:render(.) on CLI for both stencily/py and stencila/r
  • use XML parser to extract contexts, not a regex
  • use XML parser to extract the document path instead of looking for *.jats.xml
@choldgraf

This comment has been minimized.

Collaborator

choldgraf commented Nov 6, 2018

hmmmmm - looks like we've got a travis timeout on the stencila build!

https://travis-ci.org/jupyter/repo2docker/jobs/451056819#L700

any ideas?

@nuest nuest force-pushed the nuest:stencila branch from 7a0b2a0 to 490962e Nov 6, 2018

Show resolved Hide resolved repo2docker/buildpacks/base.py Outdated
Show resolved Hide resolved repo2docker/buildpacks/base.py Outdated
Show resolved Hide resolved repo2docker/buildpacks/r.py Outdated
- as dependencies in a `DESCRIPTION` file
- are needed by a specific tool, for example the package `stencila` is
installed and configured if a Stencila document is given.

This comment has been minimized.

@betatim

betatim Nov 7, 2018

Collaborator

I think this bit is about installing user specified packages so we should only list the ways by which a user can specify "please install X and Y" instead of listing the situations in which extra packages will be installed from a developer point of view.

The suggestion on L29 is connected to this.

@betatim

This comment has been minimized.

Collaborator

betatim commented Nov 7, 2018

I left a few more comments on docs and Python style. It would be great if you could run pep8 on your diff to find a nice solution for some of the lines that are (very) long.

Show resolved Hide resolved repo2docker/buildpacks/base.py Outdated
Show resolved Hide resolved repo2docker/buildpacks/base.py Outdated

betatim and others added some commits Nov 7, 2018

Update repo2docker/buildpacks/base.py
Co-Authored-By: nuest <daniel.nuest@wwu.de>
Update repo2docker/buildpacks/base.py
Co-Authored-By: nuest <daniel.nuest@wwu.de>
Update docs in buildpacks/r.py
Co-Authored-By: nuest <daniel.nuest@wwu.de>
@nuest

This comment has been minimized.

Contributor

nuest commented Nov 9, 2018

Re. faster installation (@nokome): I played around a bit with installing pre-compiled dependencies, but that turned out to be a rabbit hole, see https://gist.github.com/nuest/8beca3b75bba97f107a314798879a2fc

@nuest nuest force-pushed the nuest:stencila branch from 6804676 to 68194a0 Nov 9, 2018

@nuest nuest changed the title from [WIP] Updates for Stencila to Updates for Stencila Nov 9, 2018

@nuest

This comment has been minimized.

Contributor

nuest commented Nov 9, 2018

Tests pass again, I hope I could address all suggestions appropriately.

@betatim betatim merged commit 58cbe17 into jupyter:master Nov 9, 2018

4 checks passed

ci/circleci: build_docs Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 0%)
Details
codecov/project 76.3% (target 0%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@betatim

This comment has been minimized.

Collaborator

betatim commented Nov 9, 2018

Merged! Thanks for all the patience and iterations!

Do you want to deploy this to mybinder.org? We typically do deploys on Tuesday and Thursdays so that people can plan to be around for a bit after the deploy in case of problems. To make the change to mybinder.org checkout our SRE guide.

@nuest

This comment has been minimized.

Contributor

nuest commented Nov 9, 2018

@betatim Thanks to you for the good feedback and thorough checking. Yes, I very much would like to deploy this to mybinder, will take a look at the docs.

@choldgraf

This comment has been minimized.

Collaborator

choldgraf commented Nov 9, 2018

wohooo! congrats @nuest and thanks for the contribution!!!

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