ronantakizawa/codereview-bench huggingface dataset:
Repository: honojs/hono
File: src/helper/ssg/plugins.ts (TypeScript)

Code before:
<meta http-equiv="refresh" content="0;url=${to}" />
<meta name="robots" content="noindex" />
<link rel="canonical" href="${to}" />
<body>
  <a href="${to}">Redirecting from <code>${from}</code> to <code>${to}</code></a>
</body>`
  return html.replace(/\n/g, '')
}

/**
 * Redirect plugin for Hono SSG.
 *
 * Generates HTML redirect pages for HTTP 301 and 302 responses.
 *
 * @returns A SSGPlugin that generates HTML redirect pages.
 *
 * @experimental
 * `redirectPlugin` is an experimental feature.
 * The API might be changed.
 */
export const redirectPlugin = (): SSGPlugin => {
  return {
    afterResponseHook: (res) => {
      if (res.status === 301 || res.status === 302) {
        const location = res.headers.get('Location')
        if (!location) return false
        const html = generateRedirectHtml('', location)
        return new Response(html, {
          status: 200,
          headers: { 'Content-Type': 'text/html; charset=utf-8' },
        })
      }
      return res
    },
  }
}


Review comment (suggestion):
Please use this syntax that we use in this `hono` project.

```ts
if (!location) {
  return false
}
```

Code after:
 *
 * @experimental
 * `defaultPlugin` is an experimental feature.
 * The API might be changed.
 */
export const defaultPlugin = (): SSGPlugin => {
  return {
    afterResponseHook: (res) => {
      if (res.status !== 200) {
        return false
      }
      return res
    },
  }
}

const REDIRECT_STATUS_CODES = new Set([301, 302, 303, 307, 308])

const generateRedirectHtml = (location: string) => {
  // prettier-ignore
  const content = html`<!DOCTYPE html>
<title>Redirecting to: ${location}</title>
<meta http-equiv="refresh" content="0;url=${location}" />
<meta name="robots" content="noindex" />
<link rel="canonical" href="${location}" />
<body>
<a href="${location}">Redirecting to <code>${location}</code></a>
</body>
`
  return content.toString().replace(/\n/g, '')
}

/**
 * The redirect plugin that generates HTML redirect pages for HTTP redirect responses for status codes 301, 302, 303, 307 and 308.
 *
 * When used with `defaultPlugin`, place `redirectPlugin` before it, because `defaultPlugin` skips non-200 responses.
 *
 * ```ts
 * // ✅ Will work as expected
 * toSSG(app, fs, { plugins: [redirectPlugin(), defaultPlugin()] })
 *
 * // ❌ Will not work as expected
 * toSSG(app, fs, { plugins: [defaultPlugin(), redirectPlugin()] })
 * ```
 *
 * @experimental
 * `redirectPlugin` is an experimental feature.
 * The API might be changed.
 */
export const redirectPlugin = (): SSGPlugin => {
  return {
Repository: honojs/hono
File: src/helper/ssg/plugins.ts (TypeScript)

Code before:
import type { SSGPlugin } from './ssg'

const generateRedirectHtml = (from: string, to: string) => {
  const html = `<!DOCTYPE html>
<title>Redirecting to: ${to}</title>
<meta http-equiv="refresh" content="0;url=${to}" />
<meta name="robots" content="noindex" />
<link rel="canonical" href="${to}" />
<body>
  <a href="${to}">Redirecting from <code>${from}</code> to <code>${to}</code></a>
</body>`
  return html.replace(/\n/g, '')
}

/**
 * Redirect plugin for Hono SSG.
 *
 * Generates HTML redirect pages for HTTP 301 and 302 responses.
 *
 * @returns A SSGPlugin that generates HTML redirect pages.
 *
 * @experimental
 * `redirectPlugin` is an experimental feature.
 * The API might be changed.
 */
export const redirectPlugin = (): SSGPlugin => {
  return {
    afterResponseHook: (res) => {
      if (res.status === 301 || res.status === 302) {
        const location = res.headers.get('Location')

Review comment (security):
It's a rare case if passing the script into `to`, but I think you should escape it. How about `html`?

```ts
import { html } from '../html'
```
And you can esapce `to` with ``${html`${to}`}``

Code after:
import { html } from '../html'
import type { SSGPlugin } from './ssg'

/**
 * The default plugin that defines the recommended behavior.
 *
 * @experimental
 * `defaultPlugin` is an experimental feature.
 * The API might be changed.
 */
export const defaultPlugin = (): SSGPlugin => {
  return {
    afterResponseHook: (res) => {
      if (res.status !== 200) {
        return false
      }
      return res
    },
  }
}

const REDIRECT_STATUS_CODES = new Set([301, 302, 303, 307, 308])

const generateRedirectHtml = (location: string) => {
  // prettier-ignore
  const content = html`<!DOCTYPE html>
