ronantakizawa/codereview-bench huggingface dataset:
Repository: LAION-AI/Open-Assistant
File: website/src/pages/api/chat/index.ts (TypeScript)

Code before:
import { withoutRole } from "src/lib/auth";
import { isChatEnable } from "src/lib/isChatEnable";
import { createInferenceClient } from "src/lib/oasst_inference_client";

export const PAGE_SIZE = 20;

const handler = withoutRole("banned", async (req, res, token) => {
  if (!isChatEnable()) {
    return res.status(404).end();
  }
  const client = createInferenceClient(token);

  let data;
  if (req.method === "GET") {
    if (req.query.chat_id) {
      data = await client.get_chat(req.query.chat_id as string);
    } else {
      const params = {
        limit: PAGE_SIZE,
      };
      if (req.query.before) {
        params["before"] = req.query.before;
      }
      if (req.query.after) {
        params["after"] = req.query.after;
      }
      data = await client.get_my_chats(params);
    }
  } else if (req.method === "POST") {
    data = await client.create_chat();
  } else if (req.method === "DELETE") {
    // TODO: re-activate later
    // await client.delete_chat(req.query.chat_id as string);
    data = {};
  } else if (req.method === "PUT") {
    await client.update_chat(req.body);
    return res.status(200).end();
  }

  if (data) {
    return res.status(200).json(data);
  }
  res.status(400).end();

Review comment (suggestion):
please add type `params: GetChatParams` just to be sure.

Otherwise looks good!

Code after:
import { withoutRole } from "src/lib/auth";
import { isChatEnable } from "src/lib/isChatEnable";
import { createInferenceClient } from "src/lib/oasst_inference_client";
import { GetChatsParams } from "src/types/Chat";

export const PAGE_SIZE = 20;

const handler = withoutRole("banned", async (req, res, token) => {
  if (!isChatEnable()) {
    return res.status(404).end();
  }
  const client = createInferenceClient(token);

  let data;
  if (req.method === "GET") {
    if (req.query.chat_id) {
      data = await client.get_chat(req.query.chat_id as string);
    } else {
      const params: GetChatsParams = {
        limit: PAGE_SIZE,
      };
      if (req.query.before) {
        params["before"] = req.query.before as string;
      }
      if (req.query.after) {
        params["after"] = req.query.after as string;
      }
      data = await client.get_my_chats(params);
    }
  } else if (req.method === "POST") {
    data = await client.create_chat();
  } else if (req.method === "DELETE") {
    // TODO: re-activate later
    // await client.delete_chat(req.query.chat_id as string);
    data = {};
  } else if (req.method === "PUT") {
    await client.update_chat(req.body);
    return res.status(200).end();
  }

  if (data) {
    return res.status(200).json(data);
  }
Repository: LAION-AI/Open-Assistant
File: website/src/pages/admin/edit/[id].tsx (TypeScript)

Code before:
                    onClick={handleSubmit}
                    isLoading={submitting}
                    loadingText={"Submitting"}
                  >
                    {t("common:submit")}
                  </Button>
                </Flex>
              </Stack>
            </>
          </TwoColumnsWithCards>
        </AdminArea>
      )}
    </>
  );
};

EditMessage.getLayout = AdminLayout;

export const getServerSideProps: GetServerSideProps<{ id: string }, { id: string }> = async ({
  locale = "en",
  params,
}) => ({
  props: {
    // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
    id: params!.id,
    ...(await serverSideTranslations(locale, ["common", "message", "labelling", "tasks"])),
  },
});

export default EditMessage;


Review comment (suggestion):
no need to define the individual namespaces here, `await serverSideTranslations(locale)` is good enough.

Code after:
                    isLoading={submitting}
                    loadingText={"Submitting"}
                  >
                    {t("common:submit")}
                  </Button>
                </Flex>
              </Stack>
            </>
          </TwoColumnsWithCards>
        </AdminArea>
      )}
    </>
  );
};

EditMessage.getLayout = AdminLayout;

export const getServerSideProps: GetServerSideProps<{ id: string }, { id: string }> = async ({
  locale = "en",
  params,
}) => ({
  props: {
    // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
    id: params!.id,
    ...(await serverSideTranslations(locale)),
  },
});

