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

Use ImageSpec inside raw container task #1944

Merged
merged 5 commits into from
Nov 22, 2023
Merged

Conversation

pingsutw
Copy link
Member

@pingsutw pingsutw commented Nov 8, 2023

TL;DR

Build ImageSpec if people pass a imageSpec inside raw container task.

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

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

Complete description

(flytekit-3.10) ➜  flytekit git:(image_spec_raw_container) ✗ ls flyte-example/raw_container                                 
__pycache__              calculate-ellipse-area.R install-readr.R          raw_container_wf.py
(flytekit-3.10) ➜  flytekit git:(image_spec_raw_container) ✗ pyflyte register --non-fast --version v1 flyte-example/raw_container/raw_container_wf.py
from flytekit import ContainerTask, kwtypes, workflow, ImageSpec

image_spec = ImageSpec(registry="pingsutw", base_image="r-base")

calculate_ellipse_area_r = ContainerTask(
    name="ellipse-area-metadata-r",
    input_data_dir="/var/inputs",
    output_data_dir="/var/outputs",
    inputs=kwtypes(a=float, b=float),
    outputs=kwtypes(area=float, metadata=str),
    image=image_spec,
    command=[
        "Rscript",
        "--vanilla",
        "/root/calculate-ellipse-area.R",
        "{{.inputs.a}}",
        "{{.inputs.b}}",
        "/var/outputs",
    ],
)


@workflow
def wf(a: float = 3.0, b: float = 4.0):
    calculate_ellipse_area_r(a=a, b=b)
image

Tracking Issue

flyteorg/flyte#4070

Follow-up issue

NA

Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
@pingsutw
Copy link
Member Author

pingsutw commented Nov 8, 2023

cc @zeryx

Copy link

codecov bot commented Nov 8, 2023

Codecov Report

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

Comparison is base (bbe14dd) 85.04% compared to head (3228fb3) 85.80%.
Report is 1 commits behind head on master.

Files Patch % Lines
flytekit/core/container_task.py 85.71% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1944      +/-   ##
==========================================
+ Coverage   85.04%   85.80%   +0.75%     
==========================================
  Files         280      313      +33     
  Lines       21751    23285    +1534     
  Branches     3526     3528       +2     
==========================================
+ Hits        18498    19979    +1481     
- Misses       2650     2702      +52     
- Partials      603      604       +1     

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

@zeryx
Copy link
Collaborator

zeryx commented Nov 8, 2023

cc @zeryx

Super exciting, ImageSpec is supported independently from envd, right? flytekitplugins-envd is required only for the actual registration step?

@pingsutw
Copy link
Member Author

pingsutw commented Nov 8, 2023

yes, exactly. we should also be able to use docker plugin to build the image.

@danpf
Copy link
Contributor

danpf commented Nov 8, 2023

@pingsutw I do not see that behavior in my test cases, am I instantiating ImageSpec wrong?

commandline:

pyflyte  -c ~/.flyte/config-sandbox.yaml  run --remote runme.py my_wf

Script:

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

running_imagespec = ImageSpec(packages=["pandas", "numpy"], apt_packages=["git"], registry="localhost:30000")

running_dyn = ImageSpec(packages=["requests"], registry="localhost:30000")


@task(container_image=running_imagespec)
def t1() -> str:
    import pandas
    return "hello"


@dynamic(container_image=running_dyn)
def my_wf() -> str:
    import requests
    return t1()

Error:

