PyWorkflowApp / visual-programming

A Python Visual Programming Workspace for Data Science
MIT License
31 stars 12 forks source link

WIP: Docker containers #75

Closed reelmatt closed 4 years ago

reelmatt commented 4 years ago

This contains the work @matthew-t-smith did on Dockerizing the front-/back-ends. Please include/correct anything that might be missing!

Build each end separately:

Build/run the two together: In the root directory run docker-compose up. This should start both containers and have them talk to each other. When you visit http://localhost:3000/, you may need to refresh the page once to get the Node list on the side. This bug was fixed in #76.

reelmatt commented 4 years ago

I pushed a few tweaks, all minor things (a missing hypen, quotes, and a backslash)!

This now works for me with docker-compose up in the root dir and the separate apps can be spun up with

docker build --tag backendtest:2.0 .
docker run -p 8000:8000 --name be backendtest2.0

and

docker build --tag frontendtest:2.0 .
docker run -p 3000:3000 --name fe frontendtest2.0
matthew-t-smith commented 4 years ago

A few more changes. Still having this one issue:

back-end_1   | [28/Apr/2020 05:45:55] "GET /workflow/nodes HTTP/1.1" 500 23
back-end_1   | [28/Apr/2020 05:45:55] "POST /workflow/new HTTP/1.1" 200 78

@reelmatt can you check on the GET nodes that seems to be failing? Can see it's finding the workflow views alright now.

I will also double back when this gets resolved to fix up the Postman Tests that are surely failing with the directory changes for back-end.

reelmatt commented 4 years ago

@matthew-t-smith We may be having some differences with where directories are being located in the Docker container. Made a comment about adding the cd vp part back to the docker-compose.yml for the back-end.

The issue with GET /workflow/nodes is an order-of-operations issue introduced in one of the latest PRs. The Node listing now requires a Workflow object to know where to look for the Nodes/handle custom node creation. So the fix is just reversing the two calls to do /workflow/new first followed by /workflow/nodes. If you refresh the page once after it loads, you should see the listing pop in.

I know @reddigari has done a lot of front-end improvements in the mean time, so I think it might be easier to make a change in one of his open PRs, or make the change after this PR is merged to reduce conflict headaches.

Re: testing, I think it's actually a problem with the pyworkflow unit tests. If I add cd back-end to the workflow file before pipenv install and test locally, everything is fine. There may be a Python version issue though with the container GitHub Actions uses to run the workflow though. We might need to revert the update to the Pipfile to go from 3.8.2 back to 3.8 (or 3.8.1, that's what looks like is the latest version for default GitHub Actions—hard to confirm without it doing a run).

reelmatt commented 4 years ago

@matthew-t-smith I committed the changes to fix the GitHub Action runner; it now finds and passes the tests.

Have you looked into the discrepancy between including cd vp in the docker-compose.yml that I commented on above? The build fails when I do not include it, but I could be missing something. If we figure that out, I think we're good to merge so I'm going to open this up for review.

reddigari commented 4 years ago

A little confused by the volume mounts in docker-compose.yml, since the individual Docker files copy the source directories into the image. Likewise the front end build runs npm install, so we shouldn't need to mount node_modules. Are the mounts just for the sake of keeping the image up to date as we develop?

reddigari commented 4 years ago

Just a thought: would it be possible to include an option for a user to mount their own custom nodes directory into the pyworkflow custom nodes location? They could store their nodes locally and then have access to them as soon as the container is running as opposed to manually uploading each one, and having them disappear when the container restarts.

matthew-t-smith commented 4 years ago

@reelmatt - I think I have an idea on the directory issue. I’ll do some testing Sunday to be sure.

@reddigari - the directory mounts were largely following some other samples. I’ll double check on those. The node modules should be ignored in the images so the container is lighter, but I will make sure that’s actually happening. Also, not sure on the option of loading into the container’s directory structure. Interesting, but once I get this updated, I can see.

reddigari commented 4 years ago

As far as I understand it (which is without much dev ops experience), the volume mounts are so a running container reflects changes you make locally without having to rebuild. But the image we upload for people to pull and run needs to have the released source code copied (and dependencies installed). That's what's handled by the COPY statements, and running npm install (which will create and populate node_modules in the container).

Using build args to decide whether to run the COPY and install commands is one option, but I've also seen folks maintain two Dockerfiles, one called Dockerfile that runs all the copy/install stuff, and one called Dockerfile.dev, for development, that does not. But I'm not sure how to make that play with docker-compose.

matthew-t-smith commented 4 years ago

Abandoning in favor of PR #84