R1: General comments

[mathieuboudreau]

Assignee: @manojneuro

General recommendations

• Consider either linking the binder button directly to the first notebook or to the notebooks folder.
• Would benefit from some general cleanup in the repo (e.g. multiple "requirements.txt" files (in tutorials/ and in in binder/), makes it difficult to know which one is used in an environment).
• Clarifying somewhere the Python version could be beneficial to the students (i.e. let them know it's 3.7.1 or >=3.7.1 so that users who have only ever used 2.7 will know they need to look up updated syntaxes) . This could be shown in either the README or in one of the first notebooks.
• I believe the proper name for the repo2data file herehttps://github.com/neurolibre/brainiak-tutorials/blob/master/binder/_data_requirement.json shouldn't begin with an underscore, correct @ltetrel ?
• In the requirements.txt and apt.txt file, a better practice would be to specify a fixed version for all the packages (using package==x.x.x for requirements.txt, and package=x.x.x for apt.txt). An alternative would be to specify a version >= so that you can always have the latest version, but at least you'll know what versions all the packages were originally were you need revert back to them.

[pbellec]

@manojneuro any update?

[manojneuro]

@pbellec the binder button suggestion has been fixed in PR #18. We are working on fixing the version numbers in the build files.

[mihaic]

I'll work on adding version numbers for requirements.txt and apt.txt in as >= relations.

Regarding the multiple requirements.txt, could I please have a pointer to the documentation for maintaining this repository in relation to the upstream repository? For example, are we supposed to update the files in the binder directory manually based on upstream changes?

[ltetrel]

Hi,

So I am not sure to understand your question but here is a documentation with all the possible requirements.txt :
https://mybinder.readthedocs.io/en/latest/sample_repos.html

If there are problems with the requirements, we will keep track of this inside an issue in this repository. All the changes should be made on this repository since it is this one that binder points to.

[mihaic]

Thanks for the documentation pointer. I asked because I am trying to understand what we need to do in this Neurolibre fork when we make changes upstream. As you can imagine, we are trying to keep the maintenance simple. Ideally, requirements.txt in the binder directory will be a copy of the upstream requirements.txt and apt.txt will be a copy of the relevant upstream Dockerfile section. We would add version specifiers upstream to enable this setup. Does that sound reasonable?

[ltetrel]

Yes sure, what do you think @mathieuboudreau ?

[mathieuboudreau]

If this setup works, it sounds fine with me!

[pbellec]

@mihaic sorry for the slow answer. You can think of a neurolibre publication as a release. You would not continually push changes from upstream. If you want to publish an update, you can start a new submission, and simply do a pull request from the master of your repo (or any specific release) to the master of the neurolibre repo. We will review all the changes and update the publication. Or we can publish it as a separate entry, with a different version. I hope this clarifies the process.

[mihaic]

Thanks, @pbellec. Your explanation is very helpful. The workflow makes sense to me.

[mihaic]

I added PR #20 for requirements.txt.

For apt.txt, I think it is counter-productive to use versions. As far as I understand, the underlying Docker image is buildpack-deps:bionic as in the repo2docker upstream (https://github.com/jupyter/repo2docker/blob/master/repo2docker/buildpacks/base.py#L17), which uses Ubuntu Apt repositories. As far as I know, Ubuntu does not keep old versions of packages in the repositories, so if we add versions, building the Docker image would fail every time there is an update (e.g., security update). On the bright side, Ubuntu packages are unlikely to introduce breaking changes.