Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add basic OpenAI model implemenation #26

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

Marlangcow
Copy link
Contributor

No description provided.

@Marlangcow Marlangcow self-assigned this Jan 10, 2025
@h-albert-lee h-albert-lee linked an issue Jan 11, 2025 that may be closed by this pull request
@h-albert-lee
Copy link
Member

h-albert-lee commented Jan 11, 2025

@Marlangcow

  1. 매개 변수(temperature) 하드 코딩 하지 마세요. (-> default값을 이미 넣어두는 것으로 대응)

  2. API 파라미터들(max_tokens, top_p, frequency_pernalty, 등)은 왜 빠져있나요

  3. return_logits 활성 여부 추가(openai가 아니라, openai compatible server이기 때문에 관련 파라미터 추가. 또한 completion api와 chat completion에서의 차이가 존재)

  4. In-Place 수정 : 원본 inputs 딕셔너리를 그대로 수정하면 안됩니다

  5. 에러 처리 방식 문제: 재시도 로직이 없고, raise 옵션이 없습니다. 또한 outputs.append(item)하고 실패할 때 except 문으로 넘어가면 당연히 인덱싱 에러 납니다. (input 개수랑 output 개수가 달라지니까요)

  6. Completion api와 Chat Completion api의 차이를 인지하고 작업이 되어야 할 필요성이 있습니다. (Chat
    Completion은 role이 존재)

  7. api 설정을 전역으로 오버라이드되어있는데, 이런 경우엔 multimodel 충돌납니다.

수정 부탁드립니다~!

simple-eval이나 lm-eval-harness의 코드 베이스를 참고해볼 필요가 있어보입니다

@h-albert-lee
Copy link
Member

아 requirements.txt에도 dependency 추가하셔야 합니다!

Copy link
Member

@h-albert-lee h-albert-lee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수정 후 다시 request 부탁드려요~

@Marlangcow
Copy link
Contributor Author

@h-albert-lee lm_eval/models/api_models.py랑 simple-eval 토대로 말씀주신 피드백 8가지 수정했습니다. 다시한번 리뷰 요청드립니다!

@h-albert-lee
Copy link
Member

예이~~ 감사합니다 확인해보겠슴당

@h-albert-lee
Copy link
Member

@Marlangcow 고생많으셨습니다! 다만 몇가지 Minor 한 사항이 있어 말씀 드립니다

  1. from typing import Union이 빠진 것 같습니다
  2. chat_completion, completion의 분기는 arg로 받는게 맞는 것 같습니다(use_chat_api= True)
  3. (option)Error인 경우에도 prediction에 None이라도 들어가는 게 나을 것 같긴 한데, 나은님 판단대로 하시면 좋을 것 같습니다.
  4. (option) outputs.append(**item, **(result or {"error": "Failed to generate"}))
    는 키워드 인자를 전달하게 되어 TypeError가 발생할 수 있습니다만, 현재 동작엔 무리가 없을 것 같습니다. 수정하고싶으면 수정하시면 됩니다.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

add openai compatible api backend
2 participants