flytekit icon indicating copy to clipboard operation
flytekit copied to clipboard

Docker plugin

Open danpf opened this issue 1 year ago • 2 comments

TL;DR

Add a plugin to build imagespec using a docker buildx driver/builder

Type

  • [ ] Bug Fix
  • [ ] Feature
  • [x] Plugin

Are all requirements met?

  • [x] Code completed
  • [ ] Smoke tested
  • [x] Unit tests added
  • [x] Code documentation added
  • [ ] Any pending items have an associated Issue

Complete description

This is a quick pass at a docker / buildkit builder for flytekit. Basically, this plugin provides an alternative interface to ImageSpec which will allow the usage of preexisting Dockerfiles that may be too complex to recreate with envd.

ImageSpec itself seems very tailored to envd, so I'm not sure if this is the best way to go about adding this functionality.

Notable changes

  • flytekit/remote/remote.py
    • alter task-id (version) based on ImageSpec
  • Because of how images are built/checked for existence i have to resort to some hacks that I don't think should make it into the final PR. See horrible_horrible_hack in plugins/flytekit-docker/flytekitplugins/docker/image_builder.py

Because of this, It will likely require a deeper discussion with the team before merging

  • [ ] Need to create requirements.txt file correctly (getting weird errors)
  • [ ] Test the buildkit_build_extra_output option
  • [ ] Check if it Is it possible to test this further with the current infrastructure
  • [x] Check that it works image

danpf avatar Oct 30 '23 21:10 danpf

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (f16ac49) 54.71% compared to head (e987942) 39.83%. Report is 12 commits behind head on master.

:exclamation: Current head e987942 differs from pull request most recent head 44f42cf. Consider uploading reports for the commit 44f42cf to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           master    #1926       +/-   ##
===========================================
- Coverage   54.71%   39.83%   -14.89%     
===========================================
  Files         305      170      -135     
  Lines       22732    16649     -6083     
  Branches     3453     3451        -2     
===========================================
- Hits        12438     6632     -5806     
+ Misses      10122     9861      -261     
+ Partials      172      156       -16     
Files Coverage Δ
flytekit/image_spec/image_spec.py 43.08% <100.00%> (+1.42%) :arrow_up:

... and 152 files with indirect coverage changes

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

codecov[bot] avatar Oct 31 '23 00:10 codecov[bot]

Yea I just wanted to get something that works by touching the least amount possible. I have an idea of what needs to happen, but I wanted to get some feedback before I dumped a bunch more time into it or pushed the scope wider.

I'll probably keep plugging away at this till I'm happy with it, but any direction / thoughts would be greatly appreciated so I don't spend too much time in the wrong places.

Briefly:

  • Should ImageSpec be abstracted to ImageSpecBase + ImageSpecEnvD + ImageSpecDockerfile?

    • I'm not a huge fan of this solution, I think it would be nice to not have the plugins installed on the node
    • The current way is a bit too cluttered for me as it somewhat defeats the purpose of having the plugin abstraction.
  • if non-python files are going to be considered during hashing - that has to be managed delicately as it is not what flyte currently does without copy-all

  • horrible_horrible_hack is because the Dockerfile isn't packaged with the script and sent to the remote. So when the remote calls .build_image and tries to determine a hash, it either crashes (folder doesn't exist) or it determines a new one and thus returns a new image_name. This causes the builder to try and build the image again and then crash because docker doesn't exist. To prove to myself it could be done I just used this hack but as previously mentioned I don't want it to be merged.

danpf avatar Nov 04 '23 04:11 danpf