-
Couldn't load subscription status.
- Fork 23
Connector Implementation & Inheritance Ollama #456
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
Connector Implementation & Inheritance Ollama #456
Conversation
|
#455 (comment) |
개요replacement of #455
백로그
커버해야하는 경계값은?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다! 먼저 로컬 머신에서 기동하는 부분을 테스트했습니다.
확인해보니 몇가지 상의할 부분이 있었습니다.
첫째로 Model의 경우 Ollama의 네이밍 컨벤션을 고려해 유효성 검사 로직이 필요할까요?
제 생각에는 차후에 문서에 잘 반영해두면 될 것 같은데 어떠실까요?
둘째로 테스트에서 커버할 경계값을 더 추가하면 좋겠습니다.
현재는 공백, null에 대해서 다루고 있습니다.
Model의 경우에는 위에서 유효성 검사 로직 추가여부에 따라 달라질 것 같고,
Uri를 잘못된 형식으로 전달한 경우도 테스트해도 좋을 것 같습니다.
그외에 테스트 컨벤션을 잘 반영해주셨습니다. 😎😎
값 앞뒤로 trailing 공백에 대한 부분은 settings.BaseUrl!.Trim()) == true 부분에서 공백이 없어지기에 추가하지 않는다고 판단해서 추가하지 않았습니다 baseurl에 url 포맷이 아닌경우에 대한 경계값은 EnsureLanguageModelSettingsValid 메서드는 단순히string.IsNullOrWhiteSpace만 체크하기에 GetChatClientAsync에서 경우에만 추가하였습니다! |
로컬 컨테이너 배포시
👉 그래서 해야할 일은
Azure 클라우드 배포시
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
테스트코드 잘 확인했습니다!
다른 부분을 본 다음 돌아와서 다시 전체적으로 확인해보시죠.
그전에 호스트 머신의 Ollama를 사용하기 위한 설정방법을 문서에 추가하고,
Azure 배포시 발생하는 오류를 해결해야합니다.
저도 방법을 찾아보고 코멘트 등으로 공유드리겠습니다.
말씀하신 내용 확인했습니다! 정리하자면
필요한 과정: 컨테이너 동시 실행 설정: 애플리케이션 컨테이너와 Ollama 컨테이너를 한 번에 실행하고 관리할 수 있도록 docker-compose.yml 같은 파일을 작성 모델 데이터 유지를 위한 볼륨 설정: Ollama가 모델 파일을 매번 새로 내려받지 않도록, 컨테이너 외부의 특정 경로와 컨테이너 내부의 모델 저장 경로를 볼륨으로 연결 컨테이너 간 통신 설정: 애플리케이션에서는 http://ollama:11434와 같이 컨테이너 이름을 사용해 Ollama 서비스에 접근하도록 설정 |
네 맞습니다~ 제 의견은 컨테이너화하고, Container Apps로 배포, 이후 서버리스 GPU를 사용하는 것입니다. |
|
제가 알고 있는 토대로 host.docker.internal:11434 방식으로 문서 추가했습니다. ollama 쪽 환경변수로 함은 $env:OLLAMA_HOST = "0.0.0.0:11434" 쪽을 말하시는것 같아서 이 부분 추가했는데 다른 부분일까요? |
네 이부분 말씀드린게 맞습니다. 현재 단계에서는 Ollama는 Azure 배포를 하지 않는데, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
다음 두가지 반영해주셔요!
1) bicep으로 배포시 오류나는 것 확인하고 수정방법 제안.
우리는 ollama를 사용하는데, 아래 구문에서 github models 관련 값이 null이어서 오류발생함! 값 유무를 보고 분기 처리해야할 것 같음.
https://github.com/aliencube/open-chat-playground/pull/456/files#diff-e800ed75064b740be458a572b91420b94ca6743a609c7318facf638f60c7028bL141
OpenAI쪽 이슈에서는 이렇게 처리했군요!
https://github.com/aliencube/open-chat-playground/pull/429/files#diff-e800ed75064b740be458a572b91420b94ca6743a609c7318facf638f60c7028bR150
2) Ollama baseurl도 처리
나중에 ollama를 Azure에서 구동할 예정이라서, 문서에 https://{{OLLAMA_PLACEHOLDER}}:11434 이런 느낌으로 연결 URL 넘겨주는 방법도 포함하고, bicep에도 해당 변수를 처리할 수 있도록 수정해주셔요.
|
리뷰 준비되면 댓글로 변경사항 간략히 알려주세요 전체적으로 잘 동작하는지 확인되면, |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ollama.md 스타일 컨벤션 몇개 놓친게 있어서 반영해주시고,
테스트 쪽은 내일 이어서 확인해볼게요.
먼저 이 PR로 반영된것 참고해주세요.
#494
|
|
@donghyeon639 네 우선 리뷰 요청 주실때까지 기다리고 있겠습니다! |
|
|
@tae0y CI/CD 파이프라인에서 Ollama 서버가 돌아가여 커넥터 부분 코드, 커넥터 테스트 부분 수정하였습니다! |
오 @donghyeon639 동현님 이 부분이 코드에서 어느 부분일까요? |
해당사항 CI/CD 파이프라인에서 Ollama 서버가 돌아가면서 제거하였습니다! |
|
넵넵 리뷰가 밀리면서 백로그에 있던게 다 머지가 되어있네요. |
테스트
|
test/OpenChat.PlaygroundApp.Tests/Connectors/OllamaConnectorTests.cs
Outdated
Show resolved
Hide resolved
test/OpenChat.PlaygroundApp.Tests/Connectors/OllamaConnectorTests.cs
Outdated
Show resolved
Hide resolved
test/OpenChat.PlaygroundApp.Tests/Connectors/OllamaConnectorTests.cs
Outdated
Show resolved
Hide resolved
test/OpenChat.PlaygroundApp.Tests/Connectors/OllamaConnectorTests.cs
Outdated
Show resolved
Hide resolved
test/OpenChat.PlaygroundApp.Tests/Connectors/OllamaConnectorTests.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
테스트 코드는 이렇게 수정해주셔요!
|
문서는 #515 Draft를 보시고 병합하거나 해서 반영해주세욥. |
#515 PR을 직접 https://github.com/donghyeon639/open-chat-playground/tree/feat/269-ollama-connector-implementation-clean 브랜치로 날려도 됩니다. 그러면 @donghyeon639 님이 바로 머지할 수 있어요. 굳이 두번 일 하지 않아도 됩니다. |
- 기본모델 이외에 추가모델을 3개에서 1개로 변경함 - 레포 루트경로 확인하는 단계 위치를 수정함 - Azure Serverless GPU 사용하는 단계 추가함
넵! 이 브랜치로 바로 push 했습니다. |
- 테스트 주입 데이터를 InlineData로 이동 - 테스트명에 테스트하는 메서드명을 기재 - CreateChatClientAsync에서 settings가 null인 경우 테스트 추가
- expected 예외타입 수정
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다! Ollama는 Huggingface, LG의 바탕이 되는 Connector이고,
비용없이 돌려볼 수 있어 사용자들이 가장 처음, 그리고 가장 많이 사용할 Connector라 생각해요.
저희 깃헙 액션에서도 Ollama를 활용해 UI 통합테스트를 진행하고 있습니다.
이 Connector 개발 과정에서 함께 Ollama 사용자 환경 설정 가이드에 대한 고민하고,
Ollama 관련 Docker Container를 어떤 방식으로 지원할지, Azure에 서빙하는 방법도 구상했습니다.
여러 단위/통합테스트 컨벤션과 사용자 입력값의 검증, 경계값 테스트까지 잘 반영했습니다!
긴 시간 고생많으셨어요. 머지합니다! 🥳🔥
|
머지후 test-deploy 실패했으나 Ollama 관련은 아님.
타임아웃의 원인은 잘 모르겠음. 일단 이력만 남겨놓음. |

Purpose
Does this introduce a breaking change?
Pull Request Type
What kind of change does this Pull Request introduce?
README updated?
The top-level readme for this repo contains a link to each sample in the repo. If you're adding a new sample did you update the readme?
How to Test
What to Check
Verify that the following are valid
Other Information
https://hub.docker.com/r/ollama/ollama