Graphegon / pygraphblas

GraphBLAS for Python
https://graphegon.github.io/pygraphblas/pygraphblas/index.html
Apache License 2.0
343 stars 27 forks source link

Inconsistency in working directories and test.sh #55

Closed marci543 closed 4 years ago

marci543 commented 4 years ago

Dockerfile-minimal installs pygraphblas to /pygraphblas and uses it as working directory, but the notebook variant uses /home/jovyan. @michelp, what is the reason behind the difference? Is this a way to avoid permission problems? https://github.com/michelp/pygraphblas/blob/62a630e737a242a70214e7a86b0fefbe36cbdfd3/Dockerfile-minimal#L14-L15 https://github.com/michelp/pygraphblas/blob/62a630e737a242a70214e7a86b0fefbe36cbdfd3/Dockerfile-notebook#L43-L44

This causes inconsistency in test.sh, which copies the current source folder to /home/jovyan/pygraphblas, but runs the tests from the working directory (/pygraphblas for minimal image). In this case the source code and tests from the image itself are tested instead of the current source folder in the host file system. https://github.com/michelp/pygraphblas/blob/62a630e737a242a70214e7a86b0fefbe36cbdfd3/test.sh#L5

michelp commented 4 years ago

The jovyan thing with notebook images is still a bit of a mystery to me, I haven't had the time to dig into the details of how the jupyter notebook images are built, so I split the Dockerfiles as an expedient. Perhaps there is a better approach, like using a --build-arg to pass the working directory to one Dockerfile that can build them both.

Maybe another approach is to just put the minimal code in /home/jovyan as well? It will confuse people but also allow the files to unify. Thoughts?

marci543 commented 4 years ago

It seems to me that the Jupyter images use jovyan to have a non-root user who can run the notebooks and has the same UID and GID as the user from the host to avoid permission problems if host folders are mounted to the container. https://github.com/jupyter/docker-stacks/blob/master/base-notebook/Dockerfile In our case, installation of packages and SuiteSparse must use root user, but the Python installations can run as non-root if I am right.

I think the pygraphblas code can be kept in /pygraphblas directory for both images and it can be used as working directory. Source folder from the host can be mounted to override this folder for development purposes, e.g. in test.sh.

Multi-stage build might be used to combine the Dockerfiles, but I am not sure how it works.

marci543 commented 4 years ago

A similar inconsistency (at least for me) around building and testing is that build.sh runs a clean build based on a branch. https://github.com/michelp/pygraphblas/blob/62a630e737a242a70214e7a86b0fefbe36cbdfd3/build.sh#L13-L17 Usually I would do some modifications, run build.sh and then run the tests with test.sh. The current version does not support this, since only a branch on the remote can be built and tested. What workflow do you recommend? What do you think about separating the core parts of build.sh and the git clone and docker push part of it? The latter parts can go into something like build_and_deploy.sh.

michelp commented 4 years ago

Oops missed the part where you pushed some changes to this, I also pushed a fix that lets you build from a local repo and optionally pushes to dockerhub in docker-build.sh. Does this work for you needs? Otherwise I'll revert and use yours.

marci543 commented 4 years ago

Your solution is great. I have extended it in #56.