ronantakizawa/codereview-bench huggingface dataset:
Repository: helm/helm
File: pkg/action/validate.go (Go)

Code before:
	return accessor.SetLabels(obj, mergeStrStrMaps(current, labels))
}

func mergeAnnotations(obj runtime.Object, annotations map[string]string) error {
	current, err := accessor.Annotations(obj)
	if err != nil {
		return err
	}
	return accessor.SetAnnotations(obj, mergeStrStrMaps(current, annotations))
}

// merge two maps, always taking the value on the right
func mergeStrStrMaps(current, desired map[string]string) map[string]string {
	result := make(map[string]string)
	maps.Copy(result, current)
	maps.Copy(result, desired)
	return result
}

// validateNameAndgenerateName validates that an object only has either `Name` or `GenerateName` set (and not both)
// If `GenerateName` is set, true is returned
// If an invalid combination of `Name` and `GenerateName` are set, an error is returned
func validateNameAndgenerateName(info *resource.Info) (bool, error) {
	accessor, err := meta.Accessor(info.Object)
	if err != nil {
		return true, err
	}

	if info.Name == "" && accessor.GetGenerateName() != "" {
		return true, nil
	}

	if info.Name != "" && accessor.GetGenerateName() != "" {
		return true, fmt.Errorf("metadata.name and metadata.generateName cannot both be set")
	}

	return false, nil
}


Review comment (suggestion):
```suggestion
		return false, err
```

`GenerateName` has not being determined to be true, we should not return true here

Code after:
	return accessor.SetLabels(obj, mergeStrStrMaps(current, labels))
}

func mergeAnnotations(obj runtime.Object, annotations map[string]string) error {
	current, err := accessor.Annotations(obj)
	if err != nil {
		return err
	}
	return accessor.SetAnnotations(obj, mergeStrStrMaps(current, annotations))
}

// merge two maps, always taking the value on the right
func mergeStrStrMaps(current, desired map[string]string) map[string]string {
	result := make(map[string]string)
	maps.Copy(result, current)
	maps.Copy(result, desired)
	return result
}

// validateNameAndGenerateName validates that an object only has either `Name` or `GenerateName` set (and not both)
// If `GenerateName` is set, true is returned
// If an invalid combination of `Name` and `GenerateName` are set, an error is returned
func validateNameAndGenerateName(info *resource.Info) (bool, error) {
	accessor, err := meta.Accessor(info.Object)
	if err != nil {
		return false, err
	}

	if info.Name == "" && accessor.GetGenerateName() != "" {
		return true, nil
	}

	if info.Name != "" && accessor.GetGenerateName() != "" {
		return true, fmt.Errorf("metadata.name and metadata.generateName cannot both be set")
	}

	return false, nil
}
Repository: helm/helm
File: pkg/action/validate.go (Go)

Code before:
	if err != nil {
		return err
	}
	return accessor.SetLabels(obj, mergeStrStrMaps(current, labels))
}

func mergeAnnotations(obj runtime.Object, annotations map[string]string) error {
	current, err := accessor.Annotations(obj)
	if err != nil {
		return err
	}
	return accessor.SetAnnotations(obj, mergeStrStrMaps(current, annotations))
}

// merge two maps, always taking the value on the right
func mergeStrStrMaps(current, desired map[string]string) map[string]string {
	result := make(map[string]string)
	maps.Copy(result, current)
	maps.Copy(result, desired)
	return result
}

// validateNameAndgenerateName validates that an object only has either `Name` or `GenerateName` set (and not both)
// If `GenerateName` is set, true is returned
// If an invalid combination of `Name` and `GenerateName` are set, an error is returned
func validateNameAndgenerateName(info *resource.Info) (bool, error) {
	accessor, err := meta.Accessor(info.Object)
	if err != nil {
		return false, err
	}

	if info.Name == "" && accessor.GetGenerateName() != "" {
		return true, nil
	}

	if info.Name != "" && accessor.GetGenerateName() != "" {
		return true, fmt.Errorf("metadata.name and metadata.generateName cannot both be set")
	}

	return false, nil
}


Review comment (suggestion):
```suggestion
func validateNameAndGenerateName(info *resource.Info) (bool, error) {
```

Just noticed (on my final review) the casing isn't correctly "camelCase" here

