search

chime-experiment / ch_util

CHIME utilities
https://chime-experiment.github.io/ch_util
MIT License
3 stars 3 forks source link

Dockerfile #38

Closed aelanman closed 2 years ago

aelanman commented 2 years ago

Adds a Dockerfile for building a docker image with ch_util installed. Needed for outriggers.

jrs65 commented 2 years ago

@aelanman my initial reaction to this is that a Dockerfile for building ch_util doesn't need to be in the repo itself, you can host that elsewhere as it doesn't really serve anyone else.

I'm also not particularly happy to change the entire build system over to poetry as a side effect of this PR. I'd like to have a broader plan for the CHIME repos about moving to a modern build system, and of the modern build systems poetry is my least favourite as my previous experiences have been pretty frustrating (overall I'd probably prefer sticking with setuptools and its new PEP517 backend, or flit). I think @ljgray has some thoughts about this too.

jrs65 commented 2 years ago

I guess to expand on the Dockerfile point. Docker containers are built for specific tasks and so while they make sense in a service like coco/comet, I'm not sure why you would want one attached to a specific library? Like, what if someone decides that the outriggers instance also needs access to the kotekan-python code (as a random example), do you add that in here? have another Dockerfile deriving from this one to add the dependency?

ljgray commented 2 years ago

@aelanman my initial reaction to this is that a Dockerfile for building ch_util doesn't need to be in the repo itself, you can host that elsewhere as it doesn't really serve anyone else.

I'm also not particularly happy to change the entire build system over to poetry as a side effect of this PR. I'd like to have a broader plan for the CHIME repos about moving to a modern build system, and of the modern build systems poetry is my least favourite as my previous experiences have been pretty frustrating (overall I'd probably prefer sticking with setuptools and its new PEP517 backend, or flit). I think @ljgray has some thoughts about this too.

It looks like the poetry change is gone in 345c86a so I won't add any comments about it here, but I agree that the Dockerfile shouldn't really be in the repo since it's specific to the outriggers while the repo services multiple projects.

jrs65 commented 2 years ago

Ah, thanks. Hadn't seen that the commits had been removed. I'm a bit happier about just putting a Dockerfile in there, it doesn't really hurt, but I'd suggest thinking if there's a better place for it. Like, do you really want to wait on us reviewing it every time you need to change it? If there isn't, so be it, it can go here, although perhaps not at the top level (maybe scripts/ ?)

aelanman commented 2 years ago

I had figured it made sense to include this file in the repo, since all the other Dockerfiles used for outriggers are currently living in the repositories of the main packages they install. However, I think you're right that it makes more sense for Dockerfiles to live in the main outrigger-devops repo, so I'll close this PR, delete the branch, and move the Dockerfile there. Thanks!