Updating malachite metric for supporting model inference #368
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
justadogistaken
added
the
workflow/merge-hold
justadogistaken
force-pushed
the
optimize_metrics
branch
2 times, most recently
from
caf470c to
18c7b8c
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #368 +/- ##
==========================================
- Coverage 53.59% 53.57% -0.03%
==========================================
Files 444 445 +1
Lines 49021 49349 +328
==========================================
+ Hits 26273 26437 +164
- Misses 19791 19916 +125
- Partials 2957 2996 +39
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
justadogistaken
force-pushed
the
optimize_metrics
branch
from
18c7b8c to
c7f47d7
Compare
justadogistaken
changed the title
justadogistaken
requested review from
sun-yuliang,
csfldf and
waynepeking348
justadogistaken
added
workflow/merge-ready
|
@luomingmeng this pr changes too much for fundamental metric-info in meta-server, should it be tested carefully before it can be merged? |
justadogistaken
force-pushed
the
optimize_metrics
branch
7 times, most recently
from
3597266 to
620cfa3
Compare
waynepeking348
previously approved these changes
Nov 30, 2023
justadogistaken
force-pushed
the
optimize_metrics
branch
2 times, most recently
from
83ccfef to
30310c2
Compare
justadogistaken
force-pushed
the
optimize_metrics
branch
3 times, most recently
from
7e8d55b to
642f53f
Compare
justadogistaken
commented
Dec 5, 2023
| containerInfo.PodNamespace, containerInfo.PodName, containerInfo.ContainerName) | ||
| klog.Warningf("getContainerFeatureValue for pod: %s/%s, container: %s failed, err: %v", | ||
| containerInfo.PodNamespace, containerInfo.PodName, containerInfo.ContainerName, err) | ||
| goto IgnoreThisContainer |
There was a problem hiding this comment.
is someone against using "goto"?
There was a problem hiding this comment.
already got some goto, don't mind
justadogistaken
force-pushed
the
optimize_metrics
branch
from
642f53f to
1b8ffff
Compare
| c.InferenceServiceSocketAbsPath = o.InferenceServiceSocketAbsPath | ||
| c.NodeFeatureNames = o.NodeFeatureNames |
There was a problem hiding this comment.
NewBorweinConfiguration has some todo comments, pls delete them
| @@ -31,4 +31,5 @@ type BorweinParameter struct { | |||
|
|
|||
| // the first key is the pod UID | |||
There was a problem hiding this comment.
add prefix for this comments
csfldf
reviewed
Dec 6, 2023
| if respContainersCnt != len(requestContainers) { | ||
| return nil, fmt.Errorf("count of resp containers: %d and request containers: %d are not same", | ||
| klog.Warningf("count of resp containers: %d and request containers: %d are not same", |
There was a problem hiding this comment.
If respContainersCnt != len(requestContainers), the possibility of abnormal state may increase since denominator may decrease; so we still return error here.
And if some containers lack essential features, we also inference it forcibly.
csfldf
reviewed
Dec 6, 2023
| } | ||
|
|
||
| // AddFlags adds flags to the specified FlagSet. | ||
| func (o *BorweinOptions) AddFlags(fs *pflag.FlagSet) { | ||
| fs.StringVar(&o.InferenceServiceSocketAbsPath, "borwein-inference-svc-socket-path", o.InferenceServiceSocketAbsPath, | ||
| "socket path which borwein inference server listens at") | ||
| fs.StringSliceVar(&o.NodeFeatureNames, "borwein-node-feature-names", o.NodeFeatureNames, |
There was a problem hiding this comment.
after discussion, we use config file to configure NodeFeatureNames and ContainerFeatureNames.
csfldf
reviewed
Dec 6, 2023
| if err != nil { | ||
| return nil, fmt.Errorf("getContainerFeatureValue for pod: %s/%s, container: %s failed", |
There was a problem hiding this comment.
after discussion,
- getContainerFeatureValue should be refined to return nil err and zero value when input feature isn't a key feature
- if err != nil, we just return err here to avoid meaningless request to inference sever
justadogistaken
force-pushed
the
optimize_metrics
branch
2 times, most recently
from
85c4ec2 to
bc38275
Compare
justadogistaken
force-pushed
the
optimize_metrics
branch
from
bc38275 to
0fb5476
Compare
justadogistaken
force-pushed
the
optimize_metrics
branch
from
0fb5476 to
66dc554
Compare
csfldf
approved these changes
Dec 8, 2023
waynepeking348
approved these changes
Dec 11, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There are several things has been done in this pr:
NewBorweinConfiguration.cpu.nr.throttled.container,cpu.nr.period.container,cpu.throttled.time.containeretc... There metrics are meaningless as the raw data. And they are put into model training in the form of rate.