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

OSPP: Cloud-edge collaborative inference for LLM based on KubeEdge-Ianvs #149

Merged
merged 1 commit into from
Oct 31, 2024

Conversation

FuryMartin
Copy link
Contributor

What type of PR is this?
/kind feature

What this PR does / why we need it:
Cloud-edge collaborative inference for LLM based on KubeEdge-Ianvs

Which issue(s) this PR fixes:

Fixes #96

@kubeedge-bot kubeedge-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Sep 23, 2024
@kubeedge-bot kubeedge-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 23, 2024
@FuryMartin
Copy link
Contributor Author

FuryMartin commented Sep 23, 2024

Changes

Ianvs

Core

Denpendencies

  • fix missing dependencies.
    • requirements.txt

Example

  • add cloud-edge-collaborative-inference-for-llm example
    • examples/cloud-edge-collaborative-inference-for-llm/

Sedna

See details at FuryMartin/Sedna

  • support llm in Joint Inference
    • lib/sedna/core/joint_inference/joint_inference.py
  • fix version conflict of dependencies
    • lib/requirements.txt
  • fix TypeError Exception when model_url is None
    • lib/sedna/core/base.py
  • align dataset interface with @IcyFeather
    • lib/sedna/datasources/__init__.py

@FuryMartin
Copy link
Contributor Author

FuryMartin commented Sep 26, 2024

CI will install sedna-0.4.1-py3-none-any.whl under examples/resources/third_party/ to build environment.

However, since this PR updates Sedna, the old version installed in the CI environment lacks the new interfaces, leading to unexpected-keyword-arg and no-name-in-module errors.

Run pylint '/home/runner/work/ianvs/ianvs/core'
  
************* Module core.testcasecontroller.algorithm.paradigm.base
core/testcasecontroller/algorithm/paradigm/base.py:130:19: E1123: Unexpected keyword argument 'cloud' in constructor call (unexpected-keyword-arg)
core/testcasecontroller/algorithm/paradigm/base.py:130:19: E1123: Unexpected keyword argument 'LCReporter_enable' in constructor call (unexpected-keyword-arg)
************* Module core.testenvmanager.dataset.dataset
core/testenvmanager/dataset/dataset.py:21:0: E0611: No name 'JsonlDataParse' in module 'sedna.datasources' (no-name-in-module)
core/testenvmanager/dataset/dataset.py:21:0: E0611: No name 'JSONDataInfoParse' in module 'sedna.datasources' (no-name-in-module)

-----------------------------------
Your code has been rated at 9.8/10
Error: The operation was canceled.

It's a little tricky, I'm not sure how to solve it.

@hsj576 @MooreZheng


I have written a detailed analysis of this issue in #134, and further discussions will take place there.

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.

Reminder: there are CI issues that remain to be resolved, see https://github.com/kubeedge/ianvs/actions/runs/11201880744?pr=149

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.

The sedna installation originally used by the community should have a backup in third_party-bk.

@FuryMartin
Copy link
Contributor Author

Reminder: there are CI issues that remain to be resolved, see https://github.com/kubeedge/ianvs/actions/runs/11201880744?pr=149

As discussed in #152 , this has been temporarily fixed by introducing sedna-0.6.0.1-py3-none-any.whl. This package is constructed from my sedna fork FuryMartin/sedna.

The sedna installation originally used by the community should have a backup in third_party-bk.

I have move the sedna-0.4.1-py3-none-any.whl to third_party-bk

I have completed all the comment completion and document organization work.
Please review the code. @hsj576 @MooreZheng

@hsj576
Copy link
Member

hsj576 commented Oct 28, 2024

/lgtm

@kubeedge-bot kubeedge-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 28, 2024
@FuryMartin FuryMartin requested a review from MooreZheng October 28, 2024 14:25
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.

  1. This branch has conflicts that must be resolved
    Use the web editor or the to resolve conflicts.
    Conflicting files
    core/common/constant.py
    core/testcasecontroller/algorithm/algorithm.py
    core/testcasecontroller/algorithm/module/module.py
    core/testcasecontroller/algorithm/paradigm/init.py
    core/testcasecontroller/algorithm/paradigm/base.py
    core/testenvmanager/dataset/dataset.py

  2. This pull request contains 12 commits, which might make maintenance difficult, considering the number of contributors, pull requests, and their commits in KubeEdge Ianvs recently. In the final stage, @FuryMartin can squash the commits into one using rebase techniques.

@kubeedge-bot kubeedge-bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 29, 2024
@FuryMartin
Copy link
Contributor Author

This branch has conflicts that must be resolved. Use the web editor or the to resolve conflicts.

squash the commits into one using rebase techniques.

Thanks for the review. I'll fix them tomorrow.

@kubeedge-bot kubeedge-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 29, 2024
@kubeedge-bot kubeedge-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 30, 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.

