Skip to content

2026 04 03 fix oom#83

Open
aiturbidemil wants to merge 4 commits intomainfrom
2026-04-03-fix-oom
Open

2026 04 03 fix oom#83
aiturbidemil wants to merge 4 commits intomainfrom
2026-04-03-fix-oom

Conversation

@aiturbidemil
Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates numerous project dependencies and introduces the PL_DOCKER_REGISTRY_PUSH_TO environment variable to the build and test configurations. In the workflow template, memory allocation for UMAP processing was made dynamic based on input arguments. However, the review identified a critical runtime error in the Tengo script where math.max was used instead of the required capitalized math.Max. Additionally, it is recommended to store the calculated memory string in a variable to improve maintainability and prevent redundant calculations across the script.

Comment on lines +59 to +62
baseMemGiB := 64
if !is_undefined(args.mem) {
baseMemGiB = args.mem
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The Tengo math module uses capitalized function names (e.g., math.Max). Using math.max will result in a runtime error. Additionally, since this memory calculation is used in multiple places, it is better to calculate it once and store it in a variable to improve maintainability and ensure consistency.

	baseMemGiB := 64
	if !is_undefined(args.mem) {
		baseMemGiB = args.mem
	}
	umapMem := string(int(math.Max(16, baseMemGiB / 4))) + "GiB"

}

umapTable.mem("16GiB")
umapTable.mem(string(int(math.max(16, baseMemGiB / 4))) + "GiB")
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Use the umapMem variable calculated above to avoid the math.max typo and redundant calculations.

	umapTable.mem(umapMem)

splitDataAndSpec: true,
cpu: 1,
mem: "16GiB" // TODO: make this dynamic on input size
mem: string(int(math.max(16, baseMemGiB / 4))) + "GiB"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Use the umapMem variable calculated above to avoid the math.max typo and redundant calculations.

			mem: umapMem

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.

1 participant