export default EditMessage;
Repository: LMCache/LMCache
File: benchmarks/long_doc_qa/long_doc_qa.py (Python)

Code before:
        if actual_latency_gain < args.expected_latency_gain:
            sys.exit(
                f"ERROR: latency gain {actual_latency_gain:.2f}× < expected "
                f"{args.expected_latency_gain:.2f}×"
            )


def create_argument_parser():
    parser = argparse.ArgumentParser(
        description="Benchmark the performance with or "
        "without automatic prefix caching."
    )

    parser.add_argument(
        "--document-length",
        type=int,
        # Roughly the number of tokens for a system paper,
        # excluding images
        default=20000,
        help="Length of each document in tokens.",
    )

    parser.add_argument(
        "--num-documents",
        type=int,
        default=20,
        help="Number of documents to generate for testing.",
    )

    parser.add_argument(
        "--output-len",
        type=int,
        default=100,
        help="Maximum number of tokens to generate for each prompt.",
    )

    parser.add_argument(
        "--repeat-count",
        type=int,
        default=2,
        help="Number of times to repeat each prompt",
    )

    parser.add_argument(
        "--repeat-mode",
        type=str,
        default="random",
        help="The mode to repeat prompts. The supported "
        'modes are "random", "tile", and "interleave". '
        "See repeat_prompts() in the source code for details.",
    )

Review comment (performance):
Could you please also add this field to `LMCache/.buildkite/configs/xx.yaml`? I know this is not ideal but currently the comprehensive tests use a lot of default values in the `long_doc_qa.py`.

Code after:
        if actual_latency_gain < args.expected_latency_gain:
            sys.exit(
                f"ERROR: latency gain {actual_latency_gain:.2f}× < expected "
                f"{args.expected_latency_gain:.2f}×"
            )


def create_argument_parser():
    parser = argparse.ArgumentParser(
        description="Benchmark the performance with or "
        "without automatic prefix caching."
    )

    parser.add_argument(
        "--document-length",
        type=int,
        # Roughly the number of tokens for a system paper,
        # excluding images
        default=20000,
        help="Length of each document in tokens.",
    )

    parser.add_argument(
        "--num-documents",
        type=int,
        default=8,
        help="Number of documents to generate for testing.",
    )

    parser.add_argument(
        "--output-len",
        type=int,
        default=100,
        help="Maximum number of tokens to generate for each prompt.",
    )

    parser.add_argument(
        "--repeat-count",
        type=int,
        default=2,
        help="Number of times to repeat each prompt",
    )

    parser.add_argument(
        "--repeat-mode",
        type=str,
        default="random",
        help="The mode to repeat prompts. The supported "
        'modes are "random", "tile", and "interleave". '
        "See repeat_prompts() in the source code for details.",
    )
Repository: LMCache/LMCache
File: lmcache/integration/vllm/vllm_adapter.py (Python)

Code before:
    )

# Third Party
from vllm.attention.backends.flashmla import FlashMLAMetadata
from vllm.attention.backends.mla.common import MLACommonMetadata
from vllm.config import (
    CacheConfig,
    ModelConfig,
    ParallelConfig,
    SchedulerConfig,
)
from vllm.sequence import IntermediateTensors
from vllm.utils import cdiv, round_down

# First Party
from lmcache.integration.vllm.utils import ENGINE_NAME
from lmcache.logging import init_logger
from lmcache.utils import _lmcache_nvtx_annotate
from lmcache.v1.cache_engine import LMCacheEngineBuilder

# FIXME(Jiayi): temporarily comment this out
# from lmcache_vllm.blend_adapter import remove_request_id_indices

logger = init_logger(__name__)

if torch.cuda.is_available():
    LMCACHE_CUDA_STREAM = torch.cuda.Stream()

SUPPORTED_BACKEND_METADATA = (
    FlashAttentionMetadata,
    FlashMLAMetadata,
    MLACommonMetadata,
)

VLLM_CACHE_CONFIG: Optional[CacheConfig] = None
VLLM_MODEL_CONFIG: Optional[ModelConfig] = None
VLLM_PARALLEL_CONFIG: Optional[ParallelConfig] = None
VLLM_SCHEDULER_CONFIG: Optional[SchedulerConfig] = None


