Garden-AI / garden

https://garden-ai.readthedocs.io
MIT License
20 stars 4 forks source link

336 pre install garden in base images #373

Closed OwenPriceSkelly closed 10 months ago

OwenPriceSkelly commented 10 months ago

closes #336, plus addresses Logan's gripe about including requirements.txt or conda environment.yml files.

Overview

The new functionality is all in build_image_with_dependencies, and all of the garden-ai notebook commands now accept an optional --requirements path to either a pip- or conda-style requirements file.

local data still only tracks the most recently selected --base-image instead of storing the bespoke local images generated by the new function

Discussion

My first few stabs at this I had the CLI either remember the most-recently-used requirements file along with the base image and/or caching the pre-baked image instead, but it felt pretty unwieldy for not a lot of benefit; the layer caching we get for free with docker is basically just as good as caching the custom image in local data.

The potential downside of this is that users need to specify the --requirement argument for both notebook start and notebook publish every time, which might be surprising because it's different to the --base-image behavior.

Testing

I tested this manually end-to-end with a dummy entrypoint that just reports the version of a given package in sys.modules and sanity-checked with packages installed via a requirements.txt and an environment.yml.

I also had chatgpt write me a few happy path unit tests.

Documentation

No docs updates besides CLI help text / docstrings


📚 Documentation preview 📚: https://garden-ai--373.org.readthedocs.build/en/373/

codecov-commenter commented 10 months ago

Codecov Report

Attention: 27 lines in your changes are missing coverage. Please review.

Comparison is base (9f051a0) 75.89% compared to head (086f0e3) 75.80%.

Files Patch % Lines
garden_ai/app/notebook.py 4.16% 23 Missing :warning:
garden_ai/containers.py 88.57% 4 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #373 +/- ## ========================================== - Coverage 75.89% 75.80% -0.10% ========================================== Files 42 42 Lines 2344 2426 +82 ========================================== + Hits 1779 1839 +60 - Misses 565 587 +22 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

WillEngler commented 10 months ago

The potential downside of this is that users need to specify the --requirement argument for both notebook start and notebook publish every time, which might be surprising because it's different to the --base-image behavior.

Admittedly the --base-image behavior is a little awkward itself. Maybe as part of #362 we should think through both the --base-image and --requirements behavior together. But for now I think your solution is a huge improvement.

the layer caching we get for free with docker is basically just as good as caching the custom image in local data.

Good realization, and I love that this keeps things pretty simple for our implementation.