<title>Redirecting to: ${location}</title>
<meta http-equiv="refresh" content="0;url=${location}" />
<meta name="robots" content="noindex" />
<link rel="canonical" href="${location}" />
Repository: honojs/hono
File: src/helper/ssg/index.ts (TypeScript)

Code before:
/**
 * @module
 * SSG Helper for Hono.
 */

export * from './ssg'
export {
  X_HONO_DISABLE_SSG_HEADER_KEY,
  ssgParams,
  isSSGContext,
  disableSSG,
  onlySSG,
} from './middleware'
export { redirectPlugin } from './plugins'


Review comment (suggestion):
The `defaultPlugin` is moved. So, you should export the `defaultPlugin` from the `plugins.ts`:

```ts
export { defaultPlugin, redirectPlugin } from './plugins'
```

Code after:
/**
 * @module
 * SSG Helper for Hono.
 */

export * from './ssg'
export {
  X_HONO_DISABLE_SSG_HEADER_KEY,
  ssgParams,
  isSSGContext,
  disableSSG,
  onlySSG,
} from './middleware'
export { defaultPlugin, redirectPlugin } from './plugins'
Repository: honojs/hono
File: src/helper/ssg/index.ts (TypeScript)

Code before:
/**
 * @module
 * SSG Helper for Hono.
 */

export * from './ssg'
export {
  X_HONO_DISABLE_SSG_HEADER_KEY,
  ssgParams,
  isSSGContext,
  disableSSG,
  onlySSG,
} from './middleware'


Review comment (suggestion):
I'm wondering whether I should use `export *`.

```ts
export * from './plugins'
```

Code after:
/**
 * @module
 * SSG Helper for Hono.
 */

export * from './ssg'
export {
  X_HONO_DISABLE_SSG_HEADER_KEY,
  ssgParams,
  isSSGContext,
  disableSSG,
  onlySSG,
} from './middleware'
export { defaultPlugin, redirectPlugin } from './plugins'
Repository: honojs/hono
File: src/adapter/netlify/conninfo.ts (TypeScript)

Code before:
  },
})

/**
 * Get geolocation data from Netlify context
 * @param c - Context
 * @returns Geolocation data or undefined
 * @example
 * ```ts
 * import { Hono } from 'hono'
 * import { handle, getGeo } from 'hono/netlify'
 *
 * const app = new Hono()
 *
 * app.get('/', (c) => {
 *   const geo = getGeo(c)
 *   return c.json({
 *     country: geo?.country?.code,
 *     city: geo?.city,
 *   })
 * })
 *
 * export default handle(app)
 * ```
 */
export const getGeo = (c: Context<Env>): NetlifyContext['geo'] | undefined => c.env.context?.geo


Review comment (suggestion):
I think `getGeo` should not be covered by the conninfo. It's out of scope. The user can get the info by themselves.

Code after:
import type { Context } from '../../context'
import type { GetConnInfo } from '../../helper/conninfo'

/**
 * Netlify context type
 * @see https://docs.netlify.com/functions/api/
 */
type NetlifyContext = {
  ip?: string
  geo?: {
    city?: string
    country?: {
      code?: string
      name?: string
    }
    subdivision?: {
      code?: string
      name?: string
    }
    latitude?: number
    longitude?: number
    timezone?: string
    postalCode?: string
  }
  requestId?: string
}

type Env = {
  Bindings: {
    context: NetlifyContext
  }
}

/**
 * Get connection information from Netlify
 * @param c - Context
 * @returns Connection information including remote address
 * @example
 * ```ts
 * import { Hono } from 'hono'
 * import { handle, getConnInfo } from 'hono/netlify'
 *
 * const app = new Hono()
 *
 * app.get('/', (c) => {
 *   const info = getConnInfo(c)
 *   return c.text(`Your IP: ${info.remote.address}`)
 * })
 *
 * export default handle(app)
 * ```
 */
export const getConnInfo: GetConnInfo = (c: Context<Env>) => ({
  remote: {
    address: c.env.context?.ip,
  },
})
Repository: hpcaitech/ColossalAI
File: colossalai/checkpoint_io/index_file.py (Python)

