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

add Proposal for Large Language Model Edge Benchmark Suite: Implement… #127

Merged
merged 13 commits into from
Oct 22, 2024

Conversation

XueSongTap
Copy link
Contributor

…ation on KubeEdge-lanvs

What type of PR is this?
/kind design

What this PR does / why we need it:
This pr is a proposal for Large Language Model Edge Benchmark Suite: Implementation on KubeEdge-lanvs
Which issue(s) this PR fixes:

Fixes # #94

…ation on KubeEdge-lanvs

Signed-off-by: yexiaochuan <tap91624@gmail.com>
Copy link
Collaborator

@MooreZheng MooreZheng left a comment

Choose a reason for hiding this comment

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

Overall looks great to me. A few suggestions:

  1. The scheme is not purely of a single task. It would more like to be a compression version of the single task.
  2. GPU environment is necessary and a good point. But the current design is not clear. The overall Hardware configuration should be compatible with the previous version of yaml files.
  3. Review the system metrics, especially the inference phase.
  4. Highlight the modified part of the architecture.

@MooreZheng
Copy link
Collaborator

@hsj576 might also need to take a look at this proposal

@hsj576
Copy link
Member

hsj576 commented Jul 27, 2024

Overall looks great to me. Just a few questions:
Why do you need to add INT8 quantization, FP16 mixed precision, etc to TestCaseController? As far as I know you could load the INT8 quantization, FP16 mixed precision method by set "load_in_8bit=True," or "torch_dtype=torch.bfloat16" in “AutoModelForCausalLM.from_pretrained” function. Therefore, is it necessary to integrate those methods in the Ianvs TestCaseController?

Copy link
Contributor

@Shelley-BaoYue Shelley-BaoYue left a comment

Choose a reason for hiding this comment

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

@MooreZheng MooreZheng added kind/design Categorizes issue or PR as related to design. and removed proposal PR labels Aug 29, 2024
@MooreZheng
Copy link
Collaborator

/lgtm

@kubeedge-bot kubeedge-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 5, 2024
Copy link
Member

@hsj576 hsj576 left a comment

Choose a reason for hiding this comment

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

/lgtm

@kubeedge-bot kubeedge-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 18, 2024
Copy link
Collaborator

@MooreZheng MooreZheng left a comment

Choose a reason for hiding this comment

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

The implementation has been removed, which follows the suggestion made in the routine meeting. Overall it looks fine to me.

But some tiny concern

  1. the documents are a little bit confusing: why there are "llm-benchmark-suit" and "llm-benchmark-suite"?
  2. besides, the "edge" shall be highlighted in the name of the documents

@XueSongTap
Copy link
Contributor Author

the benchmark suite description was fixed and the edge was highlighted in the nameing of documents 4244a71

yexiaochuan added 2 commits September 28, 2024 15:36
…ation on KubeEdge-lanvs

Signed-off-by: yexiaochuan <yxc2020@foxmail.com>
Signed-off-by: yexiaochuan <yxc2020@foxmail.com>
Copy link
Collaborator

@MooreZheng MooreZheng left a comment

Choose a reason for hiding this comment

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

Looks fine to me for the revised version. As soon as the CI issues are fixed, it would be fine for ianvs to merge this proposal.

Run pylint '/home/runner/work/ianvs/ianvs/core'
************* Module core.testenvmanager.dataset.dataset
core/testenvmanager/dataset/dataset.py:119:0: W0012: Unknown option value for 'disable', expected a valid pylint message and got 'too-many-positional-arguments' (unknown-option-value)
core/testenvmanager/dataset/dataset.py:206:0: C0303: Trailing whitespace (trailing-whitespace)
core/testenvmanager/dataset/dataset.py:214:0: C0303: Trailing whitespace (trailing-whitespace)

