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

Pod count evaluation #53

Merged
merged 3 commits into from
May 24, 2024
Merged

Conversation

shinae-woo
Copy link
Collaborator

@shinae-woo shinae-woo commented May 22, 2024

Support arithmetic evaluation for PodCount in RegisterObj

e.g.,

  • "{{.parallelism}} * 2"
  • "{{.parallelism}} * {{.replicas}}"

@shinae-woo shinae-woo requested a review from dmitsh May 22, 2024 17:22
- check if tc.task if not nil before access to the object

Signed-off-by: Shinae Woo <[email protected]>
e.g., `podCount: "{{.parallelism}} * 2"`

Signed-off-by: Shinae Woo <[email protected]>
@shinae-woo shinae-woo force-pushed the pod-count-evaluation branch from e3db2bb to 2edd30f Compare May 22, 2024 17:24
@dmitsh dmitsh requested a review from lalitadithya May 23, 2024 11:52
@dmitsh
Copy link
Collaborator

dmitsh commented May 23, 2024

I was wondering if the use of FuncMap would be more intuitive for users.
For example, we define math functions:

func add(a, b int) int {return a + b}
func subtract(a, b int) int {return a - b}
func multiply(a, b int) int {return a * b}
func divide(a, b int) int {
   if b == 0 {
        return 0
    }
    return a / b
}

Then we use them in templates:

funcMap := template.FuncMap{
    "add":      add,
    "sub": subtract,
    "mul": multiply,
    "div":   divide,
}

template.New("submit").Funcs(funcMap).Parse(tmpl)

And the template will look like

{{ mul .A .B }}

@lalitadithya
Copy link
Collaborator

I was wondering if the use of FuncMap would be more intuitive for users.
For example, we define math functions:

func add(a, b int) int {return a + b}
func subtract(a, b int) int {return a - b}
func multiply(a, b int) int {return a * b}
func divide(a, b int) int {
   if b == 0 {
        return 0
    }
    return a / b
}

Then we use them in templates:

funcMap := template.FuncMap{
    "add":      add,
    "sub": subtract,
    "mul": multiply,
    "div":   divide,
}

template.New("submit").Funcs(funcMap).Parse(tmpl)

And the template will look like

{{ mul .A .B }}

We don't have to implement this, there already exists a library that provides what you have suggested -- https://github.com/Masterminds/sprig. It's the templating library that's used by helm and I believe some of our internal tools as well. Another advantage of using this library is that users get much more than the basic mathematical operations and not to mention the library is battle tested.

With that said, both approaches (the one in implemented in this PR and the one suggest in this comment) achieve the same end result for this use case but take different approaches.

My question is do we have use cases where users need to use more than just mathematical expressions? Because, if we don't then IMO we can stick to simple mathematical expression evaluation as used in the current approach.

@shinae-woo
Copy link
Collaborator Author

shinae-woo commented May 23, 2024

Multiple templates, such as Helm or Hugo, implement mathematical evaluations. The reason that I picked the current implementation was that

  • The only requirement, so far, is supporting PodCount and simple mathematical evaluation
  • Users don't need to learn a new syntax
  • Minimal code footprint

If we have other requirements beyond the current implementation, we can discuss them further, so please let me know if we have any. However, in such case, I would pick one existing implementation rather than introduce and implement our own template.

dmitsh
dmitsh previously approved these changes May 24, 2024
@dmitsh dmitsh dismissed their stale review May 24, 2024 04:55

review

@shinae-woo shinae-woo merged commit f64ecb3 into NVIDIA:main May 24, 2024
4 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
Development

Successfully merging this pull request may close these issues.

3 participants