Code before:

        return checkpoint_list, dtensor_list

    def assert_no_dtensor_checkpoint(self):
        for val in self.weight_map.values():
            if is_dtensor_checkpoint(val):
                raise ValueError(f"Checkpoint file {val} contains distributed tensor")

    def get_checkpoint_file(self, param_name: str) -> str:
        """
        Get the checkpoint file name for a parameter.

        Args:
            param_name (str): name of the parameter.

        Returns:
            str: checkpoint file name.
        """
        ckpt_path = self.weight_map[param_name]
        return ckpt_path
    
    def get_checkpoint_metadata(self) -> str: 
        """
        Get the checkpoint index metadata.
        """
        return self.metadata
    
    def get_all_param_names(self):
        """
        Get all the weight keys.
        """
        return list(self.weight_map.keys())


Review comment (suggestion):
this method is kind of redundant, since we can just do like this

```python
index_file = CheckpointIndexFile.from_file('/path/to/json')
index_file.metadata

```

Code after:

        return checkpoint_list, dtensor_list

    def assert_no_dtensor_checkpoint(self):
        for val in self.weight_map.values():
            if is_dtensor_checkpoint(val):
                raise ValueError(f"Checkpoint file {val} contains distributed tensor")

    def get_checkpoint_file(self, param_name: str) -> str:
        """
        Get the checkpoint file name for a parameter.

        Args:
            param_name (str): name of the parameter.

        Returns:
            str: checkpoint file name.
        """
        ckpt_path = self.weight_map[param_name]
        return ckpt_path
    
    def get_all_param_names(self):
        """
        Get all the weight keys.
        """
        return list(self.weight_map.keys())
Repository: hpcaitech/ColossalAI
File: colossalai/shardformer/layer/dropout.py (Python)

Code before:
import os
import time
from contextlib import contextmanager

import torch
import torch.nn as nn


class SeedManager:
    """
    This class is a random state manager to change random state for different random seed.

    """

    def __init__(self):
        original_state = torch.cuda.get_rng_state()
        seed = os.getpid()
        torch.cuda.manual_seed(int(seed))
        self.dropout_state = torch.cuda.get_rng_state()
        torch.cuda.set_rng_state(original_state)

    def set_mode(self, rng_state):
        torch.cuda.set_rng_state(rng_state)

    def get_current_mode(self):
        current_state = torch.cuda.get_rng_state()
        return current_state

    @contextmanager
    def dropout_mode(self):
        """
        This is a context manager to change the dropout state and recover the original state.

        Usage:
        ::
            >>> with _seed_manager.dropout_mode():
            >>>     input = super().forward(input)
        """
        try:
            current_mode = self.get_current_mode()
            yield self.set_mode(self.dropout_state)
        finally:

Review comment (refactor):
You can add a `TODO` note here to alert us that there will be some refactoring work to do in the future.

```python
# TODO: unify this seed manager with the colossalai.context.random
seed = os.getpid()
```

Code after:
import os
from contextlib import contextmanager

import torch
import torch.nn as nn


class SeedManager:
    """
    This class is a random state manager to change random state for different random seed.

    """

    def __init__(self):
        original_state = torch.cuda.get_rng_state()
        # TODO: unify this seed manager with the colossalai.context.random
        seed = os.getpid()
        torch.cuda.manual_seed(int(seed))
        self.dropout_state = torch.cuda.get_rng_state()
        torch.cuda.set_rng_state(original_state)

    def set_mode(self, rng_state):
        torch.cuda.set_rng_state(rng_state)

    def get_current_mode(self):
        current_state = torch.cuda.get_rng_state()
        return current_state

    @contextmanager
    def dropout_mode(self):
        """
        This is a context manager to change the dropout state and recover the original state.

        Usage:
        ::
            >>> with _seed_manager.dropout_mode():
            >>>     input = super().forward(input)
        """
        try:
            current_mode = self.get_current_mode()
            yield self.set_mode(self.dropout_state)
        finally:
Repository: hpcaitech/ColossalAI
File: colossalai/kernel/cuda_native/csrc/smoothquant/linear.cu (CUDA)

Code before:
#include "linear.h"
#include <cutlass/core_io.h>
#include <cutlass/cutlass.h>
#include <cutlass/half.h>