Code after:
	if err != nil {
		return err
	}
	return accessor.SetLabels(obj, mergeStrStrMaps(current, labels))
}

func mergeAnnotations(obj runtime.Object, annotations map[string]string) error {
	current, err := accessor.Annotations(obj)
	if err != nil {
		return err
	}
	return accessor.SetAnnotations(obj, mergeStrStrMaps(current, annotations))
}

// merge two maps, always taking the value on the right
func mergeStrStrMaps(current, desired map[string]string) map[string]string {
	result := make(map[string]string)
	maps.Copy(result, current)
	maps.Copy(result, desired)
	return result
}

// validateNameAndGenerateName validates that an object only has either `Name` or `GenerateName` set (and not both)
// If `GenerateName` is set, true is returned
// If an invalid combination of `Name` and `GenerateName` are set, an error is returned
func validateNameAndGenerateName(info *resource.Info) (bool, error) {
	accessor, err := meta.Accessor(info.Object)
	if err != nil {
		return false, err
	}

	if info.Name == "" && accessor.GetGenerateName() != "" {
		return true, nil
	}

	if info.Name != "" && accessor.GetGenerateName() != "" {
		return true, fmt.Errorf("metadata.name and metadata.generateName cannot both be set")
	}

	return false, nil
}
Repository: helm/helm
File: pkg/action/validate.go (Go)

Code before:

func mergeLabels(obj runtime.Object, labels map[string]string) error {
	current, err := accessor.Labels(obj)
	if err != nil {
		return err
	}
	return accessor.SetLabels(obj, mergeStrStrMaps(current, labels))
}

func mergeAnnotations(obj runtime.Object, annotations map[string]string) error {
	current, err := accessor.Annotations(obj)
	if err != nil {
		return err
	}
	return accessor.SetAnnotations(obj, mergeStrStrMaps(current, annotations))
}

// merge two maps, always taking the value on the right
func mergeStrStrMaps(current, desired map[string]string) map[string]string {
	result := make(map[string]string)
	maps.Copy(result, current)
	maps.Copy(result, desired)
	return result
}

// validateNameAndgenerateName validates that an object only has either `Name` or `GenerateName` set (and not both)
// If `GenerateName` is set, true is returned
// If an invalid combination of `Name` and `GenerateName` are set, an error is returned
func validateNameAndgenerateName(info *resource.Info) (bool, error) {
	accessor, err := meta.Accessor(info.Object)
	if err != nil {
		return false, err
	}

	if info.Name == "" && accessor.GetGenerateName() != "" {
		return true, nil
	}

	if info.Name != "" && accessor.GetGenerateName() != "" {
		return true, fmt.Errorf("metadata.name and metadata.generateName cannot both be set")
	}

	return false, nil
}


Review comment (suggestion):
```suggestion
// validateNameAndGenerateName validates that an object only has either `Name` or `GenerateName` set (and not both)
```

As below

Code after:

func mergeLabels(obj runtime.Object, labels map[string]string) error {
	current, err := accessor.Labels(obj)
	if err != nil {
		return err
	}
	return accessor.SetLabels(obj, mergeStrStrMaps(current, labels))
}

func mergeAnnotations(obj runtime.Object, annotations map[string]string) error {
	current, err := accessor.Annotations(obj)
	if err != nil {
		return err
	}
	return accessor.SetAnnotations(obj, mergeStrStrMaps(current, annotations))
}

// merge two maps, always taking the value on the right
func mergeStrStrMaps(current, desired map[string]string) map[string]string {
	result := make(map[string]string)
	maps.Copy(result, current)
	maps.Copy(result, desired)
	return result
}

// validateNameAndGenerateName validates that an object only has either `Name` or `GenerateName` set (and not both)
// If `GenerateName` is set, true is returned
// If an invalid combination of `Name` and `GenerateName` are set, an error is returned
func validateNameAndGenerateName(info *resource.Info) (bool, error) {
	accessor, err := meta.Accessor(info.Object)
	if err != nil {
		return false, err
	}

	if info.Name == "" && accessor.GetGenerateName() != "" {
		return true, nil
	}

	if info.Name != "" && accessor.GetGenerateName() != "" {
		return true, fmt.Errorf("metadata.name and metadata.generateName cannot both be set")
	}

	return false, nil
}
Repository: helm/helm
File: pkg/action/hooks.go (Go)

Code before:
limitations under the License.
*/

