add test ci #4

jongchanpark-furiosa · 2024-08-16T07:30:53Z

One line PR Description

This is PR for adding test ci of furiosa-smi-go

What type of PR is this?

/kind devops

Special notes for reviewer

Wrong Mockdevice is updated

.github/workflows/golang.yml

jongchanpark-furiosa · 2024-08-19T07:55:59Z

2890ad5 는 외부에 mock 관련 함수들 노출하지 않도록 변경 & getStaticMockDevices 를 사용하도록 link type TC를 업데이트 했습니다.

jongchanpark-furiosa · 2024-08-19T08:18:32Z

cc @bg-furiosa @hoony9x-furiosa-ai

몇몇 레포들이 squash 혹은 rebase merge 만 가능하도록 변경된것 같은데, 혹시 create merge commit 옵션을 못하도록 변경된 이유가 있을까요?

hoony9x-furiosa-ai · 2024-08-19T08:19:40Z

cc @bg-furiosa @hoony9x-furiosa-ai

몇몇 레포들이 squash 혹은 rebase merge 만 가능하도록 변경된것 같은데, 혹시 create merge commit 옵션을 못하도록 변경된 이유가 있을까요?

아 Repo 설정하다가 잘못 끈거 같네요. 일단 다시 돌려놓겠습니다.

bg-furiosa · 2024-08-21T01:53:56Z

.github/workflows/golang.yml

@@ -38,4 +38,4 @@ jobs:
        with:
          version: v1.55.0
      - name: build and test
-        run: make check
+        run: make all


아래 golang.yml을 참고해서 build, test, lint를 별도의 step으로 분리 부탁드립니다.
문제 발생시 디버깅을 쉽게 하기 위함입니다, 필요에 따라 Makefile도 업데이트 부탁드립니다.

https://github.com/furiosa-ai/furiosa-device-plugin/blob/main/.github/workflows/golang.yaml

https://github.com/furiosa-ai/libfuriosa-kubernetes/blob/main/.github/workflows/golang.yml

781e7f4 에서 수정하였습니다.

bg-furiosa · 2024-08-21T02:01:36Z

pkg/smi/mock_common.go

 }

