rinikerlab / lightweight-registration

https://pubs.acs.org/doi/full/10.1021/acs.jcim.4c01133
MIT License
63 stars 14 forks source link

Some improvements to Dockerfile + container deployment? #61

Open hmacdope opened 3 months ago

hmacdope commented 3 months ago

Hi all,

Thanks you for the wonderful work! Very needed and fun to work with so far.

I have two small suggestions

  1. The current Dockerfile is not working (at least for me) and only contains rdkit and python. A faster solving, more lightweight container with a full environment could be advantageous (mamba docker images are great for this).
  2. It would be fantastic to have deployment of an already built container such that larger applications can just include this in a docker-compose setup. This can be done in a github workflow. I have made a demo in my fork of the repo. You can see the built package here: https://github.com/hmacdope/lightweight-registration/pkgs/container/lightweight-registration that you can try with docker run -it ghcr.io/hmacdope/lightweight-registration:main.

But of course you can decide which if any of these you would like to accept. I will open a PR and we can discuss there?

Cheers and thanks

Hugo

greglandrum commented 3 months ago

Hi Hugo,

Thanks for the contribution.

The current Dockerfile is really only there to be used in the CI tests for lwreg

Would it perhaps make more sense to create a second one that is actually intended for deployment?

hmacdope commented 3 months ago

Sounds very reasonable, where would you like it to go? devtools/deployment/ is normally what I use for stuff like this, but up to you.

greglandrum commented 3 months ago

I think maybe just deployment/, but I don't have a strong opinion. @brje01, what do you think?

hmacdope commented 3 months ago

Any further thoughts here? :smile_cat:

greglandrum commented 3 months ago

Any further thoughts here? 😸

I think it's ok to take silence for agreement. :-) Let's use just deployment/

hmacdope commented 2 months ago

Sorry took me a while to circle back to this. I have pushed the relevant changes in my PR #62.