Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add docker commands in ImageSpec #2676

Open
wants to merge 29 commits into
base: master
Choose a base branch
from

Conversation

mao3267
Copy link
Contributor

@mao3267 mao3267 commented Aug 11, 2024

Why are the changes needed?

We want to support adding other docker commands while creating ImageSpec.

What changes were proposed in this pull request?

  1. Add a new "docker_commands" member in ImageSpec, which is a list of docker commands.
  2. Insert these commands into the Dockerfile template.

How was this patch tested?

It can not use the new ImageSpec while creating a container remotely, therefore, we didn't add test at this time.
We provide an example of building an image.

from flytekit.image_spec import ImageSpec
from flytekit import task, workflow
import flytekit

image_spec = ImageSpec(
    name="my_image",
    registry="localhost:30000",
    builder="default",
    apt_packages=["wget", "git", "build-essential"],
    docker_commands=[
        "RUN git config --global user.email '[email protected]'",
        "RUN git config --global user.name 'mao3267'",
        "RUN git clone https://github.com/mao3267/flytekit.git",
        "RUN wget https://raw.githubusercontent.com/opensource4you/gossip/main/README.md",
        "COPY . /root",
    ],
)


@task(container_image=image_spec)
def my_task() -> str:
    try:
        with open("/root/README.md", "r") as f:
            return f.read().split("\n")[0]
    except Exception as e:
        return str(e) + f" {flytekit.__version__}"


@workflow
def my_wf() -> str:
    return my_task()

As the screenshots show, the image is successfully built. However, the task will fail since the flytekit version of the remote container is still 1.13.

Setup process

git clone https://github.com/mao3267/flytekit.git
git checkout add-docker-commands
make setup && pip install -e .

Screenshots

image image

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

None

Docs link

None

mao3267 and others added 22 commits August 9, 2024 20:07
* don't call remote

Signed-off-by: Yee Hing Tong <[email protected]>

* nit

Signed-off-by: Yee Hing Tong <[email protected]>

---------

Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: mao3267 <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Co-authored-by: Kevin Su <[email protected]>
Signed-off-by: mao3267 <[email protected]>
* Add an exeception when filters' value isn't a list

* Make the exception more specific

Signed-off-by: Nelson Chen <[email protected]>

* add an unit test for value_in

Signed-off-by: Nelson Chen <[email protected]>

* lint

Signed-off-by: Kevin Su <[email protected]>

---------

Signed-off-by: Nelson Chen <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Co-authored-by: Kevin Su <[email protected]>
Signed-off-by: mao3267 <[email protected]>
Signed-off-by: wayner0628 <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Co-authored-by: Kevin Su <[email protected]>
Signed-off-by: mao3267 <[email protected]>
* Show different of types in dataclass when transforming error

Signed-off-by: Future-Outlier <[email protected]>

* add tests for dataclass

Signed-off-by: Future-Outlier <[email protected]>

* fix tests

Signed-off-by: Future-Outlier <[email protected]>

---------

Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: mao3267 <[email protected]>
…an the entire bytes buffer (flyteorg#2641)

* pass the local file directly for streaming in FlyteRemote.upload_file

Signed-off-by: Reda Oulbacha <[email protected]>

* ruff format

Signed-off-by: Reda Oulbacha <[email protected]>

* add an integration test

Signed-off-by: Reda Oulbacha <[email protected]>

* remove unnecessary len

Signed-off-by: redartera <[email protected]>

* redo registration in the integration test

Signed-off-by: redartera <[email protected]>

* fix misspel

Signed-off-by: redartera <[email protected]>

* run the integration test serially

Signed-off-by: redartera <[email protected]>

* disable agent

Signed-off-by: Kevin Su <[email protected]>

* use os.stat instead of os.seek to determine content_length

Signed-off-by: redartera <[email protected]>

* rewrite tests only uploda a file, use a separate marker

Signed-off-by: redartera <[email protected]>

* parametrize integration test makefile cmd

Signed-off-by: redartera <[email protected]>

* add workflow_dispatch for debugging

Signed-off-by: redartera <[email protected]>

* replace trigger with push

Signed-off-by: redartera <[email protected]>

* remove trailing whitespaces

Signed-off-by: redartera <[email protected]>

* remove agent disabling

Signed-off-by: redartera <[email protected]>

* remove trailing debug CI trigger

Signed-off-by: redartera <[email protected]>

* clean up botocore imports

Signed-off-by: redartera <[email protected]>

---------

Signed-off-by: Reda Oulbacha <[email protected]>
Signed-off-by: redartera <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Co-authored-by: Kevin Su <[email protected]>
Signed-off-by: mao3267 <[email protected]>
…2652)

Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>
Signed-off-by: mao3267 <[email protected]>
…#2651)

Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>
Signed-off-by: mao3267 <[email protected]>
* return exceptions when gathering

Signed-off-by: Yee Hing Tong <[email protected]>

* pr comment

Signed-off-by: Yee Hing Tong <[email protected]>

---------

Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: mao3267 <[email protected]>
Signed-off-by: Thomas J. Fan <[email protected]>
Signed-off-by: mao3267 <[email protected]>
…eorg#2660)

* Add repro test case

Signed-off-by: Eduardo Apolinario <[email protected]>

* Restore loader_args in papermill plugin

Signed-off-by: Eduardo Apolinario <[email protected]>

* Add unit tests

Signed-off-by: Eduardo Apolinario <[email protected]>

---------

Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>
Signed-off-by: mao3267 <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>
Signed-off-by: mao3267 <[email protected]>
Copy link

codecov bot commented Aug 11, 2024

Codecov Report

Attention: Patch coverage is 38.46154% with 8 lines in your changes missing coverage. Please review.

Project coverage is 80.57%. Comparing base (9666f15) to head (0a85ccc).
Report is 8 commits behind head on master.

Files Patch % Lines
flytekit/image_spec/image_spec.py 22.22% 7 Missing ⚠️
...lytekit-envd/flytekitplugins/envd/image_builder.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #2676       +/-   ##
===========================================
- Coverage   93.48%   80.57%   -12.91%     
===========================================
  Files          17      292      +275     
  Lines         859    24420    +23561     
  Branches        0     4009     +4009     
===========================================
+ Hits          803    19677    +18874     
- Misses         56     4080     +4024     
- Partials        0      663      +663     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mao3267 mao3267 marked this pull request as ready for review August 13, 2024 02:31
@mao3267 mao3267 changed the title Add docker commands Add docker commands in ImageSpec Aug 17, 2024
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently the docker context for building is a temporary directory with the files copied over.

@mao3267 Do you think adding DOCKER_COMMANDS will be confusing in terms of Docker's context? A user may assume that the context is the current working directory.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.