-func GetStaticMockDevices(arch Arch) (mockDevices []Device) {
+func getStaticMockDevices(arch Arch) (mockDevices []Device) {


GetStaticMockDevice/s함수는 smi-go의 테스트에도 사용되지만 본래 의도는 smi에 대한 mock제공입니다. 그래서 pkg외부에서 접근가능해야할거같습니다.

아래는 Mock이 사용되는 곳들에 대한 링크입니다.

https://github.com/furiosa-ai/libfuriosa-kubernetes/blob/main/pkg/npu_allocator/score_based_optimal_allocator_test.go

https://github.com/furiosa-ai/furiosa-device-plugin/blob/main/internal/device_manager/exclusive_device_test.go

https://github.com/furiosa-ai/furiosa-device-plugin/blob/main/internal/device_manager/manager_test.go

442373d 에서 수정하였습니다.

bg-furiosa · 2024-08-21T02:04:56Z

pkg/smi/mock_common.go

+	1: {0: LinkTypeHostBridge, 1: LinkTypeNoc, 2: LinkTypeCpu, 3: LinkTypeCpu, 4: LinkTypeInterconnect, 5: LinkTypeInterconnect, 6: LinkTypeInterconnect, 7: LinkTypeInterconnect},
+	2: {0: LinkTypeCpu, 1: LinkTypeCpu, 2: LinkTypeNoc, 3: LinkTypeHostBridge, 4: LinkTypeInterconnect, 5: LinkTypeInterconnect, 6: LinkTypeInterconnect, 7: LinkTypeInterconnect},
+	3: {0: LinkTypeCpu, 1: LinkTypeCpu, 2: LinkTypeHostBridge, 3: LinkTypeNoc, 4: LinkTypeInterconnect, 5: LinkTypeInterconnect, 6: LinkTypeInterconnect, 7: LinkTypeInterconnect},
+	4: {0: LinkTypeInterconnect, 1: LinkTypeInterconnect, 2: LinkTypeInterconnect, 3: LinkTypeInterconnect, 4: LinkTypeNoc, 5: LinkTypeHostBridge, 6: LinkTypeCpu, 7: LinkTypeCpu},
+	5: {0: LinkTypeInterconnect, 1: LinkTypeInterconnect, 2: LinkTypeInterconnect, 3: LinkTypeInterconnect, 4: LinkTypeHostBridge, 5: LinkTypeNoc, 6: LinkTypeCpu, 7: LinkTypeCpu},
+	6: {0: LinkTypeInterconnect, 1: LinkTypeInterconnect, 2: LinkTypeInterconnect, 3: LinkTypeInterconnect, 4: LinkTypeCpu, 5: LinkTypeCpu, 6: LinkTypeNoc, 7: LinkTypeHostBridge},
+	7: {0: LinkTypeInterconnect, 1: LinkTypeInterconnect, 2: LinkTypeInterconnect, 3: LinkTypeInterconnect, 4: LinkTypeCpu, 5: LinkTypeCpu, 6: LinkTypeHostBridge, 7: LinkTypeNoc},


smi경우 hintmap접근시 내부적으로 index정렬을 하여 항상 적은 값을 가진 value를 key로 사용하도록 구현되어 있는대요.
그래서 절반만 생성하는대 혹시 이를 전체를 생성하도록 변경하신 이유가 있을까요?

hintmap을 GetDeviceToDeviceLinkType 의 테스트에서 사용하는데, 두가지 이유로 변경했습니다.

smi 에서는 npu0,1의 순서에 상관없이 같은 결과가 나와야하므로, hintmap 에서도 hintmap[0][1] 이나 hintmap[1][0] 두가지 모두 같은 결과값을 출력하기 위해서

hintmap 을 사용하여 코드를 작성할때, index 정렬을 신경쓰지 않아도 되도록 바꾸기 위해서

smi 에서는 npu0,1의 순서에 상관없이 같은 결과가 나와야하므로, hintmap 에서도 hintmap[0][1] 이나 hintmap[1][0] 두가지 모두 같은 결과값을 출력하기 위해서

음 잘 이해가 안되는대요.
Get**LinkType()의 경우 두개 디바이스간의 link를 반환하는 API입니다.
여기서 Device는 각각의 npu index(npu{idx}pe)를 가지고 있습니다.
Device가 Mock Device이더라도 index는 존재하고(0 또는 지정된값) 최종적인 API시나리오는 두가지 입니다.

같은 index와의 비교 => Noc link반환

다른 index와의 비교 => index 정렳후 key로 사용해 hint map참조하여 결과 반환

이로인해 npu0의 mock device와 npu1의 mock device두개간의 비교는 순서가 어떻게되든 항상 같은결과를 반환하여야합니다.

아래 변경을 포함해서, mock이 위와같이 동작할수 있도록 업데이트 부탁드립니다.

https://github.com/furiosa-ai/furiosa-smi-go/pull/4/files/7e52d82276113a96fabade872ec18c426626c6f1..2890ad553530680ce2b4edda3d0d4057fe1e0c7f#r1724280821

09fb6b3 에서 수정하였습니다.

bg-furiosa · 2024-08-21T02:25:24Z

pkg/smi/mock_common.go

이 파일의 변경에 대해선 Mock관점에서 생각해보아야 합니다.

Mock을 이용해 smi를 기반으로 작성된 로직을 테스트할떄 Mock Device객체의 DeviceErrorInfo함수 호출시 해당 로직을 타게 되는대요. 항상 에러를 반환하게 되면 Mock을 통한 테스트가 힘들어질수 있습니다. 이러한 이유로 기존에 항상 0을 반환하도록 되어 있었습니다.

0이 아닌 에러를 반환하고 싶다면 Mock Device Builder와 같이 Builder pattern을 사용해 Mock Device생성시 DeviceErrorInfo가 반환하고자 하는 에러를 임베딩 할수 있도록 변경이 필요해보입니다.

아래와 같이 진행부탁드립니다.

기본적으로는 모든 에러에 대해 0 반환

필요시 mock device builder로 0이 아닌 다른 에러 카운트 반환하도록 지원

다만 mock device builder를 도입한다면 static mock의 범주를 벗어나는거 같아 적절하게 파일과 구현을 분리하면 될거같습니다.

이해했습니다. 일단 이번 PR에서는 default 값을 0으로 만들고, mock device builder는 필요할경우 추후 다른 PR에서 업데이트 하겠습니다!

8b03317 에서 수정하였습니다.

bg-furiosa · 2024-08-21T02:41:50Z

pkg/smi/device_test.go

-func testGetDeviceToDeviceLinkType(device0 Device, device1 Device, t *testing.T, expected LinkType) {
+func testGetDeviceToDeviceLinkType(devices []Device, t *testing.T) {


jongchanpark-furiosa force-pushed the test-ci branch 2 times, most recently from 4cefdd3 to a79c836 Compare August 16, 2024 07:41

add test ci

3f69f5f

jongchanpark-furiosa force-pushed the test-ci branch from a79c836 to 3f69f5f Compare August 16, 2024 07:46

update makefile

7e52d82

jongchanpark-furiosa requested a review from hoony9x-furiosa-ai August 19, 2024 05:29

jongchanpark-furiosa marked this pull request as ready for review August 19, 2024 05:29

jongchanpark-furiosa requested a review from bg-furiosa as a code owner August 19, 2024 05:29

hoony9x-furiosa-ai approved these changes Aug 19, 2024

View reviewed changes

.github/workflows/golang.yml Outdated Show resolved Hide resolved

.github/workflows/golang.yml Outdated Show resolved Hide resolved

.github/workflows/golang.yml Show resolved Hide resolved

.github/workflows/golang.yml Show resolved Hide resolved

hoony9x-furiosa-ai assigned hoony9x-furiosa-ai and jongchanpark-furiosa and unassigned hoony9x-furiosa-ai Aug 19, 2024

update link type test

2890ad5

use up-to-date scripts

45eebb8

bg-furiosa reviewed Aug 21, 2024

View reviewed changes

separate ci steps

781e7f4

bg-furiosa reviewed Aug 21, 2024

View reviewed changes

jongchanpark-furiosa added 2 commits August 21, 2024 11:24

resolve name shadowing warning

0693bef

make mockdevice function public

442373d

bg-furiosa reviewed Aug 21, 2024

View reviewed changes

change mock device error default value as 0

8b03317

bg-furiosa reviewed Aug 21, 2024

View reviewed changes

update linktype hintmap test

09fb6b3

bg-furiosa approved these changes Aug 21, 2024

View reviewed changes

bg-furiosa merged commit 289b731 into main Aug 21, 2024
1 check passed

bg-furiosa deleted the test-ci branch August 21, 2024 23:43

hoony9x-furiosa-ai restored the test-ci branch August 22, 2024 06:43

hoony9x-furiosa-ai deleted the test-ci branch August 22, 2024 06:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add test ci #4

add test ci #4

jongchanpark-furiosa commented Aug 16, 2024 •

edited

Loading

jongchanpark-furiosa commented Aug 19, 2024

jongchanpark-furiosa commented Aug 19, 2024

hoony9x-furiosa-ai commented Aug 19, 2024

bg-furiosa Aug 21, 2024 •

edited

Loading

jongchanpark-furiosa Aug 21, 2024

bg-furiosa Aug 21, 2024

jongchanpark-furiosa Aug 21, 2024

bg-furiosa Aug 21, 2024

jongchanpark-furiosa Aug 21, 2024

bg-furiosa Aug 21, 2024

jongchanpark-furiosa Aug 21, 2024

bg-furiosa Aug 21, 2024

bg-furiosa Aug 21, 2024

jongchanpark-furiosa Aug 21, 2024

jongchanpark-furiosa Aug 21, 2024

bg-furiosa Aug 21, 2024

		func testGetDeviceToDeviceLinkType(device0 Device, device1 Device, t *testing.T, expected LinkType) {
		func testGetDeviceToDeviceLinkType(devices []Device, t *testing.T) {

add test ci #4

add test ci #4

Conversation

jongchanpark-furiosa commented Aug 16, 2024 • edited Loading

One line PR Description

What type of PR is this?

Special notes for reviewer

jongchanpark-furiosa commented Aug 19, 2024

jongchanpark-furiosa commented Aug 19, 2024

hoony9x-furiosa-ai commented Aug 19, 2024

bg-furiosa Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jongchanpark-furiosa commented Aug 16, 2024 •

edited

Loading

bg-furiosa Aug 21, 2024 •

edited

Loading