ImageConfig should contain the image from sandbox.config #1106

pingsutw · 2022-07-20T10:26:32Z

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

TL;DR

Failed to run this example because ImageConfig doesn't contain the image from sandbox.config

(flytekit-3.9) ➜  core git:(master) ✗ pyflyte --config sandbox.config run --remote containerization/multi_images.py my_workflow
Traceback (most recent call last):
  File "/Users/kevin/opt/anaconda3/envs/flytekit-3.9/bin/pyflyte", line 33, in <module>
    sys.exit(load_entry_point('flytekit', 'console_scripts', 'pyflyte')())
  File "/Users/kevin/opt/anaconda3/envs/flytekit-3.9/lib/python3.9/site-packages/click/core.py", line 1128, in __call__
    return self.main(*args, **kwargs)
  File "/Users/kevin/opt/anaconda3/envs/flytekit-3.9/lib/python3.9/site-packages/click/core.py", line 1053, in main
    rv = self.invoke(ctx)
  File "/Users/kevin/opt/anaconda3/envs/flytekit-3.9/lib/python3.9/site-packages/click/core.py", line 1659, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/Users/kevin/opt/anaconda3/envs/flytekit-3.9/lib/python3.9/site-packages/click/core.py", line 1659, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/Users/kevin/opt/anaconda3/envs/flytekit-3.9/lib/python3.9/site-packages/click/core.py", line 1659, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/Users/kevin/opt/anaconda3/envs/flytekit-3.9/lib/python3.9/site-packages/click/core.py", line 1395, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/Users/kevin/opt/anaconda3/envs/flytekit-3.9/lib/python3.9/site-packages/click/core.py", line 754, in invoke
    return __callback(*args, **kwargs)
  File "/Users/kevin/git/flytekit/flytekit/clis/sdk_in_container/run.py", line 519, in _run
    remote_entity = remote.register_script(
  File "/Users/kevin/git/flytekit/flytekit/remote/remote.py", line 625, in register_script
    return self.register_workflow(entity, serialization_settings, version, default_launch_plan, options)
  File "/Users/kevin/git/flytekit/flytekit/remote/remote.py", line 499, in register_workflow
    ident = self._serialize_and_register(entity, serialization_settings, version, options)
  File "/Users/kevin/git/flytekit/flytekit/remote/remote.py", line 391, in _serialize_and_register
    _ = get_serializable(m, settings=serialization_settings, entity=entity, options=options)
  File "/Users/kevin/git/flytekit/flytekit/tools/translator.py", line 581, in get_serializable
    cp_entity = get_serializable_workflow(entity_mapping, settings, entity, options)
  File "/Users/kevin/git/flytekit/flytekit/tools/translator.py", line 226, in get_serializable_workflow
    upstream_node_models = [
  File "/Users/kevin/git/flytekit/flytekit/tools/translator.py", line 227, in <listcomp>
    get_serializable(entity_mapping, settings, n, options)
  File "/Users/kevin/git/flytekit/flytekit/tools/translator.py", line 584, in get_serializable
    cp_entity = get_serializable_node(entity_mapping, settings, entity, options)
  File "/Users/kevin/git/flytekit/flytekit/tools/translator.py", line 412, in get_serializable_node
    task_spec = get_serializable(entity_mapping, settings, entity.flyte_entity, options=options)
  File "/Users/kevin/git/flytekit/flytekit/tools/translator.py", line 578, in get_serializable
    cp_entity = get_serializable_task(entity_mapping, settings, entity)
  File "/Users/kevin/git/flytekit/flytekit/tools/translator.py", line 173, in get_serializable_task
    container = entity.get_container(settings)
  File "/Users/kevin/git/flytekit/flytekit/core/python_auto_container.py", line 164, in get_container
    image=get_registerable_container_image(self.container_image, settings.image_config),
  File "/Users/kevin/git/flytekit/flytekit/core/python_auto_container.py", line 235, in get_registerable_container_image
    raise AssertionError(f"Image Config with name {name} not found in the configuration")
AssertionError: Image Config with name trainer not found in the configuration

Type

Bug Fix
Feature
Plugin

Are all requirements met?

Complete description

How did you fix the bug, make the feature etc. Link to any design docs etc

Tracking Issue

https://github.com/flyteorg/flyte/issues/

Follow-up issue

NA
OR
https://github.com/flyteorg/flyte/issues/

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

codecov · 2022-07-20T10:33:19Z

Codecov Report

Merging #1106 (6e9b3a3) into master (72705a1) will decrease coverage by 18.54%.
The diff coverage is 63.63%.

@@             Coverage Diff             @@
##           master    #1106       +/-   ##
===========================================
- Coverage   86.67%   68.13%   -18.55%     
===========================================
  Files         269      287       +18     
  Lines       25074    25762      +688     
  Branches     2826     2882       +56     
===========================================
- Hits        21734    17553     -4181     
- Misses       2871     7733     +4862     
- Partials      469      476        +7

Impacted Files	Coverage Δ
flytekit/clis/sdk_in_container/run.py	`76.84% <14.28%> (-3.09%)`	⬇️
flytekit/configuration/__init__.py	`36.05% <66.66%> (-58.92%)`	⬇️
flytekit/configuration/internal.py	`16.00% <76.92%> (-79.84%)`	⬇️
...s/flytekit/unit/configuration/test_image_config.py	`100.00% <100.00%> (ø)`
tests/flytekit/unit/configuration/test_internal.py	`100.00% <100.00%> (ø)`
...ests/flytekit/unit/configuration/test_yaml_file.py	`100.00% <100.00%> (ø)`
flytekit/deck/__init__.py	`0.00% <0.00%> (-100.00%)`	⬇️
flytekit/core/constants.py	`0.00% <0.00%> (-100.00%)`	⬇️
flytekit/core/resources.py	`0.00% <0.00%> (-100.00%)`	⬇️
flytekit/core/tracked_abc.py	`0.00% <0.00%> (-100.00%)`	⬇️
... and 130 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72705a1...6e9b3a3. Read the comment docs.

eapolinario

This is not quite right yet. IIUC, this PR makes it so that the --config flag (which applies at the top level) becomes a required parameter.

Also, the images config section has no equivalent in the yaml config, right?

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

pingsutw · 2022-07-26T15:55:54Z

sorry, updated it.

Now --config flag should be optional.
Support reading image config in yaml config

eapolinario

LGTM.

eapolinario · 2022-07-27T13:03:12Z

flytekit/clis/sdk_in_container/run.py

@@ -512,12 +512,19 @@ def _run(*args, **kwargs):
            return

        remote = ctx.obj[FLYTE_REMOTE_INSTANCE_KEY]
+        config_file = ctx.obj.get(CTX_CONFIG_FILE)


just for own understanding, what's the level that config is applied? As in, can you pass the --config flag to the run subcommand (e.g. pyflyte run --config <file.yaml> --remote ...)? Or it has to come before the run subcommand?

We still have to use --config before the run. like pyflyte --config flyte.yaml run ....

pingsutw · 2022-07-27T13:44:57Z

Convert it to draft, need further discussion with Yee

wild-endeavor · 2022-07-28T21:35:43Z

tests/flytekit/unit/configuration/configs/sample.yaml

@@ -11,3 +11,6 @@ storage:
    access-key: minio
    endpoint: http://localhost:30084
    secret-key: miniostorage
+images:


I don't think this is supported at all on the flytectl side (and might never be since it's mostly responsible for registration time settings) but that's ok. We can still support this.

wild-endeavor · 2022-07-28T23:12:13Z

flytekit/clis/sdk_in_container/run.py

@@ -512,12 +512,19 @@ def _run(*args, **kwargs):
            return

        remote = ctx.obj[FLYTE_REMOTE_INSTANCE_KEY]
+        config_file = ctx.obj.get(CTX_CONFIG_FILE)
+
+        image_config = run_level_params.get("image_config", None)


so image_config is never going to be None because of the default and because of the validate_image callback. So the ImageConfig.auto will never be called.

wild-endeavor · 2022-07-28T23:26:08Z

flytekit/clis/sdk_in_container/run.py

+        if image_config:
+            image_config.images.extend(ImageConfig.auto(config_file).images)
+        else:
+            image_config = ImageConfig.auto(config_file)


if this is called actually, this will fail if config_file is None because ImageConfig.auto(config_file=None) will return a

for k, v in _internal.Images.get_specified_images(config_file).items() E AttributeError: 'NoneType' object has no attribute 'items'

error

flytekit/configuration/__init__.py

wild-endeavor · 2022-07-29T01:37:10Z

flytekit/configuration/internal.py

+                    images[str(i)] = cfg.legacy_config.get("images", i)
+            return images
+        if cfg.yaml_config:
+            return cfg.yaml_config.get("images")


this needs to return {} in case it doesn't exist.

flytekit/configuration/__init__.py

wild-endeavor · 2022-07-29T02:03:56Z

flytekit/clis/sdk_in_container/run.py

+
+        image_config = run_level_params.get("image_config", None)
+        if image_config:
+            image_config.images.extend(ImageConfig.auto(config_file).images)


also how does this work? weird. isn't ImageConfig supposed to be frozen??

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

* ImageConfig should contain the image from sandbox.config Signed-off-by: Kevin Su <[email protected]> * Updated Signed-off-by: Kevin Su <[email protected]> * lint Signed-off-by: Kevin Su <[email protected]> * pr into #1106 (#1113) Signed-off-by: Yee Hing Tong <[email protected]> Co-authored-by: Yee Hing Tong <[email protected]>

ImageConfig should contain the image from sandbox.config

24684da

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

pingsutw requested review from wild-endeavor, kumare3 and eapolinario as code owners July 20, 2022 10:26

eapolinario reviewed Jul 25, 2022

View reviewed changes

pingsutw added 2 commits July 26, 2022 22:48

Updated

b20403e

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

lint

0f49dc1

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

eapolinario previously approved these changes Jul 27, 2022

View reviewed changes

pingsutw marked this pull request as draft July 27, 2022 13:42

wild-endeavor reviewed Jul 28, 2022

View reviewed changes

wild-endeavor reviewed Jul 29, 2022

View reviewed changes

pr into #1106 (#1113)

6e9b3a3

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

wild-endeavor dismissed eapolinario’s stale review via 6e9b3a3 July 29, 2022 17:08

pingsutw marked this pull request as ready for review July 29, 2022 17:20

wild-endeavor approved these changes Jul 29, 2022

View reviewed changes

pingsutw merged commit 9d31342 into master Jul 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ImageConfig should contain the image from sandbox.config #1106

ImageConfig should contain the image from sandbox.config #1106

pingsutw commented Jul 20, 2022

codecov bot commented Jul 20, 2022 •

edited

Loading

eapolinario left a comment

pingsutw commented Jul 26, 2022

eapolinario left a comment

eapolinario Jul 27, 2022 •

edited

Loading

pingsutw Jul 27, 2022

pingsutw commented Jul 27, 2022

wild-endeavor Jul 28, 2022

wild-endeavor Jul 28, 2022

wild-endeavor Jul 28, 2022

wild-endeavor Jul 29, 2022

wild-endeavor Jul 29, 2022

ImageConfig should contain the image from sandbox.config #1106

ImageConfig should contain the image from sandbox.config #1106

Conversation

pingsutw commented Jul 20, 2022

TL;DR

Type

Are all requirements met?

Complete description

Tracking Issue

Follow-up issue

codecov bot commented Jul 20, 2022 • edited Loading

Codecov Report

eapolinario left a comment

Choose a reason for hiding this comment

pingsutw commented Jul 26, 2022

eapolinario left a comment

Choose a reason for hiding this comment

eapolinario Jul 27, 2022 • edited Loading

Choose a reason for hiding this comment

pingsutw Jul 27, 2022

Choose a reason for hiding this comment

pingsutw commented Jul 27, 2022

wild-endeavor Jul 28, 2022

Choose a reason for hiding this comment

wild-endeavor Jul 28, 2022

Choose a reason for hiding this comment

wild-endeavor Jul 28, 2022

Choose a reason for hiding this comment

wild-endeavor Jul 29, 2022

Choose a reason for hiding this comment

wild-endeavor Jul 29, 2022

Choose a reason for hiding this comment

codecov bot commented Jul 20, 2022 •

edited

Loading

eapolinario Jul 27, 2022 •

edited

Loading