class StoreStatus(Enum):
    PREFILL = auto()
    CHUNK_PREFILL = auto()
    DECODE = auto()
    SUFFIX_PREFILL = auto()
    NONE = auto()


class RetrieveStatus(Enum):
    PREFILL = auto()  # include (1) normal_prefill
    # (2) chunk_prefill_last

Review comment (suggestion):
Would be great if you make this device check by vLLM way:
```
from vllm.platforms import current_platform
if current_platform.is_cuda_alike():
```

Code after:
    )

# Third Party
from vllm.attention.backends.flashmla import FlashMLAMetadata
from vllm.attention.backends.mla.common import MLACommonMetadata
from vllm.config import (
    CacheConfig,
    ModelConfig,
    ParallelConfig,
    SchedulerConfig,
)
from vllm.platforms import current_platform
from vllm.sequence import IntermediateTensors

# First Party
from lmcache.integration.vllm.utils import ENGINE_NAME
from lmcache.logging import init_logger

# Use LMCache's own math utilities instead of vllm's
# (avoids dependency on vllm internal changes like https://github.com/vllm-project/vllm/pull/27188)
from lmcache.utils import _lmcache_nvtx_annotate, cdiv, round_down
from lmcache.v1.cache_engine import LMCacheEngineBuilder

# FIXME(Jiayi): temporarily comment this out
# from lmcache_vllm.blend_adapter import remove_request_id_indices

logger = init_logger(__name__)

if current_platform.is_cuda_alike():
    LMCACHE_CUDA_STREAM = torch.cuda.Stream()

SUPPORTED_BACKEND_METADATA = (
    FlashAttentionMetadata,
    FlashMLAMetadata,
    MLACommonMetadata,
)

VLLM_CACHE_CONFIG: Optional[CacheConfig] = None
VLLM_MODEL_CONFIG: Optional[ModelConfig] = None
VLLM_PARALLEL_CONFIG: Optional[ParallelConfig] = None
VLLM_SCHEDULER_CONFIG: Optional[SchedulerConfig] = None


class StoreStatus(Enum):
    PREFILL = auto()
    CHUNK_PREFILL = auto()
    DECODE = auto()
    SUFFIX_PREFILL = auto()
    NONE = auto()
Repository: LMCache/LMCache
File: tests/v1/storage_backend/test_storage_plugin.py (Python)

Code before:
# SPDX-License-Identifier: Apache-2.0
"""
Test cases for create_dynamic_backends.

This module tests the dynamic backend loading mechanism in CreateStorageBackends.
It creates a simple mock storage plugin extends StoragePluginInterface
and verifies that:
1. The backend is properly loaded via the configuration
2. The backend methods can be called through StorageManager
"""

# Standard
from typing import Any, List, Optional, Sequence
import asyncio

# Third Party
import pytest
import torch