-----------------------------------
Your code has been rated at 9.9[8](https://github.com/kubeedge/ianvs/actions/runs/11338050517/job/31531990780#step:5:9)/10

Error: The operation was canceled.

@kubeedge-bot kubeedge-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 14, 2024
yexiaochuan added 2 commits October 14, 2024 11:16
This reverts commit f341b0f.

Signed-off-by: yexiaochuan <yxc2020@foxmail.com>
…me of the documents

Signed-off-by: yexiaochuan <yxc2020@foxmail.com>
@kubeedge-bot kubeedge-bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 14, 2024
Signed-off-by: yexiaochuan <yxc2020@foxmail.com>
Signed-off-by: yexiaochuan <yxc2020@foxmail.com>
yexiaochuan added 2 commits October 14, 2024 19:49
Signed-off-by: yexiaochuan <yxc2020@foxmail.com>
Signed-off-by: yexiaochuan <yxc2020@foxmail.com>
@MooreZheng
Copy link
Collaborator

MooreZheng commented Oct 15, 2024

Looks fine to me for the revised version. As soon as the CI issues are fixed, it would be fine for ianvs to merge this proposal.

Seems that pylint message is not valid (as below). @XueSongTap might want to try out https://stackoverflow.com/questions/79019204/too-many-positional-arguments-on-one-machine-but-does-not-know-the-error-on-t

Run pylint '/home/runner/work/ianvs/ianvs/core'
************* Module core.testenvmanager.dataset.dataset
core/testenvmanager/dataset/dataset.py:119:0: W0012: Unknown option value for 'disable', expected a valid pylint message and got 'too-many-positional-arguments' (unknown-option-value)
core/testenvmanager/dataset/dataset.py:206:0: C0303: Trailing whitespace (trailing-whitespace)
core/testenvmanager/dataset/dataset.py:214:0: C0303: Trailing whitespace (trailing-whitespace)

-----------------------------------
Your code has been rated at 9.9[8](https://github.com/kubeedge/ianvs/actions/runs/11338050517/job/31531990780?pr=127#step:5:9)/10

Error: The operation was canceled.

Signed-off-by: yexiaochuan <yxc2020@foxmail.com>
Copy link
Collaborator

@MooreZheng MooreZheng left a comment

Choose a reason for hiding this comment

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

Need to fix below CI-error messages before merging.

Run pylint '/home/runner/work/ianvs/ianvs/core'
************* Module core.testenvmanager.dataset.dataset 
core/testenvmanager/dataset/dataset.py:119:0: W0012: Unknown option value for 'disable', expected a valid pylint message and got 'R0917' (unknown-option-value) core/testenvmanager/dataset/dataset.py:207:0: W0012: Unknown option value for 'disable', expected a valid pylint message and got 'R0917' (unknown-option-value) 
core/testenvmanager/dataset/dataset.py:215:0: W0012: Unknown option value for 'disable', expected a valid pylint message and got 'R0917' (unknown-option-value) core/testenvmanager/dataset/dataset.py:249:0: W0012: Unknown option value for 'disable', expected a valid pylint message and got 'R0917' (unknown-option-value) 
core/testenvmanager/dataset/dataset.py:289:0: W0012: Unknown option value for 'disable', expected a valid pylint message and got 'R0917' (unknown-option-value) core/testenvmanager/dataset/dataset.py:374:0: W0012: Unknown option value for 'disable', expected a valid pylint message and got 'R0917' (unknown-option-value)

-----------------------------------
Your code has been rated at 9.96/10

Error: Process completed with exit code 4.

BTW: @XueSongTap might want to try out this solution to similar error messages, as mentioned above

yexiaochuan and others added 3 commits October 19, 2024 16:51
Signed-off-by: yexiaochuan <yxc2020@foxmail.com>
Signed-off-by: yexiaochuan <yxc2020@foxmail.com>
@XueSongTap
Copy link
Contributor Author

Need to fix below CI-error messages before merging.

Run pylint '/home/runner/work/ianvs/ianvs/core'
************* Module core.testenvmanager.dataset.dataset 
core/testenvmanager/dataset/dataset.py:119:0: W0012: Unknown option value for 'disable', expected a valid pylint message and got 'R0917' (unknown-option-value) core/testenvmanager/dataset/dataset.py:207:0: W0012: Unknown option value for 'disable', expected a valid pylint message and got 'R0917' (unknown-option-value) 
core/testenvmanager/dataset/dataset.py:215:0: W0012: Unknown option value for 'disable', expected a valid pylint message and got 'R0917' (unknown-option-value) core/testenvmanager/dataset/dataset.py:249:0: W0012: Unknown option value for 'disable', expected a valid pylint message and got 'R0917' (unknown-option-value) 
core/testenvmanager/dataset/dataset.py:289:0: W0012: Unknown option value for 'disable', expected a valid pylint message and got 'R0917' (unknown-option-value) core/testenvmanager/dataset/dataset.py:374:0: W0012: Unknown option value for 'disable', expected a valid pylint message and got 'R0917' (unknown-option-value)

-----------------------------------
Your code has been rated at 9.96/10

Error: Process completed with exit code 4.

BTW: @XueSongTap might want to try out this solution to similar error messages, as mentioned above

Hi,I have resolved the pylint CI issues. Please review the changes and consider merging this PR.

@MooreZheng
Copy link
Collaborator

/lgtm

@kubeedge-bot kubeedge-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 22, 2024
@kubeedge-bot kubeedge-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 22, 2024
@MooreZheng
Copy link
Collaborator

/approve

@kubeedge-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hsj576, MooreZheng

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubeedge-bot kubeedge-bot merged commit a6182e6 into kubeedge:main Oct 22, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/design Categorizes issue or PR as related to design. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants