furiosa-ai / device-api

APIs that offers NPU devices' information and allow to control the devices
Apache License 2.0
5 stars 8 forks source link

Add python API (pyo3) #68

Closed yw-furiosa closed 1 year ago

yw-furiosa commented 1 year ago

Add python API according to the official documentation.

hyunsik commented 1 year ago

저도 API를 사용해보고 내일 까지 리뷰 코멘트 드리겠습니다.

yw-furiosa commented 1 year ago

네 @hyunsik 님, 빠르게 반영하겠습니다. 그런데 그 중에 몇가지 확인받고싶은 것이 있습니다.

hyunsik commented 1 year ago

@yw-furiosa 님 안녕하세요. 감사합니다.

integration test가 정확히 어떤 것을 말씀하시는지 궁금해서요, 참고할만한게 있다면 알려주시면 감사하겠습니다! 제가 이해하기론 각 기능별 세부적인 동작은 기존 device-api안에 있으므로 python쪽에서 모듈을 잘 improt할 수 있고 몇몇 함수들이 에러 없이 동작하는지 검증하는 것으로 이해했는데요(그리고 root 디렉토리에서 cargo test 혹은 make test 같은 명령어로 모두 한번에 테스트), 혹시 맞으실까요?

네 맞습니다. 사용자가 end user api로 실제 장비에서 동작하는 end-to-end 예제를 작성해주시면 그 예제를 integration 테스트 겸 예제로 사용할 수 있을 것 같습니다. 우선 자동화된 CI 추가는 시간을 꽤 소모할 수 있으니 이 PR 범위에 넣지 않아도 좋을 것 같습니다. python 파일이 실행되는 것이니 make test가 적절할 것 같습니다. 우선은 장비가 있는 머신에서 잘 실행되도록 작성해주시면 나중에 CI 를 붙이는 작업은 별도로 할 수 있을 것 같습니다.

마지막에 말씀해주신 세가지 함수 말고도 async로 동작하는 함수들이 몇몇 있는데요 (ex. Fetcher, Device), 이런 함수들의 sync버전도 구현해서 포함시킬까요?

네 맞습니다. 제가 놓치고 있던 부분인데요. 해당 부분까지 부탁드리겠습니다.

yw-furiosa commented 1 year ago

@hyunsik 님, 간단하게 example과 tests 추가해봤습니다. 그런데 example/monitoring.py 코드에서 매 초 heartbeat 값을 가져와서 함께 출력하고 있는데 기존 device-api 코드 상, heartbeat 값이 DeviceInfo 구조체가 생성될 때 한번만 읽어오기 때문에 갱신되지 않는 문제가 있습니다. 기존 DeviceInfo에서의 heartbeat값을 원래 갱신할 의도가 없다면 monitoring.py에서도 heartbeat는 빼겠습니다.

그리고 리뷰해주신 나머지 부분도 내일 중으로 모두 추가하겠습니다.

hyunsik commented 1 year ago

@yw-furiosa 빠른 진행 감사드립니다. 고생 많으셨습니다~

@libc-furiosa 님 혹시 heartbeat도 fetcher 같은 접근을 사용해야 하는 것은 아닐까요? 의견 주시면 감사하겠습니다.

libc-furiosa commented 1 year ago

heartbeat은 uptime과 유사한 용도로 사용됩니다. 읽을 때 마다 다른 값이 나와야합니다만 현재 코드 구조에서는 read once가 적용되어 있습니다.

yw-furiosa commented 1 year ago

네 알겠습니다. 이번 pr에서는 일단 해당 작업은 제외하고 진행하겠습니다.

yw-furiosa commented 1 year ago

@furiosamg 님 @hyunsik 님 리뷰 모두 반영했습니다

furiosamg commented 1 year ago

@furiosamg 님 @hyunsik 님 리뷰 모두 반영했습니다

제가 레포에 권한이 부족해서인지 review comment를 resolve할 수가 없네요.. 수고하셨습니다

hyunsik commented 1 year ago

@yw-furiosa 네 감사합니다! 저도 내일까지 리뷰 보고 코멘트 드리겠습니다.

@libc-furiosa 답변 주셔서 감사합니다.

@furiosamg 님 제가 writer 권한을 드렸는데요. 혹시 resolve 가 되는지 한번 확인 부탁드리겠습니다.

furiosamg commented 1 year ago

@furiosamg 님 제가 writer 권한을 드렸는데요. 혹시 resolve 가 되는지 한번 확인 부탁드리겠습니다.

네 이제 잘 됩니다 감사합니다~

yw-furiosa commented 1 year ago

@hyunsik 님, 말씀해주신 대로 리뷰 반영해봤습니다!

hyunsik commented 1 year ago

네 정말 고생 많으셨습니다. 머지하겠습니다!