package action

import (
	"bytes"
	"fmt"
	"log"
	"slices"
	"sort"
	"time"

	"helm.sh/helm/v4/pkg/kube"

	"go.yaml.in/yaml/v3"
	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

	release "helm.sh/helm/v4/pkg/release/v1"
)

// execHook executes all of the hooks for the given hook event.
func (cfg *Configuration) execHook(rl *release.Release, hook release.HookEvent, waitStrategy kube.WaitStrategy, timeout time.Duration, serverSideApply bool) error {
	shutdown, err := cfg.execHookWithDelayedShutdown(rl, hook, waitStrategy, timeout, serverSideApply)
	if shutdown == nil {
		return nil
	}
	if err != nil {
		if err := shutdown(); err != nil {
			return err
		}
		return err
	}
	return shutdown()
}

type ExecuteShutdownHooks = func() error

func ShutdownNoOp() error {
	return nil
}

// execHookWithDelayedShutdown executes all of the hooks for the given hook event and returns a shutdownHook function to trigger deletions after doing other things like e.g. retrieving logs.
func (cfg *Configuration) execHookWithDelayedShutdown(rl *release.Release, hook release.HookEvent, waitStrategy kube.WaitStrategy, timeout time.Duration, serverSideApply bool) (ExecuteShutdownHooks, error) {
	executingHooks := []*release.Hook{}

	for _, h := range rl.Hooks {
		for _, e := range h.Events {
			if e == hook {
				executingHooks = append(executingHooks, h)
			}

Review comment (suggestion):
Return the error if there was one.

```suggestion
		return err
```

Code after:
limitations under the License.
*/

package action

import (
	"bytes"
	"fmt"
	"log"
	"slices"
	"sort"
	"time"

	"helm.sh/helm/v4/pkg/kube"

	"go.yaml.in/yaml/v3"
	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

	release "helm.sh/helm/v4/pkg/release/v1"
)

// execHook executes all of the hooks for the given hook event.
func (cfg *Configuration) execHook(rl *release.Release, hook release.HookEvent, waitStrategy kube.WaitStrategy, timeout time.Duration, serverSideApply bool) error {
	shutdown, err := cfg.execHookWithDelayedShutdown(rl, hook, waitStrategy, timeout, serverSideApply)
	if shutdown == nil {
		return err
	}
	if err != nil {
		if err := shutdown(); err != nil {
			return err
		}
		return err
	}
	return shutdown()
}

type ExecuteShutdownFunc = func() error

func shutdownNoOp() error {
	return nil
}

// execHookWithDelayedShutdown executes all of the hooks for the given hook event and returns a shutdownHook function to trigger deletions after doing other things like e.g. retrieving logs.
func (cfg *Configuration) execHookWithDelayedShutdown(rl *release.Release, hook release.HookEvent, waitStrategy kube.WaitStrategy, timeout time.Duration, serverSideApply bool) (ExecuteShutdownFunc, error) {
	executingHooks := []*release.Hook{}

	for _, h := range rl.Hooks {
		for _, e := range h.Events {
			if e == hook {
				executingHooks = append(executingHooks, h)
			}
Repository: helm/helm
File: pkg/chart/common/util/coalesce_test.go (Go)

Code before:

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

    http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package util

import (
	"bytes"
	"encoding/json"
	"fmt"
	"maps"
	"testing"
	"text/template"

	"github.com/stretchr/testify/assert"
	req "github.com/stretchr/testify/require"

	"helm.sh/helm/v4/pkg/chart/common"
	chart "helm.sh/helm/v4/pkg/chart/v2"
)

// ref: http://www.yaml.org/spec/1.2/spec.html#id2803362
var testCoalesceValuesYaml = []byte(`
top: yup
bottom: null
right: Null
left: NULL
front: ~
back: ""
nested:
  boat: null

global:
  name: Ishmael
  subject: Queequeg
  nested:
    boat: true

pequod:
  boat: null
  global:

Review comment (suggestion):
nit: why not the standard import name ie. `require` ?
```suggestion
	"github.com/stretchr/testify/require"
```

Code after:

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

    http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package util

import (
	"bytes"
	"encoding/json"
	"fmt"
	"maps"
	"testing"
	"text/template"

	"github.com/stretchr/testify/assert"

	"helm.sh/helm/v4/pkg/chart/common"
	chart "helm.sh/helm/v4/pkg/chart/v2"
)

// ref: http://www.yaml.org/spec/1.2/spec.html#id2803362
var testCoalesceValuesYaml = []byte(`
top: yup
bottom: null
right: Null
left: NULL
front: ~
back: ""
nested:
  boat: null

global:
  name: Ishmael
  subject: Queequeg
  nested:
    boat: true

pequod:
  boat: null
  global:
    name: Stinky
Repository: helm/helm
File: pkg/chart/common/util/coalesce_test.go (Go)

Code before:
		if vv, ok := v[nullKey]; !ok {
			t.Errorf("Expected key %q to be present but it was removed", nullKey)
		} else if vv != nil {
			t.Errorf("Expected key %q to be null but it has a value of %v", nullKey, vv)
		}
	}

	if _, ok := v["nested"].(map[string]interface{})["boat"]; !ok {
		t.Error("Expected nested boat key to be present but it was removed")
	}

	subchart := v["pequod"].(map[string]interface{})["ahab"].(map[string]interface{})
	if _, ok := subchart["boat"]; !ok {
		t.Error("Expected subchart boat key to be present but it was removed")
	}

	if _, ok := subchart["nested"].(map[string]interface{})["bar"]; !ok {
		t.Error("Expected subchart nested bar key to be present but it was removed")
	}

	// CoalesceValues should not mutate the passed arguments
	is.Equal(valsCopy, vals)
}

func TestCoalesceTables(t *testing.T) {
	t.Run("case 1", func(t *testing.T) {
		is := assert.New(t)
		dst := map[string]interface{}{
			"name": "Ishmael",
			"address": map[string]interface{}{
				"street":  "123 Spouter Inn Ct.",
				"city":    "Nantucket",
				"country": nil,
			},
			"details": map[string]interface{}{
				"friends": []string{"Tashtego"},
			},
			"boat": "pequod",
			"hole": nil,
		}
		src := map[string]interface{}{
			"occupation": "whaler",
			"address": map[string]interface{}{
				"state":   "MA",
				"street":  "234 Spouter Inn Ct.",
				"country": "US",
			},
			"details": "empty",
			"boat": map[string]interface{}{
				"mast": true,
			},

Review comment (refactor):
Perhaps this and the below `t.Run(...` separate test cases?

(but see my comment below about creating a new PR for these refactoring changes)

Code after:
			t.Errorf("Expected key %q to be present but it was removed", nullKey)
		} else if vv != nil {
			t.Errorf("Expected key %q to be null but it has a value of %v", nullKey, vv)
		}
	}

	if _, ok := v["nested"].(map[string]interface{})["boat"]; !ok {
		t.Error("Expected nested boat key to be present but it was removed")
	}

	subchart := v["pequod"].(map[string]interface{})["ahab"].(map[string]interface{})
	if _, ok := subchart["boat"]; !ok {
		t.Error("Expected subchart boat key to be present but it was removed")
	}

	if _, ok := subchart["nested"].(map[string]interface{})["bar"]; !ok {
		t.Error("Expected subchart nested bar key to be present but it was removed")
	}

	// CoalesceValues should not mutate the passed arguments
	is.Equal(valsCopy, vals)
}

func TestCoalesceTables(t *testing.T) {
	dst := map[string]interface{}{
		"name": "Ishmael",
		"address": map[string]interface{}{
			"street":  "123 Spouter Inn Ct.",
			"city":    "Nantucket",
			"country": nil,
		},
		"details": map[string]interface{}{
			"friends": []string{"Tashtego"},
		},
		"boat": "pequod",
		"hole": nil,
	}
	src := map[string]interface{}{
		"occupation": "whaler",
		"address": map[string]interface{}{
			"state":   "MA",
			"street":  "234 Spouter Inn Ct.",
			"country": "US",
		},
		"details": "empty",
		"boat": map[string]interface{}{
			"mast": true,
		},
		"hole": "black",
	}
Repository: hiyouga/LLaMA-Factory
File: examples/extras/badam/sft.sh (Shell)

Code before:
--template default \
--finetuning_type full \
--output_dir ../../../saves/LLaMA2-7B/badam \
--overwrite_cache \
--overwrite_output_dir \
--cutoff_len 1024 \
--preprocessing_num_workers 32 \
--per_device_train_batch_size 8 \
--per_device_eval_batch_size 5 \
--gradient_accumulation_steps 2 \
--lr_scheduler_type cosine \
--logging_steps 10 \
--warmup_steps 20 \
--save_steps 100 \
--eval_steps 100 \
--evaluation_strategy steps \
--load_best_model_at_end \
--learning_rate 5e-5 \
--num_train_epochs 3.0 \
--val_size 0.1 \
--plot_loss \
--use_badam \
--switch_mode descending \
--badam_verbose 2 \
--switch_block_every 50 \
--pure_bf16 \



Review comment (suggestion):
The meaning of `pure_bf16` is different from loading the original dtype, it always loads the model in bfloat16 type, but I think BAdam should be compatible with float16 type, using `fp16` can be more appropriate here

Code after:
    --finetuning_type full \
    --use_badam \
    --badam_switch_mode descending \
    --badam_switch_block_every 50 \
    --badam_verbose 2 \
    --output_dir ../../../saves/LLaMA2-7B/badam/sft \
    --overwrite_cache \
    --overwrite_output_dir \
    --cutoff_len 1024 \
    --preprocessing_num_workers 16 \
    --per_device_train_batch_size 1 \
    --per_device_eval_batch_size 1 \
    --gradient_accumulation_steps 8 \
    --lr_scheduler_type cosine \
    --logging_steps 10 \
    --warmup_steps 20 \
    --save_steps 100 \
    --eval_steps 100 \
    --evaluation_strategy steps \
    --load_best_model_at_end \
    --learning_rate 5e-5 \
    --num_train_epochs 3.0 \
    --max_samples 3000 \
    --val_size 0.1 \
    --plot_loss \
    --pure_bf16
Repository: hiyouga/LLaMA-Factory
File: src/llamafactory/chat/sglang_engine.py (Python)

Code before:
import asyncio
import gc
import os
import re
import subprocess
import time
import requests
import atexit
import json
from typing import (
    TYPE_CHECKING,
    Any,
    AsyncGenerator,
    AsyncIterator,
    Dict,
    List,
    Optional,
    Sequence,
)

import psutil

# Import SGLang correctly
import sglang as sgl
import torch
from typing_extensions import override

from ..data import get_template_and_fix_tokenizer
from ..extras.constants import (
    AUDIO_PLACEHOLDER,
    IMAGE_PLACEHOLDER,
    VIDEO_PLACEHOLDER,
)
from ..extras.logging import get_logger
from ..hparams import (
    DataArguments,
    FinetuningArguments,
    GeneratingArguments,
    ModelArguments,
)
from ..model import load_config, load_tokenizer
from ..model.model_utils.quantization import QuantizationMethod
from .base_engine import BaseEngine, Response


if TYPE_CHECKING:
    from ..data import AudioInput, ImageInput, VideoInput



Review comment (suggestion):
Please add `if_sglang_available` util function to packages.py https://github.com/hiyouga/LLaMA-Factory/blob/0dbce72fb8b03ea2c20809f3334c54d2106ff5c2/src/llamafactory/extras/packages.py#L98-L99

Code after:
# Copyright 2025 the LlamaFactory team.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
#     http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

import asyncio
import atexit
import json
from collections.abc import AsyncGenerator, AsyncIterator, Sequence
from typing import TYPE_CHECKING, Any, Optional, Union

import requests
from typing_extensions import override

from ..data import get_template_and_fix_tokenizer
from ..extras import logging
from ..extras.constants import AUDIO_PLACEHOLDER, IMAGE_PLACEHOLDER, VIDEO_PLACEHOLDER, EngineName
from ..extras.misc import get_device_count, torch_gc
from ..extras.packages import is_sglang_available
from ..hparams import DataArguments, FinetuningArguments, GeneratingArguments, ModelArguments
from ..model import load_config, load_tokenizer
from ..model.model_utils.quantization import QuantizationMethod
from .base_engine import BaseEngine, Response


if is_sglang_available():
    from sglang.utils import launch_server_cmd, terminate_process, wait_for_server


if TYPE_CHECKING:
    from ..data.mm_plugin import AudioInput, ImageInput, VideoInput


logger = logging.get_logger(__name__)


class SGLangEngine(BaseEngine):
    """Inference engine for SGLang models.

    This class wraps the SGLang engine to provide a consistent interface for text generation