ronantakizawa/codereview-bench huggingface dataset:
Repository: microsoft/retina
File: pkg/telemetry/telemetry.go (Go)

Code before:
		t.RUnlock()
	}
	client.Track(exception)
}

type PerformanceCounter struct {
	functionName string
	startTime    time.Time
}

// Used with start to measure the execution time of a function
// usage defer telemetry.StopPerf(telemetry.StartPerf("functionName"))
// start perf will be evaluated on defer line, and then stop perf will be evaluated on return
func (t *TelemetryClient) StartPerf(functionName string) *PerformanceCounter {
	return &PerformanceCounter{
		functionName: functionName,
		startTime:    time.Now(),
	}
}

func (t *TelemetryClient) StopPerf(counter *PerformanceCounter) {
	ms := float64(time.Since(counter.startTime).Milliseconds())
	t.TrackMetric(counter.functionName, ms, nil)
}

func (t *TelemetryClient) Heartbeat(ctx context.Context, heartbeatInterval time.Duration, customlabels map[string]string) {
	ticker := time.NewTicker(heartbeatInterval) // TODOL: make configurable
	defer ticker.Stop()

	for {
		select {
		case <-ctx.Done():
			return
		case <-ticker.C:
			t.heartbeat(ctx, customlabels)
		}
	}
}


Review comment (suggestion):
I don't think this is the intention? Once you call `t.Heartbeat()` it will run forever. So how will data be refreshed for custom data?

This should be a custom function? 
`func (t *TelemetryClient) Heartbeat(ctx context.Context, heartbeatInterval time.Duration, func()map[string]string...)`

Code after:
		for k, v := range t.properties {
			exception.Properties[k] = v
		}

		exception.Properties["process"] = t.processName

		t.RUnlock()
	}
	client.Track(exception)
}

type PerformanceCounter struct {
	functionName string
	startTime    time.Time
}

// Used with start to measure the execution time of a function
// usage defer telemetry.StopPerf(telemetry.StartPerf("functionName"))
// start perf will be evaluated on defer line, and then stop perf will be evaluated on return
func (t *TelemetryClient) StartPerf(functionName string) *PerformanceCounter {
	return &PerformanceCounter{
		functionName: functionName,
		startTime:    time.Now(),
	}
}

func (t *TelemetryClient) StopPerf(counter *PerformanceCounter) {
	ms := float64(time.Since(counter.startTime).Milliseconds())
	t.TrackMetric(counter.functionName, ms, nil)
}

func (t *TelemetryClient) Heartbeat(ctx context.Context, heartbeatInterval time.Duration, funcs ...func() map[string]string) {
	ticker := time.NewTicker(heartbeatInterval) // TODOL: make configurable
	defer ticker.Stop()

	for {
		select {
		case <-ctx.Done():
			return
		case <-ticker.C:
			t.heartbeat(ctx, funcs...)
		}
	}
}
Repository: microsoft/retina
File: pkg/capture/file/timestamp.go (Go)

Code before:
	"time"

	"github.com/pkg/errors"
	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

const captureFileNameTimestampFormat string = "20060102150405UTC"

func Now() *metav1.Time {
	return &metav1.Time{Time: time.Now().UTC().Truncate(time.Second)}
}

// Converts a string in the capture file name format to metav1.Time
func StringToTime(timestamp string) (*metav1.Time, error) {
	parsedTime, err := time.Parse(captureFileNameTimestampFormat, timestamp)
	if err != nil {
		return nil, errors.Wrap(err, "failed to create timestamp from string")
	}
	return &metav1.Time{Time: parsedTime}, nil
}

// Converts a metav1.Time to a string in the capture file name format
func TimeToString(timestamp *metav1.Time) string {
	return timestamp.Format(captureFileNameTimestampFormat)
}


Review comment (suggestion):
This does not seem right. It seems that currently `timestamp` can be nil only when the CaptureFilename pointer receiver of method String() does not have StartTimestamp initialized. Shall we initialize it there instead of doing it in TimeToString? 