[3/3] currentAttempt done. Last Error: SYSTEM::Traceback (most recent call last):

      File "/usr/local/lib/python3.11/site-packages/flytekit/exceptions/scopes.py", line 165, in system_entry_point
        return wrapped(*args, **kwargs)
               ^^^^^^^^^^^^^^^^^^^^^^^^
      File "/usr/local/lib/python3.11/site-packages/flytekit/core/base_task.py", line 624, in dispatch_execute
        raise e
      File "/usr/local/lib/python3.11/site-packages/flytekit/core/base_task.py", line 621, in dispatch_execute
        native_outputs = self.execute(**native_inputs)
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/usr/local/lib/python3.11/site-packages/flytekit/core/python_function_task.py", line 180, in execute
        return self.dynamic_execute(self._task_function, **kwargs)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/usr/local/lib/python3.11/site-packages/flytekit/core/python_function_task.py", line 320, in dynamic_execute
        return self.compile_into_workflow(ctx, task_function, **kwargs)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/usr/local/lib/python3.11/site-packages/flytekit/core/python_function_task.py", line 222, in compile_into_workflow
        workflow_spec: admin_workflow_models.WorkflowSpec = get_serializable(
                                                            ^^^^^^^^^^^^^^^^^
      File "/usr/local/lib/python3.11/site-packages/flytekit/tools/translator.py", line 699, in get_serializable
        cp_entity = get_serializable_workflow(entity_mapping, settings, entity, options)
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/usr/local/lib/python3.11/site-packages/flytekit/tools/translator.py", line 239, in get_serializable_workflow
        serialized_nodes.append(get_serializable(entity_mapping, settings, n, options))
                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/usr/local/lib/python3.11/site-packages/flytekit/tools/translator.py", line 702, in get_serializable
        cp_entity = get_serializable_node(entity_mapping, settings, entity, options)
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/usr/local/lib/python3.11/site-packages/flytekit/tools/translator.py", line 435, in get_serializable_node
        task_spec = get_serializable(entity_mapping, settings, entity.flyte_entity, options=options)
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/usr/local/lib/python3.11/site-packages/flytekit/tools/translator.py", line 696, in get_serializable
        cp_entity = get_serializable_task(settings, entity)
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/usr/local/lib/python3.11/site-packages/flytekit/tools/translator.py", line 180, in get_serializable_task
        container = entity.get_container(settings)
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/usr/local/lib/python3.11/site-packages/flytekit/core/python_auto_container.py", line 177, in get_container
        return self._get_container(settings)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/usr/local/lib/python3.11/site-packages/flytekit/core/python_auto_container.py", line 188, in _get_container
        image=get_registerable_container_image(self.container_image, settings.image_config),
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/usr/local/lib/python3.11/site-packages/flytekit/core/python_auto_container.py", line 257, in get_registerable_container_image
        ImageBuildEngine.build(img)
      File "/usr/local/lib/python3.11/site-packages/flytekit/image_spec/image_spec.py", line 151, in build
        raise Exception(f"Builder {image_spec.builder} is not registered.")

Message:

    Builder envd is not registered.

SYSTEM ERROR! Contact platform administrators.

@pingsutw
Copy link
Member Author

pingsutw commented Nov 8, 2023

@danpf you could change the default builder to docker you added.

running_imagespec = ImageSpec(packages=["pandas", "numpy"], builder="docker")

@danpf
Copy link
Contributor

danpf commented Nov 8, 2023

I think it won't, because the docker builder won't be registered either.

Actually though, you are technically right! A Task does not need the builder registered, but dynamic does. running the task directly succeeds.

ByronHsu
ByronHsu previously approved these changes Nov 12, 2023
@@ -112,8 +114,15 @@ def _get_data_loading_config(self) -> _task_model.DataLoadingConfig:
def _get_container(self, settings: SerializationSettings) -> _task_model.Container:
env = settings.env or {}
env = {**env, **self.environment} if self.environment else env
if isinstance(self._image, ImageSpec):
if settings.fast_serialization_settings is None or not settings.fast_serialization_settings.enabled:
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we use get_registerable_container_image to resolve the image name so it can also handle the case like {{.image.default.fqn}}:{{.image.default.version}})

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
@pingsutw pingsutw merged commit e5c3c78 into master Nov 22, 2023
74 checks passed
ringohoffman pushed a commit to ringohoffman/flytekit that referenced this pull request Nov 24, 2023
---------

Signed-off-by: Kevin Su <pingsutw@apache.org>
RRap0so pushed a commit to RRap0so/flytekit that referenced this pull request Dec 15, 2023
---------

Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Rafael Raposo <rafaelraposo@spotify.com>
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.

5 participants