# First Party
from lmcache.config import LMCacheEngineMetadata
from lmcache.utils import CacheEngineKey
from lmcache.v1.config import LMCacheEngineConfig
from lmcache.v1.event_manager import EventManager
from lmcache.v1.memory_management import (
    AdHocMemoryAllocator,
    MemoryFormat,
    MemoryObj,

Review comment (suggestion):
```suggestion
Test cases for  storage_plugin_launcher.
```

Code after:
# SPDX-License-Identifier: Apache-2.0
"""
Test cases for  storage_plugin_launcher.

This module tests the storage plugins loading mechanism in CreateStorageBackends.
It creates a simple mock storage plugin extends StoragePluginInterface
and verifies that:
1. The backend is properly loaded via the configuration
2. The backend methods can be called through StorageManager
"""

# Standard
from typing import Any, List, Optional, Sequence
import asyncio

# Third Party
import pytest
import torch

# First Party
from lmcache.config import LMCacheEngineMetadata
from lmcache.utils import CacheEngineKey
from lmcache.v1.config import LMCacheEngineConfig
from lmcache.v1.event_manager import EventManager
from lmcache.v1.memory_management import (
    AdHocMemoryAllocator,
    MemoryFormat,
    MemoryObj,
Repository: LMCache/LMCache
File: tests/v1/storage_backend/test_storage_plugin.py (Python)

Code before:
# SPDX-License-Identifier: Apache-2.0
"""
Test cases for create_dynamic_backends.

This module tests the dynamic backend loading mechanism in CreateStorageBackends.
It creates a simple mock storage plugin extends StoragePluginInterface
and verifies that:
1. The backend is properly loaded via the configuration
2. The backend methods can be called through StorageManager
"""

# Standard
from typing import Any, List, Optional, Sequence
import asyncio

# Third Party
import pytest
import torch

# First Party
from lmcache.config import LMCacheEngineMetadata
from lmcache.utils import CacheEngineKey
from lmcache.v1.config import LMCacheEngineConfig
from lmcache.v1.event_manager import EventManager
from lmcache.v1.memory_management import (
    AdHocMemoryAllocator,
    MemoryFormat,
    MemoryObj,
)
from lmcache.v1.storage_backend import CreateStorageBackends

Review comment (suggestion):
```suggestion
This module tests the storage plugins loading mechanism in CreateStorageBackends.
```

Code after:
# SPDX-License-Identifier: Apache-2.0
"""
Test cases for  storage_plugin_launcher.

This module tests the storage plugins loading mechanism in CreateStorageBackends.
It creates a simple mock storage plugin extends StoragePluginInterface
and verifies that:
1. The backend is properly loaded via the configuration
2. The backend methods can be called through StorageManager
"""

# Standard
from typing import Any, List, Optional, Sequence
import asyncio

# Third Party
import pytest
import torch

# First Party
from lmcache.config import LMCacheEngineMetadata
from lmcache.utils import CacheEngineKey
from lmcache.v1.config import LMCacheEngineConfig
from lmcache.v1.event_manager import EventManager
from lmcache.v1.memory_management import (
    AdHocMemoryAllocator,
    MemoryFormat,
    MemoryObj,
)
from lmcache.v1.storage_backend import CreateStorageBackends
Repository: LMCache/LMCache
File: lmcache/v1/storage_backend/connector/s3_connector.py (Python)

Code before:
            keys=keys,
            pin=pin,
            priority=Priorities.PEEK,
        )

    def support_batched_get_non_blocking(self) -> bool:
        return True

    async def _batched_get_non_blocking(
        self,
        lookup_id: str,
        keys: List[CacheEngineKey],
    ) -> List[MemoryObj]:
        # batched get is already a coroutine
        result = await self.batched_get(keys)
        return [r for r in result if r is not None]

    async def batched_get_non_blocking(
        self, lookup_id: str, keys: List[CacheEngineKey]
    ) -> List[MemoryObj]:
        return await self.pq_executor.submit_job(
            self._batched_get_non_blocking,
            lookup_id=lookup_id,
            keys=keys,
            priority=Priorities.PREFETCH,
        )

    async def list(self) -> List[str]:
        raise NotImplementedError

    def support_ping(self) -> bool:
        return False

    # TODO(Jiayi): This needs to be implemented.
    async def ping(self) -> int:
        raise NotImplementedError

    def support_batched_get(self) -> bool:
        return True

    async def close(self):
        await self.pq_executor.shutdown(wait=True)


Review comment (suggestion):
Dumb question.
Is this four of msg is the all the types of connection errors?
I can see the tons of error types in [here](https://docs.aws.amazon.com/AmazonS3/latest/API/ErrorResponses.html).

Code after:
    async def batched_get_non_blocking(
        self, lookup_id: str, keys: List[CacheEngineKey]
    ) -> List[MemoryObj]:
        return await self.pq_executor.submit_job(
            self._batched_get_non_blocking,
            lookup_id=lookup_id,
            keys=keys,
            priority=Priorities.PREFETCH,
        )

    async def list(self) -> List[str]:
        raise NotImplementedError

    def support_ping(self) -> bool:
        return False

    # TODO(Jiayi): This needs to be implemented.
    async def ping(self) -> int:
        raise NotImplementedError

    def support_batched_get(self) -> bool:
        return True

    def _update_connection_failures(self, error_msg: str) -> bool:
        # Check if it's a connection error
        is_connection_error = (
            "CONNECTION_REFUSED" in error_msg
            or "SOCKET" in error_msg
            or "DNS" in error_msg
            or "TIMEOUT" in error_msg
        )

        if is_connection_error:
            self.connection_failures += 1
            logger.error(
                f"S3 connection error ({self.connection_failures}/"
                f"{self.max_connection_failures}): {error_msg}"
            )

            if self.connection_failures >= self.max_connection_failures:
                self.connection_disabled = True
                logger.error(
                    f"S3 connection disabled after "
                    f"{self.max_connection_failures} "
                    f"consecutive failures. "
                    f"All future S3 operations will be skipped."
                )

        return is_connection_error

    def _reset_connection_failures(self):
Repository: LMCache/LMCache
File: lmcache/v1/health_monitor/constants.py (Python)

Code before:
# SPDX-License-Identifier: Apache-2.0
"""
Constants for health monitoring.
"""

# Ping error codes
PING_TIMEOUT_ERROR_CODE = -1
PING_GENERIC_ERROR_CODE = -2

# Configuration keys
PING_TIMEOUT_CONFIG_KEY = "ping_timeout"
PING_INTERVAL_CONFIG_KEY = "ping_interval"
GET_BLOCKING_FAILED_THRESHOLD_CONFIG_KEY = "get_blocking_failed_threshold"
WAITING_TIME_FOR_RECOVERY_CONFIG_KEY = "waiting_time_for_recovery"

# Default values
DEFAULT_PING_TIMEOUT = 5.0
DEFAULT_PING_INTERVAL = 30.0
DEFAULT_GET_BLOCKING_FAILED_THRESHOLD = 10
DEFAULT_WAITING_TIME_FOR_RECOVERY = 300.0

# Memory thresholds
DEFAULT_MEMORY_THRESHOLD_PERCENT = 95.0  # Unhealthy if memory usage > 95%


Review comment (suggestion):
It would be nice if you can update the `docs/source/production/observability/health_monitor.rst` with these two new added extra configs.

Code after:
# SPDX-License-Identifier: Apache-2.0
"""
Constants for health monitoring.
"""

# Standard
from enum import Enum


class FallbackPolicy(str, Enum):
    """Fallback policy when health check fails."""

    RECOMPUTE = "recompute"  # Skip all cache operations, fall back to recomputation
    LOCAL_CPU = "local_cpu"  # Fall back to local CPU backend only


# Ping error codes
PING_TIMEOUT_ERROR_CODE = -1
PING_GENERIC_ERROR_CODE = -2

# Configuration keys
PING_TIMEOUT_CONFIG_KEY = "ping_timeout"
PING_INTERVAL_CONFIG_KEY = "ping_interval"
FALLBACK_POLICY_CONFIG_KEY = "fallback_policy"
GET_BLOCKING_FAILED_THRESHOLD_CONFIG_KEY = "get_blocking_failed_threshold"
WAITING_TIME_FOR_RECOVERY_CONFIG_KEY = "waiting_time_for_recovery"

# Default values
DEFAULT_PING_TIMEOUT = 5.0
DEFAULT_PING_INTERVAL = 30.0
DEFAULT_FALLBACK_POLICY = FallbackPolicy.RECOMPUTE
DEFAULT_GET_BLOCKING_FAILED_THRESHOLD = 10
DEFAULT_WAITING_TIME_FOR_RECOVERY = 300.0

# Memory thresholds
DEFAULT_MEMORY_THRESHOLD_PERCENT = 95.0  # Unhealthy if memory usage > 95%
Repository: LadybirdBrowser/ladybird
File: Libraries/LibWeb/CookieStore/CookieChangeEvent.cpp (C++)

Code before:
GC::Ref<CookieChangeEvent> CookieChangeEvent::construct_impl(JS::Realm& realm, FlyString const& event_name, CookieChangeEventInit const& event_init)
{
    return create(realm, event_name, event_init);
}

CookieChangeEvent::CookieChangeEvent(JS::Realm& realm, FlyString const& event_name, CookieChangeEventInit const& event_init)
    : DOM::Event(realm, event_name, event_init)
    , m_changed(event_init.changed.value_or({}))
    , m_deleted(event_init.deleted.value_or({}))
{
}

CookieChangeEvent::~CookieChangeEvent() = default;

void CookieChangeEvent::initialize(JS::Realm& realm)
{
    WEB_SET_PROTOTYPE_FOR_INTERFACE(CookieChangeEvent);
    Base::initialize(realm);
}

void CookieChangeEvent::visit_edges(Cell::Visitor& visitor)
{
    Base::visit_edges(visitor);

    visitor.visit(m_cached_changed);
    visitor.visit(m_cached_deleted);
}

}


Review comment (performance):
Should we also add a `VISIT_CACHED_ATTRIBUTE` macro? Just seems a little off to have to know the exact name of the cached members here, but nowhere else. No strong feelings.

Code after:
GC::Ref<CookieChangeEvent> CookieChangeEvent::construct_impl(JS::Realm& realm, FlyString const& event_name, CookieChangeEventInit const& event_init)
{
    return create(realm, event_name, event_init);
}

CookieChangeEvent::CookieChangeEvent(JS::Realm& realm, FlyString const& event_name, CookieChangeEventInit const& event_init)
    : DOM::Event(realm, event_name, event_init)
    , m_changed(event_init.changed.value_or({}))
    , m_deleted(event_init.deleted.value_or({}))
{
}

CookieChangeEvent::~CookieChangeEvent() = default;

void CookieChangeEvent::initialize(JS::Realm& realm)
{
    WEB_SET_PROTOTYPE_FOR_INTERFACE(CookieChangeEvent);
    Base::initialize(realm);
}

void CookieChangeEvent::visit_edges(Cell::Visitor& visitor)
{
    Base::visit_edges(visitor);

    VISIT_CACHED_ATTRIBUTE(changed);
    VISIT_CACHED_ATTRIBUTE(deleted);
}

}
Repository: LadybirdBrowser/ladybird
File: Libraries/LibWeb/Layout/ImageBox.cpp (C++)