https://github.com/microsoft/retina/blob/ba36848028f1591e2f40c8826da250eb316d0aa0/pkg/capture/file/capture_filename.go#L15-L18

Code after:
	"time"

	"github.com/pkg/errors"
	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

const captureFileNameTimestampFormat string = "20060102150405UTC"

func Now() *metav1.Time {
	return &metav1.Time{Time: time.Now().UTC().Truncate(time.Second)}
}

// Converts a string in the capture file name format to metav1.Time
func StringToTime(timestamp string) (*metav1.Time, error) {
	parsedTime, err := time.Parse(captureFileNameTimestampFormat, timestamp)
	if err != nil {
		return nil, errors.Wrap(err, "failed to create timestamp from string")
	}
	return &metav1.Time{Time: parsedTime}, nil
}

// Converts a metav1.Time to a string in the capture file name format
// Returns a zero time string if timestamp is nil
// Converts to UTC if other timezone is provided
func TimeToString(timestamp *metav1.Time) string {
	if timestamp == nil {
		return (&metav1.Time{Time: time.Time{}}).Format(captureFileNameTimestampFormat)
	}
	return timestamp.UTC().Format(captureFileNameTimestampFormat)
}
Repository: microsoft/retina
File: pkg/plugin/pktmon/pktmon_windows.go (Go)

Code before:
			"no events received within %v - check if another ETW consumer is active or if there is network traffic to monitor",
			eventHealthCheckFirstEvent)
	case err := <-resultCh:
		if err != nil {
			return err
		}
		p.l.Info("pktmon event stream verified - events flowing normally")
		return nil
	}
}