Current conflicts:

This branch has conflicts that must be resolved
Use the web editor or the to resolve conflicts.
Conflicting files
core/common/constant.py
core/storymanager/rank/rank.py
core/testcasecontroller/algorithm/algorithm.py
core/testcasecontroller/algorithm/module/module.py
core/testcasecontroller/algorithm/paradigm/init.py
core/testcasecontroller/algorithm/paradigm/base.py
core/testcasecontroller/metrics/metrics.py
core/testenvmanager/dataset/dataset.py

@FuryMartin
Copy link
Contributor Author

FuryMartin commented Oct 30, 2024

Current conflicts:

This branch has conflicts that must be resolved Use the web editor or the to resolve conflicts. Conflicting files core/common/constant.py core/storymanager/rank/rank.py core/testcasecontroller/algorithm/algorithm.py core/testcasecontroller/algorithm/module/module.py core/testcasecontroller/algorithm/paradigm/init.py core/testcasecontroller/algorithm/paradigm/base.py core/testcasecontroller/metrics/metrics.py core/testenvmanager/dataset/dataset.py

Thanks for the review. These conflicts are caused by the revert #160.I believe they will be fixed automatically by merging #161 at first.

Update at 2024-10-30 21:18

#161 has been closed. @Yoda-wu is trying to create a new PR #162 contains the commits of #143 and #161.

I advise @MooreZheng to first merge #162 , and then proceed with merging #144 . (Because I might have some conflicts with #144 as well)

I will be on call any time tomorrow.

Thanks!

@kubeedge-bot kubeedge-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 30, 2024
@MooreZheng
Copy link
Collaborator

Thanks for the review. These conflicts are caused by the revert #160.I believe they will be fixed automatically by merging #161 at first.

Update at 2024-10-30 21:18

#161 has been closed. @Yoda-wu is trying to create a new PR #162 contains the commits of #143 and #161.

I advise @MooreZheng to first merge #162 , and then proceed with merging #144 . (Because I might have some conflicts with #144 as well)

I will be on call any time tomorrow.

Thanks!

Sure. Will try following that path.

@hsj576
Copy link
Member

hsj576 commented Oct 30, 2024

/lgtm

@kubeedge-bot kubeedge-bot added lgtm Indicates that a PR is ready to be merged. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 30, 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.

After merging PR 162 and PR 144, the following new conflicts do come to fix

This branch has conflicts that must be resolved
Use the web editor or the to resolve conflicts.
Conflicting files
core/common/constant.py
core/testenvmanager/dataset/dataset.py

@kubeedge-bot kubeedge-bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 30, 2024
@FuryMartin
Copy link
Contributor Author

After merging PR 162 and PR 144, the following new conflicts do come to fix

Fixed.

@FuryMartin FuryMartin force-pushed the code branch 2 times, most recently from 3005581 to 0c6427c Compare October 30, 2024 14:42
@FuryMartin
Copy link
Contributor Author

FuryMartin commented Oct 30, 2024

Hi @MooreZheng , if possible, please merge #159 first, too.

Reason

#144 fix #152's pylint error by globally disable two style checks in dataset.py

# pylint: disable=no-name-in-module
# pylint: disable=too-many-instance-attributes

As we discussed before, such global-disable-thing is not a good manner.

#162, #144, #159 all used such method to pass the CI check.

I have fixed it in my PR by including a new sedna package, so it's better to leave me the last to merge.

BTW, #154 is not related to this issue, it can be merged at anytime.

@MooreZheng
Copy link
Collaborator

Hi @MooreZheng , if possible, please merge #159 first, too.

FYI I will on my flight tomorrow morning... Have talked to the #159 owner @safe-b on WeChat and left comments on his PR. @FuryMartin might want to lend him a hand to speed up the overall process.

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.

Other OSPP projects have been merged, so this shall be the last conflict

This branch has conflicts that must be resolved
Use the web editor or the to resolve conflicts.
Conflicting files
core/testenvmanager/dataset/dataset.py

…example

Signed-off-by: Yu Fan <[email protected]>

add: sedna 0.6.0.1 and move 0.4.1 to third_party-bk

Signed-off-by: Yu Fan <[email protected]>
@FuryMartin
Copy link
Contributor Author

Other OSPP projects have been merged, so this shall be the last conflict

Solved.

@MooreZheng
Copy link
Collaborator

/lgtm

@kubeedge-bot kubeedge-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 31, 2024
@MooreZheng
Copy link
Collaborator

/approve

@kubeedge-bot kubeedge-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 31, 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.

All concerns are fixed. Well done! @FuryMartin

@kubeedge-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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 852973a into kubeedge:main Oct 31, 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/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cloud-edge collaborative inference for LLM based on KubeEdge-Ianvs
4 participants