Code before:
            CSSPixels alt_text_width = 0;
            if (!m_cached_alt_text_width.has_value())
                m_cached_alt_text_width = CSSPixels::nearest_value_for(font->width(alt));
            alt_text_width = m_cached_alt_text_width.value();

            set_natural_width(alt_text_width + 16);
            set_natural_height(CSSPixels::nearest_value_for(font->pixel_size()) + 16);
        }
    }

    if (!has_natural_width() && !has_natural_height()) {
        // FIXME: Do something.
    }
}

void ImageBox::dom_node_did_update_alt_text(Badge<ImageProvider>)
{
    m_cached_alt_text_width = {};
}

bool ImageBox::renders_as_alt_text() const
{
    if (auto dom_node = this->dom_node()) {
        if (auto const* image_provider = as_if<ImageProvider>(*dom_node))
            return !image_provider->is_image_available();
    }
    return false;
}

GC::Ptr<Painting::Paintable> ImageBox::create_paintable() const
{
    return Painting::ImagePaintable::create(*this);
}

}


Review comment (suggestion):
Same here I think:
```c++
if (auto const* image_provider = as_if<ImageProvider>(dom_node().ptr()))
    return !image_provider->is_image_available();
```

Code after:
            CSSPixels alt_text_width = 0;
            if (!m_cached_alt_text_width.has_value())
                m_cached_alt_text_width = CSSPixels::nearest_value_for(font->width(alt));
            alt_text_width = m_cached_alt_text_width.value();

            set_natural_width(alt_text_width + 16);
            set_natural_height(CSSPixels::nearest_value_for(font->pixel_size()) + 16);
        }
    }

    if (!has_natural_width() && !has_natural_height()) {
        // FIXME: Do something.
    }
}

void ImageBox::dom_node_did_update_alt_text(Badge<ImageProvider>)
{
    m_cached_alt_text_width = {};
}

bool ImageBox::renders_as_alt_text() const
{
    if (auto const* image_provider = as_if<ImageProvider>(dom_node().ptr()))
        return !image_provider->is_image_available();

    return false;
}

GC::Ptr<Painting::Paintable> ImageBox::create_paintable() const
{
    return Painting::ImagePaintable::create(*this);
}

}