ronantakizawa/codereview-bench huggingface dataset:
Repository: LAION-AI/Open-Assistant
File: website/src/components/Messages/RenderedMarkdown.tsx (TypeScript)

Code before:
    borderBottomColor: "gray.400",
    _dark: {
      borderBottomColor: "gray.700",
    },
  },
  "table thead tr": {
    borderBottomColor: "gray.400",
    borderBottomWidth: "1px",
    _dark: {
      borderBottomColor: "gray.700",
    },
  },
  "ol li::marker": {
    content: "counter(list-item) '.'",
  },
};

const plugins = [remarkGfm];

const disallowedElements = ["img"];

// eslint-disable-next-line react/display-name
const RenderedMarkdown = ({ markdown }: RenderedMarkdownProps) => {
  const { isOpen, onOpen, onClose } = useDisclosure();
  const [link, setLink] = useState<string | undefined>();
  const [isCopied, setIsCopied] = useState(false);
  const toast = useToast();

  const components: ReactMarkdownOptions["components"] = useMemo(() => {
    return {
      // eslint-disable-next-line @typescript-eslint/no-unused-vars
      code({ node, inline, className, children, style, ...props }) {
        const match = /language-(\w+)/.exec(className || "");
        const lang = match ? match[1] : "";

        const handleCopyClick = async (event: MouseEvent) => {
          event.stopPropagation();
          toast.closeAll();

          try {
            await navigator.clipboard.writeText(String(children));

            toast({
              title: "Copied!",
              status: "success",
              duration: 1500,
              isClosable: true,
            });

            setIsCopied(true);
            setTimeout(() => setIsCopied(false), 1500);

Review comment (refactor):
This will cause a markdown containing multiple `code` blocks will show a copied state at the same time. You better extract the `code` to a separate component

Code after:
      borderInlineStartColor: "gray.500",
    },
  },
  "table tbody tr": {
    borderBottomColor: "gray.400",
    _dark: {
      borderBottomColor: "gray.700",
    },
  },
  "table thead tr": {
    borderBottomColor: "gray.400",
    borderBottomWidth: "1px",
    _dark: {
      borderBottomColor: "gray.700",
    },
  },
  "ol li::marker": {
    content: "counter(list-item) '.'",
  },
};

const plugins = [remarkGfm];

const disallowedElements = ["img"];

// eslint-disable-next-line react/display-name
const RenderedMarkdown = ({ markdown }: RenderedMarkdownProps) => {
  const { isOpen, onOpen, onClose } = useDisclosure();
  const [link, setLink] = useState<string | undefined>();

  const components: ReactMarkdownOptions["components"] = useMemo(() => {
    return {
      // eslint-disable-next-line @typescript-eslint/no-unused-vars
      code: RenderedCodeblock,
      a({ href, ...props }) {
        if (!href) {
          return props.children;
        }

        return (
          <NextLink // use NextLink to handle locale if link is our internal, but it's really edge case.
            {...props}
            href={href}
            target="_blank"
            rel="noopener noreferrer"
            onClick={(e: MouseEvent) => {
              e.preventDefault();
              setLink(href);
              onOpen();
            }}
          ></NextLink>
Repository: LAION-AI/Open-Assistant
File: website/src/components/Messages/MessageTableEntry.tsx (TypeScript)

Code before:
  onLabel: () => void;
  onReport: () => void;
  message: Message;
}) => {
  const toast = useToast();
  const { t } = useTranslation(["message", "common"]);
  const { id } = message;
  const { trigger: deleteMessage } = useSWRMutation(`/api/admin/messages`, () =>
    del(`/api/admin/delete_message/${id}`)
  );

  const { trigger: stopTree } = useSWRMutation(`/api/admin/stop_tree/${id}`, put, {
    onSuccess: () => {
      const displayId = id.slice(0, CHAR_COUNT) + "..." + id.slice(-CHAR_COUNT);
      toast({
        title: t("common:success"),
        description: t("tree_stopped", { id: displayId }),
        status: "success",
        duration: 5000,
        isClosable: true,
      });
    },
  });

  const handleDelete = async () => {
    await deleteMessage();
  };

  const handleStop = () => {
    stopTree();
  };

  const handleCopy = (text: string) => {
    navigator.clipboard.writeText(text);
    const displayId = text.slice(0, CHAR_COUNT) + "..." + text.slice(-CHAR_COUNT);

    toast({
      title: t("common:copied"),
      description: displayId,
      status: "info",
      duration: 5000,
      isClosable: true,
    });
  };

  const isAdminOrMod = useHasAnyRole(["admin", "moderator"]);
  const { locale } = useRouter();
  return (
    <Menu>
      <MenuButton>
        <MoreHorizontal />

Review comment (suggestion):
You need to call mutate to revalidate the data here. The key should be something like this `/api/messages/${id}/tree`

Code after:
  userEmoji: string[];
  onLabel: () => void;
  onReport: () => void;
  message: Message;
}) => {
  const toast = useToast();
  const { t } = useTranslation(["message", "common"]);
  const { id } = message;

  const { trigger: stopTree } = useSWRMutation(`/api/admin/stop_tree/${id}`, put, {
    onSuccess: () => {
      const displayId = id.slice(0, CHAR_COUNT) + "..." + id.slice(-CHAR_COUNT);
      toast({
        title: t("common:success"),
        description: t("tree_stopped", { id: displayId }),
        status: "success",
        duration: 5000,
        isClosable: true,
      });
    },
  });

  const { trigger: handleDelete } = useDeleteMessage(message.id);

  const handleStop = () => {
    stopTree();
  };

  const handleCopy = (text: string) => {
    navigator.clipboard.writeText(text);
    const displayId = text.slice(0, CHAR_COUNT) + "..." + text.slice(-CHAR_COUNT);

    toast({
      title: t("common:copied"),
      description: displayId,
      status: "info",
      duration: 5000,
      isClosable: true,
    });
  };

  const isAdminOrMod = useHasAnyRole(["admin", "moderator"]);
  const { locale } = useRouter();
  return (
    <Menu>
      <MenuButton>
        <MoreHorizontal />
      </MenuButton>
      <Portal>
        <MenuList>
          <MenuGroup title={t("reactions")}>
Repository: LAION-AI/Open-Assistant
File: website/src/components/Messages/BaseMessageEntry.tsx (TypeScript)

Code before:
import { Avatar, AvatarProps, Box, BoxProps, HStack, useBreakpointValue, useColorModeValue } from "@chakra-ui/react";
import { forwardRef, lazy, PropsWithChildren, Suspense, useMemo } from "react";

const RenderedMarkdown = lazy(() => import("./RenderedMarkdown"));

export type BaseMessageEntryProps = PropsWithChildren<{
  content: string;
  avatarProps: Pick<AvatarProps, "name" | "src">;
}> &
  BoxProps;

// eslint-disable-next-line react/display-name
export const BaseMessageEntry = forwardRef<HTMLDivElement, BaseMessageEntryProps>(
  ({ content, avatarProps, children, ...props }, ref) => {
    const inlineAvatar = useBreakpointValue({ base: true, md: false });
    const borderColor = useColorModeValue("blackAlpha.200", "whiteAlpha.200");
    const bg = useColorModeValue("#DFE8F1", "#42536B");

    const avatar = useMemo(
      () => (
        <Avatar
          borderColor={borderColor}
          size={inlineAvatar ? "xs" : "sm"}
          mr={inlineAvatar ? 2 : 0}
          mt={inlineAvatar ? 0 : `6px`}
          mb={inlineAvatar ? 1.5 : 0}
          {...avatarProps}
        />
      ),
      [avatarProps, borderColor, inlineAvatar]
    );
    return (
      <HStack
        ref={ref}
        w={["full", "full", "full", "fit-content"]}

Review comment (suggestion):
maybe this could be simplified to 
```tsx
BoxProps & {
  content: string;
  avatarProps: Pick<AvatarProps, "name" | "src">;
}
```

Code after:
import { Avatar, AvatarProps, Box, BoxProps, HStack, useBreakpointValue, useColorModeValue } from "@chakra-ui/react";
import { forwardRef, lazy, Suspense, useMemo } from "react";

const RenderedMarkdown = lazy(() => import("./RenderedMarkdown"));

export type BaseMessageEntryProps = BoxProps & {
  content: string;
  avatarProps: Pick<AvatarProps, "name" | "src">;
};

// eslint-disable-next-line react/display-name
export const BaseMessageEntry = forwardRef<HTMLDivElement, BaseMessageEntryProps>(
  ({ content, avatarProps, children, ...props }, ref) => {
    const inlineAvatar = useBreakpointValue({ base: true, md: false });
    const borderColor = useColorModeValue("blackAlpha.200", "whiteAlpha.200");
    const bg = useColorModeValue("#DFE8F1", "#42536B");

    const avatar = useMemo(
      () => (
        <Avatar
          borderColor={borderColor}
          size={inlineAvatar ? "xs" : "sm"}
          mr={inlineAvatar ? 2 : 0}
          mt={inlineAvatar ? 0 : `6px`}
          mb={inlineAvatar ? 1.5 : 0}
          {...avatarProps}
        />
      ),
      [avatarProps, borderColor, inlineAvatar]
    );
    return (
      <HStack
        ref={ref}
        w={["full", "full", "full", "fit-content"]}
        gap={0.5}
Repository: LAION-AI/Open-Assistant
File: website/src/components/Chat/ChatSection.tsx (TypeScript)

Code before:
  );
};

const sliderItems = [
  {
    key: "temperature",
  },
  {
    key: "max_new_tokens",
    max: 1000,
    step: 1,
    min: 1,
  },
  {
    key: "top_p",
  },
  {
    key: "repetition_penalty",
    min: 1,
    max: 2,
  },
  {
    key: "top_k",
    min: 1,
    max: 2,
  },
] as const;

const ChatConfigForm = ({
  modelInfos,
  control,
  register,
}: { modelInfos: ModelInfo[] } & StrictOmit<ChatConfigDrawerProps, "setValue">) => {
  const { t } = useTranslation("chat");
  return (
    <Stack gap="4">
      <FormControl>
        <FormLabel>{t("chat_config.model")}</FormLabel>
        <Select {...register("model_name")}>
          {modelInfos.map(({ name }) => (
            <option value={name} key={name}>
              {name}
            </option>
          ))}
        </Select>
      </FormControl>
      {sliderItems.map((item) => (
        <Controller
          name={item.key}
          key={item.key}
          control={control}

Review comment (bug):
I'm not sure about the range of these parameters. But for `repetition_penalty` and `top_k` it should be greater than 1 otherwise the inference backend will crash

Code after:
import { FormProvider, useForm } from "react-hook-form";
import { WorkParametersInput } from "src/types/Chat";

import { ChatConfigDrawer } from "./ChatConfigDrawer";
import { useChatContext } from "./ChatContext";
import { ChatConversation } from "./ChatConversation";

export const ChatSection = ({ chatId }: { chatId: string }) => {
  const { modelInfos } = useChatContext();
  const form = useForm<WorkParametersInput>({
    defaultValues: modelInfos[0].parameter_configs[0].work_parameters,
  });

  return (
    <FormProvider {...form}>
      <ChatConversation chatId={chatId} />
      <ChatConfigDrawer></ChatConfigDrawer>
    </FormProvider>
  );
};
Repository: LAION-AI/Open-Assistant
File: website/src/lib/oasst_inference_client.ts (TypeScript)

Code before:
import axios, { AxiosRequestConfig } from "axios";
import type { JWT } from "next-auth/jwt";
import { ChatItem, InferenceMessage, InferencePostMessageResponse } from "src/types/Chat";
import type { Readable } from "stream";

export class OasstInferenceClient {
  // this is not a long lived class, this is why the token is immutable
  constructor(private readonly inferenceAccessToken: string) {}

  async request<T = unknown>(path: string, init?: AxiosRequestConfig) {
    const { data } = await axios<T>(process.env.NEXT_PUBLIC_INFERENCE_SERVER_HOST + path, {
      ...init,
      headers: {
        ...init?.headers,
        Authorization: `Bearer ${this.inferenceAccessToken}`,
        "Content-Type": "application/json",
      },
    });
    return data;
  }

  get_my_chats(): Promise<ChatItem[]> {
    return this.request("/chats");
  }

  create_chat(): Promise<ChatItem> {
    return this.request("/chats", { method: "POST", data: "" });
  }

  get_chat(chat_id: string): Promise<ChatItem> {
    return this.request(`/chats/${chat_id}`);
  }

  get_message(chat_id: string, message_id: string): Promise<InferenceMessage> {
    return this.request(`/chats/${chat_id}/messages/${message_id}`);
  }

Review comment (suggestion):
NEXT_PUBLIC_INFERENCE_SERVER_HOST will be undefined in production. We need to pass it from getServerSideProps. Or we can use getInitialProps in the `_app.tsx` file and pass it to a context or any global variable.

Code after:
import axios, { AxiosRequestConfig } from "axios";
import { JWT } from "next-auth/jwt";
import {
  ChatItem,
  InferenceMessage,
  InferencePostMessageResponse,
  ModelInfo,
  WorkParametersInput,
} from "src/types/Chat";
import type { Readable } from "stream";

export class OasstInferenceClient {
  // this is not a long lived class, this is why the token is immutable
  constructor(private readonly inferenceAccessToken: string) {}

  async request<T = unknown>(path: string, init?: AxiosRequestConfig) {
    const { data } = await axios<T>(process.env.INFERENCE_SERVER_HOST + path, {
      ...init,
      headers: {
        ...init?.headers,
        Authorization: `Bearer ${this.inferenceAccessToken}`,
        "Content-Type": "application/json",
      },
    });
    return data;
  }

  get_my_chats(): Promise<ChatItem[]> {
    return this.request("/chats");
  }

  create_chat(): Promise<ChatItem> {
    return this.request("/chats", { method: "POST", data: "" });
  }

  get_chat(chat_id: string): Promise<ChatItem> {
Repository: LAION-AI/Open-Assistant
File: inference/worker/get_model_config_prop.py (Python)

Code before:
import sys

from oasst_shared import model_configs
from settings import settings

if __name__ == "__main__":
    model_config = model_configs.MODEL_CONFIGS.get(settings.model_config_name)
    if model_config is None:
        print(f"Unknown model config name: {settings.model_config_name}")
        sys.exit(2)
    prop = sys.argv[1]
    if not hasattr(model_config, prop):
        print(f"Unknown property: {prop}")
        sys.exit(3)
    val = getattr(model_config, prop)
    if isinstance(val, bool):
        val = str(val).lower()
    print(val, end="")


Review comment (suggestion):
maybe add `len(sys.argv) > 1` check (since everything else is here handled gracefully)

Code after:
import sys

from oasst_shared import model_configs
from settings import settings

if __name__ == "__main__":
    model_config = model_configs.MODEL_CONFIGS.get(settings.model_config_name)
    if model_config is None:
        print(f"Unknown model config name: {settings.model_config_name}")
        sys.exit(2)
    if len(sys.argv) != 2:
        print("Usage: get_model_config_prop.py <property>")
        sys.exit(2)
    prop = sys.argv[1]
    if not hasattr(model_config, prop):
        print(f"Unknown property: {prop}")
        sys.exit(2)
    val = getattr(model_config, prop)
    if isinstance(val, bool):
        val = str(val).lower()
    print(val, end="")
Repository: LAION-AI/Open-Assistant
File: model/model_training/custom_datasets/extra_rm_datasets.py (Python)

Code before:
from model_training.custom_datasets.rank_datasets import AnthropicRLHF, HFDataset


def load_anthroopic_rlhf():
    train = AnthropicRLHF(split="train")
    validation = AnthropicRLHF(split="test")

    print(f"AnthropicRLHF extra dataset size: {len(train)}, {len(validation)}")

    return train, validation


def load_open_ai_summarize_from_feedback():
    axis_test = HFDataset(split="test", subset="axis")
    axis_validation = HFDataset(split="validation", subset="axis")

    comp_train = HFDataset(split="train", subset="comparisons")
    comp_validation = HFDataset(split="validation", subset="comparisons")

    train = comp_train + axis_test
    validation = comp_validation + axis_validation

    print(f"HFDataset extra dataset size: {len(train)}, {len(validation)}")

    return train, validation


Review comment (suggestion):
typo:
```suggestion
def load_anthropic_rlhf():
```

Code after:
from model_training.custom_datasets.rank_datasets import AnthropicRLHF, HFDataset


def load_anthropic_rlhf():
    train = AnthropicRLHF(split="train")
    validation = AnthropicRLHF(split="test")

    print(f"AnthropicRLHF extra dataset size: {len(train)}, {len(validation)}")

    return train, validation


def load_open_ai_summarize_from_feedback():
    axis_test = HFDataset(split="test", subset="axis")
    axis_validation = HFDataset(split="validation", subset="axis")

    comp_train = HFDataset(split="train", subset="comparisons")
    comp_validation = HFDataset(split="validation", subset="comparisons")

    train = comp_train + axis_test
    validation = comp_validation + axis_validation

    print(f"HFDataset extra dataset size: {len(train)}, {len(validation)}")

    return train, validation
Repository: LAION-AI/Open-Assistant
File: website/src/pages/api/chat/assistant_message.ts (TypeScript)

Code before:
import { boolean } from "boolean";
import { withoutRole } from "src/lib/auth";
import { createInferenceClient } from "src/lib/oasst_inference_client";
import { InferencePostAssistantMessageParams } from "src/types/Chat";

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

  const data = await client.post_assistant_message(req.body as InferencePostAssistantMessageParams);

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

export default handler;


Review comment (suggestion):
this line is repeated too many times, maybe we can re-add the `isChatEnabled` but we can use the new `getEnv()` in it

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

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

  const data = await client.post_assistant_message(req.body as InferencePostAssistantMessageParams);

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

export default handler;
Repository: LAION-AI/Open-Assistant
File: oasst-shared/oasst_shared/schemas/inference.py (Python)

Code before:
    timeout = "timeout"


class MessageRead(pydantic.BaseModel):
    id: str
    parent_id: str | None
    content: str | None
    created_at: datetime
    role: Literal["prompter", "assistant"]
    state: MessageState
    score: int
    reports: list[Report] = []
    # work parameters will be None on user prompts
    work_parameters: WorkParameters | None

    @property
    def is_assistant(self) -> bool:
        return self.role == "assistant"


class Thread(pydantic.BaseModel):
    messages: list[MessageRead]


class SafetyParameters(pydantic.BaseModel):
    enabled: bool = True


class SafetyRequest(pydantic.BaseModel):
    inputs: str
    parameters: SafetyParameters


class SafetyResponse(pydantic.BaseModel):
    outputs: str


class WorkerRequestBase(pydantic.BaseModel):
    id: str = pydantic.Field(default_factory=lambda: str(uuid.uuid4()))


class WorkRequest(WorkerRequestBase):
    request_type: Literal["work"] = "work"
    thread: Thread = pydantic.Field(..., repr=False)
    created_at: datetime = pydantic.Field(default_factory=datetime.utcnow)
    parameters: WorkParameters = pydantic.Field(default_factory=WorkParameters)
    safety_parameters: SafetyParameters = pydantic.Field(default_factory=SafetyParameters)


class PingRequest(WorkerRequestBase):
    request_type: Literal["ping"] = "ping"

Review comment (suggestion):
would set to `False` by default bc if this gets merged & released without the safety server being up yet, we want it to be "backwards compatible"

Code after:
    timeout = "timeout"


class MessageRead(pydantic.BaseModel):
    id: str
    parent_id: str | None
    content: str | None
    created_at: datetime
    role: Literal["prompter", "assistant"]
    state: MessageState
    score: int
    reports: list[Report] = []
    # work parameters will be None on user prompts
    work_parameters: WorkParameters | None

    @property
    def is_assistant(self) -> bool:
        return self.role == "assistant"


class Thread(pydantic.BaseModel):
    messages: list[MessageRead]


class SafetyParameters(pydantic.BaseModel):
    level: int = 0

    @pydantic.validator("level")
    def level_must_be_in_range(cls, v):
        if v < 0 or v > 9:
            raise ValueError("level must be in range [0, 9]")
        return v


class SafetyRequest(pydantic.BaseModel):
    inputs: str
    parameters: SafetyParameters


class SafetyResponse(pydantic.BaseModel):
    outputs: str


class WorkerRequestBase(pydantic.BaseModel):
    id: str = pydantic.Field(default_factory=lambda: str(uuid.uuid4()))


class WorkRequest(WorkerRequestBase):
    request_type: Literal["work"] = "work"
    thread: Thread = pydantic.Field(..., repr=False)
    created_at: datetime = pydantic.Field(default_factory=datetime.utcnow)
Repository: LAION-AI/Open-Assistant
File: inference/server/oasst_inference_server/admin.py (Python)

Code before:
from loguru import logger
from oasst_inference_server import database, models
from oasst_shared import utils as shared_utils


async def delete_user_from_db(session: database.AsyncSession, user_id: str) -> bool:
    """Deletes a user."""
    logger.info(f"Deleting user {user_id}")
    user = await session.get(models.DbUser, user_id)
    if not user:
        return False
    user.deleted = True

    # Anonymise user data
    user.display_name = shared_utils.DELETED_USER_DISPLAY_NAME
    # Ensure uniqueness
    user.provider_account_id = f"{shared_utils.DELETED_USER_ID_PREFIX}{user.id}"

    await session.commit()
    return True


Review comment (bug):
I think we should just raise an exception (404) on fail here instead of returning `bool`, that's more in line with the rest of the codebase

Code after:
import fastapi
from loguru import logger
from oasst_inference_server import database, models
from oasst_shared import utils as shared_utils


async def delete_user_from_db(session: database.AsyncSession, user_id: str):
    """Deletes a user."""
    logger.info(f"Deleting user {user_id}")
    user = await session.get(models.DbUser, user_id)
    if not user:
        raise fastapi.HTTPException(
            status_code=fastapi.status.HTTP_404_NOT_FOUND,
            detail="User not found",
        )
    user.deleted = True

    # Anonymise user data
    user.display_name = shared_utils.DELETED_USER_DISPLAY_NAME
    # Ensure uniqueness
    user.provider_account_id = f"{shared_utils.DELETED_USER_ID_PREFIX}{user.id}"

    await session.commit()
Repository: LAION-AI/Open-Assistant
File: website/src/components/Chat/ChatConfigSaver.tsx (TypeScript)

Code before:
import { useEffect } from "react";
import { useFormContext } from "react-hook-form";
import { ChatConfigFormData } from "src/types/Chat";
import { setConfigCache } from "src/utils/chat";

export const ChatConfigSaver = () => {
  const { watch } = useFormContext<ChatConfigFormData>();
  const config = watch();
  useEffect(() => {
    setConfigCache(config);
  }, [config]);

  return null;
};


Review comment (suggestion):
when we change presets, we use the function `reset`, which sets the `isDirty` to false, so we are not saving.

Code after:
import { useEffect } from "react";
import { useFormContext } from "react-hook-form";
import { ChatConfigFormData } from "src/types/Chat";
import { setConfigCache } from "src/utils/chat";

export const ChatConfigSaver = () => {
  const { watch, formState } = useFormContext<ChatConfigFormData>();
  const config = watch();
  useEffect(() => {
    if (formState.isDirty) {
      setConfigCache(config);
    }
  }, [config, formState.isDirty]);

  return null;
};