Skip to content

add requests and limits to the job definition#47

Draft
colobas wants to merge 1 commit into
alexellis:masterfrom
colobas:master
Draft

add requests and limits to the job definition#47
colobas wants to merge 1 commit into
alexellis:masterfrom
colobas:master

Conversation

@colobas
Copy link
Copy Markdown

@colobas colobas commented Aug 17, 2023

add requests and limits to the job definition

Description

This PR gives users the option of specifying resource requests and limits in the job YAML file.
It does this by adding two optional fields to the Job struct, which are parsed as ResourceList

Motivation and Context

This lets users make sure their jobs have the appropriate resources allocated to them. E.g., to use this to launch a job that trains or runs inference with a deep learning model and requires a GPU.
Addresses #49

  • I have raised an issue to propose this change
    • ...which has been given a label of design/approved by a maintainer (required)

How Has This Been Tested?

Tested manually by using it with the new fields and seeing that it achieves what I intended.
Also tested with the given examples in the repo to make sure I didn't introduce a regression.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have signed-off my commits with git commit -s

@derek
Copy link
Copy Markdown

derek Bot commented Aug 17, 2023

Thank you for your contribution. unfortunately, one or more of your commits are missing the required "Signed-off-by:" statement. Signing off is part of the Developer Certificate of Origin (DCO) which is used by this project.

Read the DCO and project contributing guide carefully, and amend your commits using the git CLI. Note that this does not require any cryptography, keys or special steps to be taken.

💡 Shall we fix this?

This will only take a few moments.

First, clone your fork and checkout this branch using the git CLI.

Next, set up your real name and email address:

git config --global user.name "Your Full Name"
git config --global user.email "you@domain.com"

Finally, run one of these commands to add the "Signed-off-by" line to your commits.

If you only have one commit so far then run: git commit --amend --signoff and then git push --force.
If you have multiple commits, watch this video.

Check that the message has been added properly by running "git log".

@derek derek Bot added the invalid This doesn't seem right label Aug 17, 2023
@derek
Copy link
Copy Markdown

derek Bot commented Aug 17, 2023

Thank you for your contribution. I've just checked and your Pull Request doesn't appear to have any description.
That's something we need before your Pull Request can be merged. Please see our contributing guide.

@alexellis
Copy link
Copy Markdown
Owner

alexellis commented Aug 17, 2023

Hey @colobas thanks for your interest in my tool.

I will need you to fill out the PR description talking about - why this is needed, who it's for and how you've tested it.

You'll also need to update the docs (README file) - I can see that hasn't been done yet.

Looking forward to talking more.

Alex

@colobas colobas marked this pull request as draft August 18, 2023 21:44
@colobas
Copy link
Copy Markdown
Author

colobas commented Aug 18, 2023

Sorry I haven't had time to fix this PR. I'll get to it sometime next week.

@alexellis
Copy link
Copy Markdown
Owner

Gotcha, I see you pushed changes, but the PR description still needs updating from "No description provided."

Can you adapt this one? https://raw.githubusercontent.com/alexellis/arkade/master/.github/PULL_REQUEST_TEMPLATE.md

Signed-off-by: Guilherme Pires <mail@gpir.es>
@alexellis
Copy link
Copy Markdown
Owner

alexellis commented Sep 6, 2023

 Tested manually by using it with the new fields and seeing that it achieves what I intended.
Also tested with the given examples in the repo to make sure I didn't introduce a regression.

Do you mind showing some console output, and a sample YAML file input that you used? That's just how we do things.

Thanks 👍

Following that, do you want to make the PR as ready for review?

@alexellis
Copy link
Copy Markdown
Owner

@Jasstkn could you do a review and comment with anything that may not have been though about yet? Updating examples etc.

Comment thread main.go
for key, value := range inputMap {
quantity, err := resource.ParseQuantity(value)
if err != nil {
fmt.Printf("Error converting value for key %s: %v\n", key, err)
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.

the error handling doesn't look clean to me. In the main function log.Fatal is being used in case of parsing issues. I would vote to use the same approach here. What are your thoughts?

Comment thread README.md
image: ghcr.io/openfaas/config-checker:latest
namespace: openfaas
sa: openfaas-checker
# optionally specify resource requests/limits
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.

@alexellis what do you think about having a full example with all the currently supported parameters under examples/ folder? In the readme I would just leave a link to the full example instead of copying all the newly added things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

invalid This doesn't seem right new-contributor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants