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

[Docs] Examples for customizing resources are not correct #4035

Closed
2 tasks done
Tracked by #4064
honnix opened this issue Sep 15, 2023 · 10 comments · Fixed by flyteorg/flytekit#1871
Closed
2 tasks done
Tracked by #4064

[Docs] Examples for customizing resources are not correct #4035

honnix opened this issue Sep 15, 2023 · 10 comments · Fixed by flyteorg/flytekit#1871
Assignees
Labels
documentation Improvements or additions to documentation hacktoberfest

Comments

@honnix
Copy link
Member

honnix commented Sep 15, 2023

Description

In old version of flytekit, customizing cpu or memory was done as with_overrides(cpu="10M"), but this is no longer the case and it needs to be with_overrides(requests=Resources(...), limits=Resources(...)).

Some part of the document still refers to the old way, for example https://docs.flyte.org/projects/flytekit/en/latest/generated/flytekit.map_task.html#flytekit-map-task. The same issue exists in flytekit test cases, for example https://github.com/flyteorg/flytekit/blob/27b288bae7e4fc2dee6c47c455e4e4289af0d035/tests/flytekit/unit/core/test_dynamic.py#L176

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

  • Yes

Have you read the Code of Conduct?

  • Yes
@honnix honnix added documentation Improvements or additions to documentation untriaged This issues has not yet been looked at by the Maintainers labels Sep 15, 2023
@honnix honnix changed the title [Docs] Examples customizing resources are not correct [Docs] Examples for customizing resources are not correct Sep 15, 2023
@eapolinario eapolinario removed the untriaged This issues has not yet been looked at by the Maintainers label Sep 15, 2023
@Illuminati9
Copy link

Hey @honnix , I want to work on this issue under hacktoberfest'23. Can you please explain it in clearly??

@Kota-Karthik
Copy link

@samhita-alla @honnix
I am interested in doing this issue
So kindly assign me this issue
Thank You : )

@samhita-alla
Copy link
Contributor

Need to update the documentation, @Illuminati9. Please go ahead and create a PR, @Illuminati9, @Kota-Karthik. We will consider whoever creates a PR first on a first-come, first-serve basis.

@Kota-Karthik
Copy link

Kota-Karthik commented Oct 2, 2023

@samhita-alla the docs has to be changed in flyte repo and not in flytekit repo right?
but i am unable to find the docs in Flyte
am i supposed to do changes in FlyteSnacks ?
cause You have mentioned this issue under the section of FlyteSnacks in #4064
could you help me with this?

@Kota-Karthik
Copy link

Hello @samhita-alla,

I have reviewed the FlyteSnacks Repository and was unable to find any instances of the with_override function being used in an outdated manner. This function was not used in the documentation of FlyteSnacks.

I found the with_override function being used for memory allocation in only two files:

  1. flytesnacks\examples\advanced_composition\advanced_composition\map_task.py on line 68, where it was used correctly.
  2. flytesnacks\examples\productionizing\productionizing\customizing_resources.py on line 124.

Based on my findings, I don't believe there is an issue with the documentation in the FlyteSnacks Repository.
Could you please specify where I should look for this issue?

Thank you : )

@itssiddhantjain
Copy link

Hello @honnix , please assign this issue to me as I have already worked on this kind of problem in past and has a great experience.

@honnix
Copy link
Member Author

honnix commented Oct 2, 2023

@itssiddhantjain I'm not part of the group organizing hacktoberfest so I'm not sure of the process. Happy to assign if @samhita-alla is fine with that?

@itssiddhantjain
Copy link

Hey @samhita-alla, please assign this issue to me as I have already worked on this kind of problem in past and has a great experience.

@samhita-alla
Copy link
Contributor

@Kota-Karthik, the changes need to be made in the flytekit repository. Sorry for the confusion.

@LunarMarathon
Copy link

Hi, I have raised a PR. I went through all the instances of with_overrides() but only one instance was using the older way of doing it (only this one which was already mentioned by honnix: https://docs.flyte.org/projects/flytekit/en/latest/generated/flytekit.map_task.html#flytekit-map-task)

@honnix you've mentioned this ex too - https://github.com/flyteorg/flytekit/blob/27b288bae7e4fc2dee6c47c455e4e4289af0d035/tests/flytekit/unit/core/test_dynamic.py#L176
to1 = task1(s="hello").with_overrides(requests=Resources(cpu="3", mem="5Gi"))
but it's already using this format: with_overrides(requests=Resources(...), limits=Resources(...)) ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation hacktoberfest
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants