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 model ouput to kcmas #423

Conversation

justadogistaken
Copy link
Contributor

@justadogistaken justadogistaken commented Dec 27, 2023

Emitting model inference result of pod to kcmas. Which could help centric components leverage the output to work on optimization stuff.

@justadogistaken justadogistaken added the workflow/merge-hold merge-hold: code is ready but still has dependency label Dec 27, 2023
@justadogistaken justadogistaken self-assigned this Dec 27, 2023
@justadogistaken justadogistaken force-pushed the feat/add_model_output_to_kcmas branch from 85c63fd to fb08fbd Compare January 2, 2024 08:37
Copy link

codecov bot commented Jan 2, 2024

Codecov Report

Attention: 85 lines in your changes are missing coverage. Please review.

Comparison is base (b6b30f2) 54.35% compared to head (766a246) 54.46%.
Report is 6 commits behind head on main.

❗ Current head 766a246 differs from pull request most recent head f1fe9cf. Consider uploading reports for the commit f1fe9cf to get more accurate results

Files Patch % Lines
...sadvisor/plugin/metric-emitter/syncer/pod/model.go 15.55% 38 Missing ⚠️
...server/agent/metric/malachite/fetcher_calculate.go 59.70% 20 Missing and 7 partials ⚠️
pkg/metaserver/agent/metric/malachite/fetcher.go 68.88% 10 Missing and 4 partials ⚠️
...aware/resource/helper/modelctrl/borwein/borwein.go 84.61% 2 Missing and 2 partials ⚠️
...in/inference/modelresultfetcher/borwein/borwein.go 87.50% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #423      +/-   ##
==========================================
+ Coverage   54.35%   54.46%   +0.10%     
==========================================
  Files         472      474       +2     
  Lines       51936    52084     +148     
==========================================
+ Hits        28232    28365     +133     
- Misses      20560    20572      +12     
- Partials     3144     3147       +3     
Flag Coverage Δ
unittest 54.46% <62.22%> (+0.10%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@justadogistaken justadogistaken force-pushed the feat/add_model_output_to_kcmas branch from fb08fbd to 5dda432 Compare January 2, 2024 09:53
@justadogistaken justadogistaken changed the title [wip]add model ouput to kcmas add model ouput to kcmas Jan 2, 2024
@justadogistaken justadogistaken added workflow/merge-ready merge-ready: code is ready and can be merged and removed workflow/merge-hold merge-hold: code is ready but still has dependency labels Jan 2, 2024
@@ -127,3 +163,106 @@ func (infp *InferencePlugin) fetchModelResult(ctx context.Context) {

wg.Wait()
}

func (infp *InferencePlugin) syncModelResults(ctx context.Context) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

not suggest to emit kcmas-related metrics everywhere, maybe it's better to put in metric package instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with you, but why I emit kcmas-metrics is it's hard to reuse podSyncer under metric package.
So do you accept that is goes as:
Create a new file under metric package. And define an exported function(not belongs to any struct). Then call it under InferencePlugin.Run?

@justadogistaken justadogistaken force-pushed the feat/add_model_output_to_kcmas branch from 5dda432 to bbdf36a Compare January 3, 2024 07:47
@justadogistaken justadogistaken force-pushed the feat/add_model_output_to_kcmas branch 6 times, most recently from aafcda3 to 3751ca7 Compare January 3, 2024 09:08
@justadogistaken justadogistaken force-pushed the feat/add_model_output_to_kcmas branch from 3751ca7 to 8c6cf06 Compare January 3, 2024 09:18
sun-yuliang
sun-yuliang previously approved these changes Jan 3, 2024
@@ -33,4 +33,37 @@ type BorweinParameter struct {
// the first key is the pod UID
Copy link
Collaborator

Choose a reason for hiding this comment

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

move key description to Results property?

emitter := emitterPool.GetDefaultMetricsEmitter().WithTags("advisor-inference")
metricEmitter := emitterPool.GetDefaultMetricsEmitter().WithTags("advisor-inference")

dataEmitter, err := emitterPool.GetMetricsEmitter(metricspool.PrometheusMetricOptions{
Copy link
Collaborator

Choose a reason for hiding this comment

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

it seems that dataEmitter isn't used in inferencePlugin?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

forgot to delete that.
removed already.

@justadogistaken justadogistaken force-pushed the feat/add_model_output_to_kcmas branch from 9e4a749 to 766a246 Compare January 3, 2024 11:47
@justadogistaken justadogistaken requested a review from csfldf January 4, 2024 02:33
@justadogistaken justadogistaken force-pushed the feat/add_model_output_to_kcmas branch from 766a246 to f1fe9cf Compare January 4, 2024 03:12
@waynepeking348 waynepeking348 merged commit 1674652 into kubewharf:main Jan 4, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
workflow/merge-ready merge-ready: code is ready and can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants