DataONEorg / slinky

Slinky, the DataONE Graph Store
Apache License 2.0
4 stars 4 forks source link

Refactor Images to pip Install #23

Closed ThomasThelen closed 2 years ago

ThomasThelen commented 3 years ago

I feel like there's an identity crisis in this repository that's between the actual d1lod python library, the deployment of it, and the documentation.

For example, the fact that we need to copy the d1lod directory into the docker build directory, which then gets copied into the image, means something's not quite right. Doing this will bring about bugs in the future where the requirements.txt in the d1lod folder aren't installed; I'm assuming this is why some of the scheduler requirements are a subset of the d1lod requirements file (since d1lod is installed in the scheduler, the scheduler's requirements file gives us a way to install the d1lod requirements-without ever touching the d1lod requirements file) (and scheduler doesn't use python-dateutil, but has it defined in the requirements, probably so that d1lod has access to it).

If it were up to me, I'd create a separate repository for the python library, and document it as a standalone library which is standard enough. Then I'd create a second repository that's responsible for DataONE's deployment of services to spin up the graph store. One container in that stack uses the d1lod package and could be installed via a simple git clone & pip install, or just a pip install git+ . This would also allow for better placed documentation. Right now we have the docs/ folder, which is probably more appropriate in the python library's documentation (maybe as readthedocs). The architecture of the Slinky K8 stack would belong in the deployment repository.

At the least we should be able to pip install the d1lod folder. At this moment, attempting to do so raises the following

Screen Shot 2021-03-24 at 10 56 16 AM

Any thoughts?

amoeba commented 3 years ago

Sounds like you have a lot of experience I don't have yet so I'm happy to defer to you on some of the Python- and k8s-isms. The d1lod Python package needs to be mostly ripped out or at least overhauled anyway so I wouldn't stress too much about above issues like copying into images and not being able to pip-install. Those are probably just my goofs.

The current approach is advantageous because I could tweak the d1lod package source, re-build containers, and see my changes reflected immediately (without pushing to another repo then pulling down changes, or pushing to a package repository). So I'd love to keep that unless it causes more problems than its worth.

Would doing as you suggest above and just refactoring things so a local pip-install works be fine for now?

ThomasThelen commented 3 years ago

I think getting the pip install to work with pip install git+ would be huge for separating the build stuff from the actual contents of the repo.

The reason the previous approach was working was because the docker-compose file was at the project root-so it had access to d1lod and to the Dockerfiles. This is important because you're not able to include anything above the directory that the docker build command is run from-in this case the project root. After getting rid of the docker-compose file, the build command has to be run in each container's folder. For this reason, we can't copy d1lod as ../d1lod since that sort of directory traversal is forbidden by docker-hence the manual copying of the d1lod now.

That's a really good point about testing this stuff locally... pip install git+ would require a git push for every code change which would be a huge pain. I think this issue is coming from differing requirements from a CI perspective: ideally Slinky deploys on production without any sort of configuration needed (this is where the idea for installing via git comes from). From a development perspective we want to be able to create images from local directories. One compromise is having commented out lines in the Dockerfiles that should be uncommented for building locally vs deploying.

When it comes to running the stack under a test/dev environment with local images, the k8 deployment files will need to be modified to point to the local docker registry instead of dockerhub (which I should document somewhere)

amoeba commented 3 years ago

I did a bit of research on how to do a good job building Python packages and I think I have a good system in place at https://github.com/DataONEorg/slinky/tree/feature_update_graph_pattern/. The d1lod package now uses an entirely setup.py-based install so you can just pip install [-e] . it or pip install it from GitHub. I'm pretty happy with this for now because it supports building development images from local sources and also building release images from git branches/tags without any real fuss.

Some of the issues raised above aren't fully addressed by my changes so I'm going to leave this open until we can touch base on those.

ThomasThelen commented 3 years ago

I think that the pip install should get around the manual copying of the packages to the library folders-which I think was mixing me up the most. Once the pip refactor is complete I think this issue should be resolved.

edit: We still have to copy the source code into the container which I think was the other thing I was suggesting we get away from. The best solution to this is to be able to add the worker & scheduler to the requirements file. But specify a remote source (ie GitHub). But this would require separate repositories for both

ThomasThelen commented 2 years ago

Completed in #42 ; we can now pip install . in the root d1lod directory.