Skip to content

Skip building the image if failed to check if image exists#1609

Closed
pingsutw wants to merge 18 commits into
masterfrom
imageSpec-bug
Closed

Skip building the image if failed to check if image exists#1609
pingsutw wants to merge 18 commits into
masterfrom
imageSpec-bug

Conversation

@pingsutw

@pingsutw pingsutw commented Apr 26, 2023

Copy link
Copy Markdown
Member

TL;DR

Skip building the image if failed to check if image exists, and we assume image already exists.

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

^^^

Tracking Issue

NA

Follow-up issue

NA

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

codecov Bot commented Apr 26, 2023

Copy link
Copy Markdown

Codecov Report

Merging #1609 (8bc6c23) into master (e5b0511) will decrease coverage by 0.06%.
The diff coverage is 29.62%.

@@            Coverage Diff             @@
##           master    #1609      +/-   ##
==========================================
- Coverage   70.16%   70.10%   -0.06%     
==========================================
  Files         328      328              
  Lines       30142    30156      +14     
  Branches     5448     5452       +4     
==========================================
- Hits        21149    21141       -8     
- Misses       8465     8485      +20     
- Partials      528      530       +2     
Impacted Files Coverage Δ
flytekit/configuration/__init__.py 36.99% <0.00%> (-0.12%) ⬇️
...ts/flytekit/unit/core/test_python_function_task.py 92.79% <ø> (-0.13%) ⬇️
flytekit/image_spec/image_spec.py 31.73% <18.18%> (-13.33%) ⬇️
flytekit/core/python_auto_container.py 57.14% <100.00%> (+0.32%) ⬆️
...s/flytekit/unit/core/image_spec/test_image_spec.py 100.00% <100.00%> (ø)

Signed-off-by: Kevin Su <pingsutw@apache.org>
@pingsutw pingsutw changed the title [WIP] Use docker endpoint to check if image exists Skip building the image if failed to check if image exists Apr 26, 2023
pingsutw added 10 commits April 26, 2023 07:47
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Comment thread flytekit/image_spec/image_spec.py
Comment thread flytekit/image_spec/image_spec.py Outdated
except Exception as e:
tag = calculate_hash_from_image_spec(self)
# if docker engine is not running locally
response = requests.get(f"https://hub.docker.com/v2/repositories/{self.registry}/{self.name}/tags/{tag}")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

let's kill this section for now. the url should be from the registry, not hard-coded here, but we have to think about tokens and such...

flytekit_virtualenv_root=self.flytekit_virtualenv_root,
python_interpreter=self.python_interpreter,
fast_serialization_settings=self.fast_serialization_settings,
source_root=self.source_root,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is this change still needed? or just for cleanliness?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yes, we need it for dynamic task

pingsutw added 3 commits May 1, 2023 14:50
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
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.

2 participants