#include <cutlass/gemm/device/gemm.h>
#include <cutlass/numeric_types.h>
#include <cutlass/util/host_tensor.h>
#include <cutlass/epilogue/thread/linear_combination_silu.h>
#include <cstdint>
#include <cuda.h>
#include <cuda_runtime.h>
#include <cuda_fp16.h>
#include <iostream>
#include <torch/torch.h>
torch::Tensor linear_silu_a8_w8_bfp32_ofp32(torch::Tensor input,  // INT8
                                       torch::Tensor weight, // INT8
                                       torch::Tensor bias,   // FP32
                                       float alpha,          // FP32
                                       float beta            // FP32
) {
  auto M = input.size(0);
  auto N = weight.size(0);
  auto K = input.size(1);

  using ElementOutput = float;
  using ElementAccumulator = int32_t;
  using ElementComputeEpilogue = float;
  using ElementInputA = int8_t; // <- data type of elements in input matrix A

Review comment (suggestion):
Wonder if we should add 
```python
// modified from XXX
```

Code after:
// modified from https://github.com/Guangxuan-Xiao/torch-int/blob/main/torch_int/kernels/linear.cu

#include "linear.h"
#include <cutlass/core_io.h>
#include <cutlass/cutlass.h>
#include <cutlass/half.h>

#include <cutlass/gemm/device/gemm.h>
#include <cutlass/numeric_types.h>
#include <cutlass/util/host_tensor.h>
#include <cutlass/epilogue/thread/linear_combination_silu.h>
#include <cstdint>
#include <cuda.h>
#include <cuda_runtime.h>
#include <cuda_fp16.h>
#include <iostream>
#include <torch/torch.h>
torch::Tensor linear_silu_a8_w8_bfp32_ofp32(torch::Tensor input,  // INT8
                                       torch::Tensor weight, // INT8
                                       torch::Tensor bias,   // FP32
                                       float alpha,          // FP32
                                       float beta            // FP32
) {
  auto M = input.size(0);
  auto N = weight.size(0);
  auto K = input.size(1);

  using ElementOutput = float;
  using ElementAccumulator = int32_t;
Repository: hpcaitech/ColossalAI
File: colossalai/accelerator/api.py (Python)

Code before:

    global _ACCELERATOR

    if isinstance(accelerator, str):
        _ACCELERATOR = _ACCELERATOR_MAPPING[accelerator]()
    elif isinstance(accelerator, BaseAccelerator):
        _ACCELERATOR = accelerator
    else:
        raise TypeError("accelerator must be either a string or an instance of BaseAccelerator")


def auto_set_accelerator() -> None:
    """
    Automatically check if any accelerator is available.
    If an accelerator is availabe, set it as the global accelerator.
    """
    global _ACCELERATOR

    for _, accelerator_cls in _ACCELERATOR_MAPPING.items():
        try:
            accelerator = accelerator_cls()

            if accelerator.is_available():
                _ACCELERATOR = accelerator
            break
        except RuntimeError:
            pass

    if _ACCELERATOR is None:
        raise RuntimeError(
            f"No accelerator is available. Please check your environment. The list of accelerators we support is {list(_ACCELERATOR_MAPPING.keys())}"
        )


def get_accelerator() -> BaseAccelerator:
    """
    Return the accelerator for the current process. If the accelerator is not initialized, it will be initialized
    to the default accelerator type.

    Returns: the accelerator for the current process.
    """
    global _ACCELERATOR

    if _ACCELERATOR is None:
        _ACCELERATOR = _DEFAULT_ACCELERATOR_TYPE()
    return _ACCELERATOR


Review comment (suggestion):
![image](https://github.com/hpcaitech/ColossalAI/assets/23111350/f55e1509-fed0-4f2c-9c62-8b0c78c227ef)
Maybe it's not runtime error

Code after:

    if isinstance(accelerator, str):
        _ACCELERATOR = _ACCELERATOR_MAPPING[accelerator]()
    elif isinstance(accelerator, BaseAccelerator):
        _ACCELERATOR = accelerator
    else:
        raise TypeError("accelerator must be either a string or an instance of BaseAccelerator")


def auto_set_accelerator() -> None:
    """
    Automatically check if any accelerator is available.
    If an accelerator is availabe, set it as the global accelerator.
    """
    global _ACCELERATOR

    for _, accelerator_cls in _ACCELERATOR_MAPPING.items():
        try:
            accelerator = accelerator_cls()
            if accelerator.is_available():
                _ACCELERATOR = accelerator
            break
        except:
            pass

    if _ACCELERATOR is None:
        raise RuntimeError(
            f"No accelerator is available. Please check your environment. The list of accelerators we support is {list(_ACCELERATOR_MAPPING.keys())}"
        )


def get_accelerator() -> BaseAccelerator:
    """
    Return the accelerator for the current process. If the accelerator is not initialized, it will be initialized
    to the default accelerator type.

    Returns: the accelerator for the current process.
    """
    global _ACCELERATOR

    if _ACCELERATOR is None:
        auto_set_accelerator()
    return _ACCELERATOR
Repository: hpcaitech/ColossalAI
File: colossalai/inference/flash_decoding_utils.py (Python)

Code before:
import torch

from colossalai.context.singleton_meta import SingletonMeta
from colossalai.utils import get_current_device


class FDIntermTensors(metaclass=SingletonMeta):
    """Singleton class to hold tensors used for storing intermediate values in flash-decoding.
    For now, it holds intermediate output and logsumexp (which will be used in reduction step along kv)
    """

    def __init__(self):
        self._tensors_initialized = False

    def _re_initialize(self):
        self._tensors_initialized = False
        del self._mid_output
        del self._mid_output_lse

    @property
    def is_initialized(self):
        return self._tensors_initialized

    @property
    def mid_output(self):
        assert self.is_initialized, "Intermediate tensors not initialized yet"
        return self._mid_output

    @property
    def mid_output_lse(self):
        assert self.is_initialized, "Intermediate tensors not initialized yet"
        return self._mid_output_lse

    def initialize(
        self,
        max_batch_size: int,
        num_attn_heads: int,
        kv_max_split_num: int,
        head_dim: int,
        dtype: torch.dtype = torch.float32,
        device: torch.device = get_current_device(),
    ) -> None:
        """Initialize tensors.

Review comment (bug):
This is semantically incorrect, `re-initialize` means that something is created again even though it has been already created. The function body seems to me that the variables are only deleted and not created again?

Code after:
import torch

from colossalai.context.singleton_meta import SingletonMeta
from colossalai.utils import get_current_device


class FDIntermTensors(metaclass=SingletonMeta):
    """Singleton class to hold tensors used for storing intermediate values in flash-decoding.
    For now, it holds intermediate output and logsumexp (which will be used in reduction step along kv)
    """

    def __init__(self):
        self._tensors_initialized = False

    def _reset(self):
        self._tensors_initialized = False
        del self._mid_output
        del self._mid_output_lse

    @property
    def is_initialized(self):
        return self._tensors_initialized

    @property
    def mid_output(self):
        assert self.is_initialized, "Intermediate tensors not initialized yet"
        return self._mid_output

    @property
    def mid_output_lse(self):
        assert self.is_initialized, "Intermediate tensors not initialized yet"
        return self._mid_output_lse

    def initialize(
        self,
        max_batch_size: int,
        num_attn_heads: int,
        kv_max_split_num: int,
        head_dim: int,
        dtype: torch.dtype = torch.float32,
        device: torch.device = get_current_device(),
    ) -> None:
        """Initialize tensors.
Repository: hpcaitech/ColossalAI
File: examples/inference/client/run_locust.sh (Shell)

Code before:
#!/bin/bash

#argument1: model_path

# launch server
model_path=$(realpath $1)
echo "Model Path: $model_path"
echo "Starting server..."
python -m colossalai.inference.server.api_server --model $model_path &
SERVER_PID=$!

# waiting time
sleep 60

# Run Locust
echo "Starting Locust..."
echo "The test will automatically begin, you can turn to http://0.0.0.0:8089 for more information."
locust -f locustfile.py -t 300 --tags online-generation --host http://127.0.0.1:8000 --autostart --users 100 --stop-timeout 10

# kill Server
echo "Stopping server..."
kill $SERVER_PID

echo "Test and server shutdown completely"


Review comment (suggestion):
You should give a default value.

```bash
model_path=${1:-"lmsys/vicuna-7b-v1.3"}
```

Code after:
#!/bin/bash

#argument1: model_path

# launch server
model_path=${1:-"lmsys/vicuna-7b-v1.3"}
echo "Model Path: $model_path"
echo "Starting server..."
python -m colossalai.inference.server.api_server --model $model_path &
SERVER_PID=$!

# waiting time
sleep 60

# Run Locust
echo "Starting Locust..."
echo "The test will automatically begin, you can turn to http://0.0.0.0:8089 for more information."
locust -f locustfile.py -t 300 --tags online-generation --host http://127.0.0.1:8000 --autostart --users 100 --stop-timeout 10

# kill Server
echo "Stopping server..."
kill $SERVER_PID

echo "Test and server shutdown completely"