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

[Housekeeping] Remove storage from flytekit and helm values #3728

Closed
2 tasks done
fg91 opened this issue May 25, 2023 · 4 comments · Fixed by #4782
Closed
2 tasks done

[Housekeeping] Remove storage from flytekit and helm values #3728

fg91 opened this issue May 25, 2023 · 4 comments · Fixed by #4782
Assignees
Labels
helm housekeeping Issues that help maintain flyte and keep it tech-debt free

Comments

@fg91
Copy link
Member

fg91 commented May 25, 2023

Describe the issue

  • In flytekit.Resources one can request storage which, however, doesn't have any effect.

    This is also acknowledged in the docstring:

     .. note::
    
      Persistent storage is not currently supported on the Flyte backend.
    
  • The task default resource and limits also allow setting storage which doesn't have an effect either. Ephemeral storage defaults which would work cannot be configured on the platform side.

The storage arg should be deprecated from flytekit.Resources and from the helm values. Ephemeral storage defaults and limits should be configurable in the helm values.

See discussion in slack.

What if we do not do this?

Users might try requesting storage and would then wonder why there is no effect.

Related component(s)

flytekit, helm chart

Are you sure this issue hasn't been raised already?

  • Yes

Have you read the Code of Conduct?

  • Yes
@fg91 fg91 added the housekeeping Issues that help maintain flyte and keep it tech-debt free label May 25, 2023
@davidmirror-ops davidmirror-ops self-assigned this Dec 15, 2023
@fg91
Copy link
Member Author

fg91 commented Jan 25, 2024

@Future-Outlier I saw that you tackled part of this in flyteorg/flytekit#2078, thanks a lot! 🙏
Would you be willing to also take a look at the helm chart so that we can close this issue? No problem if not!

Copy link

dosubot bot commented Jan 25, 2024

🤖

I'm glad to see you're interested in contributing to the repository! Your proposed changes to update the helm chart sound like they would be very valuable. I encourage you to go ahead and create a pull request with your proposed changes. Your contribution would be greatly appreciated by the community. Thank you for your willingness to help improve the project!


This response is meant to be useful and save you time. It is not meant to be a precise solution, but rather a starting point for your own research.

Help me be more useful! Please leave a 👍 if this is helpful and 👎 if it is irrelevant.

If you want to continue the conversation, start your reply with @dosu-bot.

@Future-Outlier
Copy link
Member

@Future-Outlier I saw that you tackled part of this in flyteorg/flytekit#2078, thanks a lot! 🙏

Would you be willing to also take a look at the helm chart so that we can close this issue? No problem if not!

I can try to take a look, thank you for giving me this chance, will let you know if I face problems!

@Future-Outlier Future-Outlier self-assigned this Jan 26, 2024
@Future-Outlier
Copy link
Member

I think it is right to delete the storage arg in the helm chart.
We can see it here to prove that the place you mention is correct.
https://pkg.go.dev/github.com/lyft/[email protected]/pkg/runtime/interfaces#TaskResourceConfiguration
Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
helm housekeeping Issues that help maintain flyte and keep it tech-debt free
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants