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

Remove storage as a task resource option #2078

Conversation

Future-Outlier
Copy link
Member

@Future-Outlier Future-Outlier commented Jan 2, 2024

Tracking issue

Fixes flyteorg/flyte#3955

Why are the changes needed?

Make the storage unused in the task resource option, so that users will not be confused about why they can define the storage but it didn't work.

What changes were proposed in this pull request?

Remove all variables with task resource storage.
Search variable like this.
image

How was this patch tested?

  1. run flytekit task with specified resource
  2. use kubectl describe nodeId to check if the Resources as expected

Success Scenario

from flytekit import task, Resources

@task(
        requests=Resources(cpu="2", ephemeral_storage="200Mi", mem="200Mi"),
)
def test_storage() -> None:
    import time
    print("@@@ I am sleeping for 10 seconds")
    time.sleep(10)
    return
pyflyte run --remote remove_resource_storage.py test_storage
kubectl describe pod ff436fac5f4d74583b75-f4wb3a3y-0 -n flytesnacks-development

The output is below.

    Limits:
      cpu:                2
      ephemeral-storage:  200Mi
      memory:             200Mi
    Requests:
      cpu:                2
      ephemeral-storage:  200Mi
      memory:             200Mi

image

Failure Scenario

from flytekit import task, Resources

@task(
        requests=Resources(cpu="2", storage="200Mi"),
)
def test_storage() -> None:
    import time
    print("@@@ I am sleeping for 10 seconds")
    time.sleep(10)
    return
pyflyte run --remote remove_resource_storage.py test_storage

image

Check all the applicable boxes

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

Related PRs

flyteorg/flyte#4658

Signed-off-by: Future Outlier <eric901201@gmai.com>
Copy link

codecov bot commented Jan 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e107994) 85.71% compared to head (4591da1) 85.73%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2078      +/-   ##
==========================================
+ Coverage   85.71%   85.73%   +0.01%     
==========================================
  Files         313      313              
  Lines       23396    23383      -13     
  Branches     3501     3497       -4     
==========================================
- Hits        20055    20048       -7     
+ Misses       2733     2729       -4     
+ Partials      608      606       -2     

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

@Future-Outlier Future-Outlier changed the title [WIP] Remove storage as a task resource option Remove storage as a task resource option Jan 2, 2024
@Future-Outlier Future-Outlier marked this pull request as ready for review January 2, 2024 06:17
@Future-Outlier
Copy link
Member Author

@katrogan Please take a look, thanks a lot!

@pingsutw pingsutw merged commit a3a640a into flyteorg:master Jan 13, 2024
84 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants