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 'chenvincent610@gmail.com'",
        "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
Signed-off-by: mao3267 <chenvincent610@gmail.com>
Signed-off-by: mao3267 <chenvincent610@gmail.com>
* don't call remote

Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>

* nit

Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>

---------

Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>
Signed-off-by: mao3267 <chenvincent610@gmail.com>
Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Co-authored-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: mao3267 <chenvincent610@gmail.com>
* Add an exeception when filters' value isn't a list

* Make the exception more specific

Signed-off-by: Nelson Chen <asd3431090@gmail.com>

* add an unit test for value_in

Signed-off-by: Nelson Chen <asd3431090@gmail.com>

* lint

Signed-off-by: Kevin Su <pingsutw@apache.org>

---------

Signed-off-by: Nelson Chen <asd3431090@gmail.com>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Co-authored-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: mao3267 <chenvincent610@gmail.com>
Signed-off-by: wayner0628 <a901639@gmail.com>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Co-authored-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: mao3267 <chenvincent610@gmail.com>
* Show different of types in dataclass when transforming error

Signed-off-by: Future-Outlier <eric901201@gmail.com>

* add tests for dataclass

Signed-off-by: Future-Outlier <eric901201@gmail.com>

* fix tests

Signed-off-by: Future-Outlier <eric901201@gmail.com>

---------

Signed-off-by: Future-Outlier <eric901201@gmail.com>
Signed-off-by: mao3267 <chenvincent610@gmail.com>
Signed-off-by: mao3267 <chenvincent610@gmail.com>
…an the entire bytes buffer (flyteorg#2641)

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

Signed-off-by: Reda Oulbacha <reda@artera.ai>

* ruff format

Signed-off-by: Reda Oulbacha <reda@artera.ai>

* add an integration test

Signed-off-by: Reda Oulbacha <reda@artera.ai>

* remove unnecessary len

Signed-off-by: redartera <120470035+redartera@users.noreply.github.com>

* redo registration in the integration test

Signed-off-by: redartera <120470035+redartera@users.noreply.github.com>

* fix misspel

Signed-off-by: redartera <120470035+redartera@users.noreply.github.com>

* run the integration test serially

Signed-off-by: redartera <120470035+redartera@users.noreply.github.com>

* disable agent

Signed-off-by: Kevin Su <pingsutw@apache.org>

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

Signed-off-by: redartera <120470035+redartera@users.noreply.github.com>

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

Signed-off-by: redartera <120470035+redartera@users.noreply.github.com>

* parametrize integration test makefile cmd

Signed-off-by: redartera <120470035+redartera@users.noreply.github.com>

* add workflow_dispatch for debugging

Signed-off-by: redartera <120470035+redartera@users.noreply.github.com>

* replace trigger with push

Signed-off-by: redartera <120470035+redartera@users.noreply.github.com>

* remove trailing whitespaces

Signed-off-by: redartera <120470035+redartera@users.noreply.github.com>

* remove agent disabling

Signed-off-by: redartera <120470035+redartera@users.noreply.github.com>

* remove trailing debug CI trigger

Signed-off-by: redartera <120470035+redartera@users.noreply.github.com>

* clean up botocore imports

Signed-off-by: redartera <120470035+redartera@users.noreply.github.com>

---------

Signed-off-by: Reda Oulbacha <reda@artera.ai>
Signed-off-by: redartera <120470035+redartera@users.noreply.github.com>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Co-authored-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: mao3267 <chenvincent610@gmail.com>
…2652)

Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>
Co-authored-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>
Signed-off-by: mao3267 <chenvincent610@gmail.com>
…#2651)

Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>
Co-authored-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>
Signed-off-by: mao3267 <chenvincent610@gmail.com>
Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>
Signed-off-by: mao3267 <chenvincent610@gmail.com>
* return exceptions when gathering

Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>

* pr comment

Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>

---------

Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>
Signed-off-by: mao3267 <chenvincent610@gmail.com>
Signed-off-by: mao3267 <chenvincent610@gmail.com>
Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>
Signed-off-by: mao3267 <chenvincent610@gmail.com>
Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>
Signed-off-by: mao3267 <chenvincent610@gmail.com>
Signed-off-by: Chen Zhu <chzhu@linkedin.com>
Signed-off-by: Chen Zhu <zhuchen1033@gmail.com>
Signed-off-by: mao3267 <chenvincent610@gmail.com>
…eorg#2660)

* Add repro test case

Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>

* Restore loader_args in papermill plugin

Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>

* Add unit tests

Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>

---------

Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>
Co-authored-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>
Signed-off-by: mao3267 <chenvincent610@gmail.com>
Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>
Signed-off-by: mao3267 <chenvincent610@gmail.com>
…2665)

Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: mao3267 <chenvincent610@gmail.com>
Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>
Co-authored-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>
Signed-off-by: mao3267 <chenvincent610@gmail.com>
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.

Signed-off-by: mao3267 <chenvincent610@gmail.com>
Signed-off-by: mao3267 <chenvincent610@gmail.com>
@mao3267 mao3267 marked this pull request as ready for review August 13, 2024 02:31
Signed-off-by: mao3267 <chenvincent610@gmail.com>
Signed-off-by: mao3267 <chenvincent610@gmail.com>
Signed-off-by: mao3267 <chenvincent610@gmail.com>
Signed-off-by: mao3267 <chenvincent610@gmail.com>
@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.