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

refactor(core): Improve task module extraction logic #2290

Merged
merged 7 commits into from
Apr 3, 2024
Merged

Conversation

pingsutw
Copy link
Member

@pingsutw pingsutw commented Mar 20, 2024

Tracking issue

NA

Why are the changes needed?

Cannot run a task registered by flytekit remote

What changes were proposed in this pull request?

  • Update task module extraction logic
  • Update remote.register task interface. make SerializationSettings optional

Before:

remote = FlyteRemote(...)
remote.register_task(
        entity=t1,
        serialization_settings=SerializationSettings(
            image_config=ImageConfig.auto_default_image(),
            source_root=Path(".").absolute().__str__(),
        ),
        version="v1",
    )

After:

remote = FlyteRemote(...)
remote.register_task(
        entity=t1,
        version="v1",
    )

How was this patch tested?

remote cluster

python functional_tests.py 

Setup process

from datetime import datetime

from flytekit import task, ImageSpec
from flytekit.configuration import Config, SerializationSettings, ImageConfig
from flytekit.remote import FlyteRemote
from flytekit.tools.script_mode import hash_file

image_spec = ImageSpec(
    name="flyte-conformance",
    registry="ghcr.io/unionai",
    packages=["pandas"],
)


@task(container_image=image_spec)
def t1(x: int) -> datetime:
    return datetime.now()


def test_cache_override():
    remote = FlyteRemote(
        config=Config.auto("/Users/kevin/.flyte/config-dogfood-gcp.yaml"),
        default_project="flytesnacks",
        default_domain="development",
    )
    _, digest, _ = hash_file(__file__)
    flyte_task = remote.register_task(
        entity=t1,
        version=digest,
    )

    exe = remote.execute(entity=flyte_task, inputs={"x": 3}, wait=False)

Screenshots

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

Related PRs

NA

Docs link

NA

@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Mar 20, 2024
Copy link

codecov bot commented Mar 20, 2024

Codecov Report

Attention: Patch coverage is 41.66667% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 83.09%. Comparing base (6a383cd) to head (6d639c5).
Report is 1 commits behind head on master.

Files Patch % Lines
flytekit/remote/remote.py 33.33% 4 Missing ⚠️
flytekit/core/tracker.py 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2290      +/-   ##
==========================================
- Coverage   83.12%   83.09%   -0.03%     
==========================================
  Files         324      324              
  Lines       24779    24789      +10     
  Branches     3523     3524       +1     
==========================================
+ Hits        20597    20599       +2     
- Misses       3558     3565       +7     
- Partials      624      625       +1     

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

Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Future-Outlier
Future-Outlier previously approved these changes Mar 21, 2024
Copy link
Member

@Future-Outlier Future-Outlier left a comment

Choose a reason for hiding this comment

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

test again

@dosubot dosubot bot added the lgtm This PR has been approved by maintainer label Mar 21, 2024
wild-endeavor
wild-endeavor previously approved these changes Mar 26, 2024
@@ -788,6 +792,14 @@ def register_task(
:param version: version that will be used to register. If not specified will default to using the serialization settings default
:return:
"""
if serialization_settings is None:
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a comment here explaining when this situation is relevant? and maybe also a comment in tracker.py explaining when the second element in the return tuple (the thing that used to be "") is relevant?

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 <[email protected]>
@pingsutw pingsutw dismissed stale reviews from wild-endeavor and Future-Outlier via 8512bc6 April 3, 2024 20:56
@pingsutw pingsutw requested a review from samhita-alla as a code owner April 3, 2024 20:56
Signed-off-by: Kevin Su <[email protected]>
@pingsutw pingsutw merged commit df5dbea into master Apr 3, 2024
47 of 49 checks passed
ChungYujoyce pushed a commit to ChungYujoyce/flytekit that referenced this pull request Apr 5, 2024
fiedlerNr9 pushed a commit that referenced this pull request Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm This PR has been approved by maintainer size:S This PR changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants