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

Documentation for device-api #19

Closed sukyoungjeong-furiosa closed 2 years ago

sukyoungjeong-furiosa commented 2 years ago

2

sukyoungjeong-furiosa commented 2 years ago

furiosa-device crate이 볼륨이 크지는 않으나, documentation의 경우 퍼블릭하게 노출되고 쉽게 browse 될 수 있는 만큼 충분히 명확하게 작성하는 것을 목적으로 하려하는데요. 우선은 1차로 doc을 간단하게 작성하였고 의견을 부탁드리겠습니다. (@hyunsik @libc-furiosa)

저는 크게 세가지 부분을 신경쓰려 합니다. (1) top level crate documentation에 dependency 및 usage example 등. (2) struct Device 하위에 NPU의 Mode에 관하여 간단하게 기술 (Single, Fusion, MultiCore) (3) DeviceConfig 하위에 builder examples

이를 제외하면 나머지 부분은 어느정도 self-describing 하고 있어서 짧게 기술해도 괜찮을 것이라고 생각 됩니다.

(*리뷰에 blocking feature에 대한 doc은 포함하지 않았습니다.)

sukyoungjeong-furiosa commented 2 years ago

To render docs in local, please use a command as below: cargo rustdoc --lib --all-features -- --cfg docsrs

*Note: It requires nightly. (which is for using feature(doc_cfg), in order to display feature flags. ) (And docs.rs uses nightly)

I referred tokio for documenting feature dependent items (currently, module blocking only).

hyunsik commented 2 years ago

리뷰가 조금 늦었습니다. 제가 내일 까지 코멘트 드릴께요.

sukyoungjeong-furiosa commented 2 years ago

네, 지금 버전으로 리뷰를 봐주시면 될것 같습니다. 감사합니다.

visibility 관련해서 고민이 좀 있는데 대표적으로 WarboyConfigBuilder 같은 것인데요. 지금 구현처럼 굳이 builder를 reexport하지 않아도 사용성에 전혀 무리가 없다고 생각 합니다. 그런데 렌더링된 문서에 docstring이 이 붙지가 않아서 고민이네요. 일단 그대로 두었습니다.

hyunsik commented 2 years ago

고생 많으셨네요. 제 생각에 전체적으로 좋은 듯 합니다. 그래서 세부 내용에 대해 의견 드려봅니다.

sukyoungjeong-furiosa commented 2 years ago

꼼꼼히 봐주셔서 감사합니다. 리뷰 주신 부분을 반영하였습니다.

documentation에서 렌더링해서 보니 신경쓰여서 #20 에서 코멘트 주신 내용을 여기에서 반영했습니다. (blocking::get_device_status를 private으로 숨겼습니다.)

get_device_status() 는 pub 일 필요 없습니다. private 으로 변경해도 될 것 같네요.

hyunsik commented 2 years ago

WarboyConfigBuilder 에 대한 제 의견을 말씀드리면 public 으로 노출하고 re-export 해도 좋을 것 같습니다. 레퍼런스가 없으면 fused, count 등 API 가 노출이 안되어 조금 불편할 것 같습니다. 사용자가 직접 정의해서 사용할 타입은 아니겠지만 문서에 더 노출되어 나쁠 것은 없을 것 같네요. 의견 주시면 감사하겠습니다.

sukyoungjeong-furiosa commented 2 years ago

네, 문서 제안 주신 부분을 우선 반영하였습니다. WarboyConfigBuilder에 대한 의견도 감사드립니다. 해당부분도 추가 하겠습니다.

수정 제안 주신부분 포함하여 전체적으로 저도 한번 더 읽어보고, 민석님께도 영어 검수를 요청드려보겠습니다.

sukyoungjeong-furiosa commented 2 years ago

영어 검수를 요청드렸는데 피드백이 있으면 별도 PR로 반영하고 이 PR은 마무리를 하려 합니다.