func (p *Plugin) GetFlow(ctx context.Context) error {
	ctx, cancel := context.WithCancel(ctx)
	defer cancel()

	err := p.StartStream(ctx)
	if err != nil {
		return errors.Wrapf(err, "failed to setup pktmon stream")
	}

	for {
		select {
		case <-ctx.Done():
			return errors.Wrapf(ctx.Err(), "pktmon plugin context done")
		default:
			event, err := p.stream.Recv()
			if err != nil {
				return errors.Wrapf(err, "failed to receive pktmon event")
			}

			fl := event.GetFlow()
			if fl == nil {
				p.l.Error("received nil flow, flow proto mismatch from client/server?")
				return nil
			}

			ev := &v1.Event{
				Event:     fl,
				Timestamp: fl.GetTime(),
			}

			if p.enricher != nil {
				p.enricher.Write(ev)
			} else {
				p.l.Error("enricher is nil when writing event")
			}

			// Write the event to the external channel.
			if p.externalChannel != nil {
				select {
				case p.externalChannel <- ev:

Review comment (bug):
is there any situation where nil flows could be generated that we would not want to fail? Or is it definitely always a problem?

if event.GetFlow() == nil, this fails straight away. 

Would we want to just ignore nil flows and maybe wait for a non nil flow eventually come? 

Code after:
				nilFlowCount++
				if nilFlowCount > maxNilFlowsAllowed {
					resultCh <- fmt.Errorf("exceeded max nil flows allowed: %d: %w", maxNilFlowsAllowed, ErrTooManyNilFlows)
					return
				}
				// Skip nil flows and wait for next event
				continue
			}

			// Got a valid flow
			resultCh <- nil
			return
		}
	}()

	// Wait for either a successful event or timeout
	select {
	case <-healthCtx.Done():
		// Timeout occurred - no events received, but stream connection is working.
		// This could be due to low/no traffic rather than ETW registration failure.
		p.l.Warn("no events received during health check",
			zap.Duration("timeout", pktmonHealthCheckTimeout),
			zap.String("cause", "likely no network traffic or node is idle"))
		return nil
	case err := <-resultCh:
		if err != nil {
			// Stream error indicates ETW registration failure or connection issue
			// Use %w to properly wrap for ErrorIs traversal
			return fmt.Errorf("%w: %w", ErrStreamHealthCheckFailed, err)
		}
		p.l.Info("pktmon event stream verified - events flowing normally")
		return nil
	}
}

func (p *Plugin) GetFlow(ctx context.Context) error {
	ctx, cancel := context.WithCancel(ctx)
	defer cancel()

	err := p.StartStream(ctx)
	if err != nil {
		return errors.Wrapf(err, "failed to setup pktmon stream")
	}

	for {
		select {
		case <-ctx.Done():
			return errors.Wrapf(ctx.Err(), "pktmon plugin context done")
		default:
			event, err := p.stream.Recv()
			if err != nil {
Repository: mingrammer/diagrams
File: diagrams/aws/network.py (Python)

Code before:

class CloudMap(_Network):
    _icon = "cloud-map.png"


class Cloudfront(_Network):
    _icon = "cloudfront.png"


class DirectConnect(_Network):
    _icon = "direct-connect.png"


class ElasticLoadBalancing(_Network):
    _icon = "elastic-load-balancing.png"


class GlobalAccelerator(_Network):
    _icon = "global-accelerator.png"


class InternetGateway(_Network):
    _icon = "internet-gateway.png"


class NateGateway(_Network):
    _icon = "nate-gateway.png"


class NetworkingAndContentDelivery(_Network):
    _icon = "networking-and-content-delivery.png"


class Privatelink(_Network):
    _icon = "privatelink.png"


class Route53(_Network):
    _icon = "route-53.png"


class SiteToSiteVpn(_Network):
    _icon = "site-to-site-vpn.png"


class TransitGateway(_Network):
    _icon = "transit-gateway.png"


class VPC(_Network):
    _icon = "vpc.png"

Review comment (suggestion):
NatGateway? It seems a typo. Please update the resource filename.

And `NAT` should be an uppercase, you can add it to upper words list: (lexicographical order) https://github.com/mingrammer/diagrams/blob/master/config.py#L50

Code after:

class CloudMap(_Network):
    _icon = "cloud-map.png"


class Cloudfront(_Network):
    _icon = "cloudfront.png"


class DirectConnect(_Network):
    _icon = "direct-connect.png"


class ElasticLoadBalancing(_Network):
    _icon = "elastic-load-balancing.png"


class GlobalAccelerator(_Network):
    _icon = "global-accelerator.png"


class InternetGateway(_Network):
    _icon = "internet-gateway.png"


class NATGateway(_Network):
    _icon = "nat-gateway.png"


class NetworkingAndContentDelivery(_Network):
    _icon = "networking-and-content-delivery.png"


class Privatelink(_Network):
    _icon = "privatelink.png"


class Route53(_Network):
    _icon = "route-53.png"


class SiteToSiteVpn(_Network):
    _icon = "site-to-site-vpn.png"


class TransitGateway(_Network):
    _icon = "transit-gateway.png"


class VPC(_Network):
    _icon = "vpc.png"
Repository: mingrammer/diagrams
File: config.py (Python)

Code before:
    "azure": ("Azure-",),
    "gcp": ("Cloud-",),
    "firebase": ("Cloud-",),
    "k8s": (),
    "alibabacloud": (),
    "oci": ("OCI-",),
    "programming": (),
}

#########################
#  Doc Auto Generation  #
#########################

TMPL_APIDOC = "apidoc.tmpl"

#########################
# Class Auto Generation #
#########################

TMPL_MODULE = "module.tmpl"

UPPER_WORDS = {
    "aws": ("aws", "api", "ebs", "ec2", "efs", "emr", "rds", "ml", "mq", "nat", "vpc", "waf", "sdk"),
    "azure": ("ad", "b2c", "ai", "api", "cdn", "ddos", "dns", "fxt", "hana", "hd", "id", "sap", "sql", "vm"),
    "gcp": ("gcp", "ai", "api", "cdn", "dns", "gke", "gpu", "iap", "ml", "nat", "os", "sdk", "sql", "tpu", "vpn"),
    "firebase": ("fcm", ),
    "k8s": (
        "api", "cm", "ccm", "crb", "crd", "ds", "etcd", "hpa", "k8s", "ns", "psp", "pv", "pvc", "rb", "rs",
        "sa", "sc", "sts", "svc",
    ),
    "oci": ("oci",),
}

TITLE_WORDS = {
    "onprem": {
        "onprem": "OnPrem",
    },
    "alibabacloud": {
        "alibabacloud": "AlibabaCloud"
    },
    "aws": {
        "cloudfront": "CloudFront"
    }
}

# TODO: check if the classname exists
ALIASES = {
    "onprem": {
        "ci": {
            "Circleci": "CircleCI",
            "Gitlabci": "GitlabCI",

Review comment (suggestion):
Last request: `Ab` of ABTesting and `Ml` of MLKit should be uppercase. Could you please add these in this tuple? (maybe `ab`, `fcm`, `ml`)

Code after:
    "azure": ("Azure-",),
    "gcp": ("Cloud-",),
    "firebase": ("Cloud-",),
    "k8s": (),
    "alibabacloud": (),
    "oci": ("OCI-",),
    "programming": (),
}

#########################
#  Doc Auto Generation  #
#########################

TMPL_APIDOC = "apidoc.tmpl"

#########################
# Class Auto Generation #
#########################

TMPL_MODULE = "module.tmpl"

UPPER_WORDS = {
    "aws": ("aws", "api", "ebs", "ec2", "efs", "emr", "rds", "ml", "mq", "nat", "vpc", "waf", "sdk"),
    "azure": ("ad", "b2c", "ai", "api", "cdn", "ddos", "dns", "fxt", "hana", "hd", "id", "sap", "sql", "vm"),
    "gcp": ("gcp", "ai", "api", "cdn", "dns", "gke", "gpu", "iap", "ml", "nat", "os", "sdk", "sql", "tpu", "vpn"),
    "firebase": ("ab", "fcm", "ml"),
    "k8s": (
        "api", "cm", "ccm", "crb", "crd", "ds", "etcd", "hpa", "k8s", "ns", "psp", "pv", "pvc", "rb", "rs",
        "sa", "sc", "sts", "svc",
    ),
    "oci": ("oci",),
}

TITLE_WORDS = {
    "onprem": {
        "onprem": "OnPrem",
    },
    "alibabacloud": {
        "alibabacloud": "AlibabaCloud"
    },
    "aws": {
        "cloudfront": "CloudFront"
    }
}

# TODO: check if the classname exists
ALIASES = {
    "onprem": {
        "ci": {
            "Circleci": "CircleCI",
            "Gitlabci": "GitlabCI",
Repository: mingrammer/diagrams
File: diagrams/onprem/certificates.py (Python)

Code before:
# This module is automatically generated by autogen.sh. DO NOT EDIT.

from . import _OnPrem


class _Certificates(_OnPrem):
    _type = "certificates"
    _icon_dir = "resources/onprem/certificates"


class Certmanager(_Certificates):
    _icon = "certmanager.png"


class Letsencrypt(_Certificates):
    _icon = "letsencrypt.png"


# Aliases


Review comment (suggestion):
I request some changes similar to [this request change](https://github.com/mingrammer/diagrams/pull/260#discussion_r487851767).

It should be `class CertManager`.

Code after:
# This module is automatically generated by autogen.sh. DO NOT EDIT.

from . import _OnPrem


class _Certificates(_OnPrem):
    _type = "certificates"
    _icon_dir = "resources/onprem/certificates"


class CertManager(_Certificates):
    _icon = "cert-manager.png"


class LetsEncrypt(_Certificates):
    _icon = "lets-encrypt.png"


# Aliases
Repository: mingrammer/diagrams
File: diagrams/onprem/certificates.py (Python)

Code before:
# This module is automatically generated by autogen.sh. DO NOT EDIT.

from . import _OnPrem


class _Certificates(_OnPrem):
    _type = "certificates"
    _icon_dir = "resources/onprem/certificates"


class Certmanager(_Certificates):
    _icon = "certmanager.png"


class Letsencrypt(_Certificates):
    _icon = "letsencrypt.png"


# Aliases


Review comment (suggestion):
I request some changes similar to [this request change](https://github.com/mingrammer/diagrams/pull/260#discussion_r487851767).

It should be `class LetsEncrypt`.

Code after:
# This module is automatically generated by autogen.sh. DO NOT EDIT.

from . import _OnPrem


class _Certificates(_OnPrem):
    _type = "certificates"
    _icon_dir = "resources/onprem/certificates"


class CertManager(_Certificates):
    _icon = "cert-manager.png"


class LetsEncrypt(_Certificates):
    _icon = "lets-encrypt.png"


# Aliases
Repository: mingrammer/diagrams
File: config.py (Python)

Code before:
            "Couchdb": "CouchDB",
            "Hbase": "HBase",
            "Influxdb": "InfluxDB",
            "Janusgraph": "JanusGraph",
            "Mariadb": "MariaDB",
            "Mongodb": "MongoDB",
            "Mssql": "MSSQL",
            "Mysql": "MySQL",
            "Postgresql": "PostgreSQL",
        },
        "gitops": {
            "Argocd": "ArgoCD",
        },
        "logging": {
            "Fluentbit": "FluentBit",
            "Rsyslog": "RSyslog",
        },
        "network": {
            "Etcd": "ETCD",
            "Haproxy": "HAProxy",
            "OpenServiceMesh": "OSM",
            "Pfsense": "PFSense",
            "Vyos": "VyOS"
        },
        "proxmox": {
            "ProxmoxVE": "PVE",
        },
        "queue": {
            "Activemq": "ActiveMQ",
            "Rabbitmq": "RabbitMQ",
            "Zeromq": "ZeroMQ",
        },
        "workflow": {
            "Kubeflow": "KubeFlow",
            "Nifi": "NiFi",
        }
    },
    "aws": {
        "analytics": {
            "ElasticsearchService": "ES",
        },
        "business": {
            "AlexaForBusiness": "A4B"
        },
        "blockchain": {
            "QuantumLedgerDatabaseQldb": "QLDB"
        },
        "compute": {
            "ApplicationAutoScaling": "AutoScaling",
            "EC2ContainerRegistry": "ECR",
            "ElasticBeanstalk": "EB",

Review comment (suggestion):
It should be `"Pve": "ProxmoxVE",` and need to rerun `autogen.sh` to update alias for proxmox. This might be the last request change.

Code after:
            "Couchdb": "CouchDB",
            "Hbase": "HBase",
            "Influxdb": "InfluxDB",
            "Janusgraph": "JanusGraph",
            "Mariadb": "MariaDB",
            "Mongodb": "MongoDB",
            "Mssql": "MSSQL",
            "Mysql": "MySQL",
            "Postgresql": "PostgreSQL",
        },
        "gitops": {
            "Argocd": "ArgoCD",
        },
        "logging": {
            "Fluentbit": "FluentBit",
            "Rsyslog": "RSyslog",
        },
        "network": {
            "Etcd": "ETCD",
            "Haproxy": "HAProxy",
            "OpenServiceMesh": "OSM",
            "Pfsense": "PFSense",
            "Vyos": "VyOS"
        },
        "proxmox": {
            "Pve": "ProxmoxVE",
        },
        "queue": {
            "Activemq": "ActiveMQ",
            "Rabbitmq": "RabbitMQ",
            "Zeromq": "ZeroMQ",
        },
        "storage": {
            "ceph": {
                "CEPH": "ceph",
                "Ceph-OSD": "CEPH-OSD"
            }
        },
        "workflow": {
            "Kubeflow": "KubeFlow",
            "Nifi": "NiFi",
        }
    },
    "aws": {
        "analytics": {
            "ElasticsearchService": "ES",
        },
        "business": {
            "AlexaForBusiness": "A4B"
        },
        "blockchain": {
Repository: mlc-ai/mlc-llm
File: python/mlc_chat/interface/bench.py (Python)

Code before:
"""Python entrypoint of benchmark."""
from typing import Optional

from mlc_chat.chat_module import ChatConfig, ChatModule

from .compiler_flags import ModelConfigOverride


def bench(  # pylint: disable=too-many-arguments
    model: str,
    prompt: str,
    device: str,
    opt: str,
    overrides: ModelConfigOverride,
    generate_length: int,
    model_lib_path: Optional[str],
):
    """run the benchmarking"""
    # Set up chat config and generate config
    config = ChatConfig(opt=opt)
    # Apply overrides
    config = overrides.apply(config)
    # Set up ChatModule
    cm = ChatModule(model, device, chat_config=config, model_lib_path=model_lib_path)

    output = cm.benchmark_generate(prompt, generate_length=generate_length)
    print(f"Generated text:\n{output}\n")
    print(f"Statistics:\n{cm.stats(verbose=True)}")


Review comment (suggestion):
Unfortunately it's a bit more complicated here. `ModelConfigOverride` is designed to only apply to `ModelConfig`, such as `LlamaConfig`, etc., rather than ChatConfig. We will have to add most of [those flags](https://github.com/mlc-ai/mlc-llm/blob/eddc5b146f458d331ca89d414c608b2465c0947a/python/mlc_chat/chat_module.py#L190-L210) to argparse :(((

Code after:
"""Python entrypoint of benchmark."""
from typing import Optional

from mlc_chat.chat_module import ChatConfig, ChatModule

from .chat import ChatConfigOverride


def bench(  # pylint: disable=too-many-arguments
    model: str,
    prompt: str,
    device: str,
    opt: str,
    overrides: ChatConfigOverride,
    generate_length: int,
    model_lib_path: Optional[str],
):
    """run the benchmarking"""
    # Set up chat config
    config = ChatConfig(opt=opt)
    # Apply overrides
    config = overrides.apply(config)
    # Set up ChatModule
    cm = ChatModule(model, device, chat_config=config, model_lib_path=model_lib_path)

    output = cm.benchmark_generate(prompt, generate_length=generate_length)
    print(f"Generated text:\n{output}\n")
    print(f"Statistics:\n{cm.stats(verbose=True)}")
Repository: mlc-ai/mlc-llm
File: python/mlc_chat/support/max_thread_check.py (Python)

Code before:
"""Helper functions for checking max num thread."""

from tvm.target import Target


def check_max_num_threads(target: Target, bdx: int, bdy: int, bdz: int):
    """Check whether max num threads exceeded given a target."""
    assert (
        bdx * bdy * bdz <= target.max_num_threads
    ), f"{target.kind} max num threads exceeded: {bdx}*{bdy}*{bdz}>{target.max_num_threads}"

    if str(target.kind) != "webgpu":
        # https://gpuweb.github.io/gpuweb/#dom-supported-limits-maxcomputeworkgroupsizez
        assert bdz <= 64, f"webgpu's z dimension cannot exceed 64, but got bdz={bdz}"


Review comment (suggestion):
Should it be `== "webgpu"`? Also for WebGPU `blockIdx.z` should be 1, which is also be something we could check.

Code after:
"""Helper functions for checking max num thread."""

from tvm.target import Target


def get_max_num_threads_per_block(target: Target):
    """
    max(max_num_threads, max_threads_per_block); if latter does not exist, return max_num_threads.
    We add this method since some targets have both fields and `max_threads_per_block` is larger.
    """
    max_num_threads = target.max_num_threads
    max_threads_per_block = target.attrs.get("max_threads_per_block", None)
    if max_threads_per_block is None:
        return max_num_threads
    return max(max_num_threads, max_threads_per_block)


def check_thread_limits(target: Target, bdx: int, bdy: int, bdz: int, gdz: int):
    """
    Check whether max num threads exceeded given a target.

    Parameters
    ----------
    bdx: threadIdx.x
    bdy: threadIdx.y
    bdz: threadIdx.z
    gdz: blockIdx.z
    """
    max_num_threads_per_block = get_max_num_threads_per_block(target)

    assert (
        bdx * bdy * bdz <= max_num_threads_per_block
    ), f"{target.kind} max num threads exceeded: {bdx}*{bdy}*{bdz}>{max_num_threads_per_block}"

    if str(target.kind) == "webgpu":
        # https://gpuweb.github.io/gpuweb/#dom-supported-limits-maxcomputeworkgroupsizez
        assert bdz <= 64, f"webgpu's threadIdx.z cannot exceed 64, but got bdz={bdz}"