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.

Sign up for GitHub

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

Jump to bottom

Support JSON format logs in file-metrics-collector #1765

Merged
merged 15 commits into from
Apr 5, 2022
Merged

Support JSON format logs in file-metrics-collector #1765

merged 15 commits into from
Apr 5, 2022

Conversation

tenzen-y
Copy link
Member

@tenzen-y tenzen-y commented Jan 1, 2022 •
edited
Loading

What this PR does / why we need it:
Support JSON format logs in file-metrics-collector.
Also modified the script for E2E so that the test does not fail even if the Trial status becomes EarlyStopped.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1744

Checklist:

  • Docs included if any changes are user facing

@tenzen-y tenzen-y force-pushed the support-json-format-in-file-metrics-collector branch 2 times, most recently from 0dfb912 to 543d245 Compare January 1, 2022 03:48
@coveralls
Copy link

coveralls commented Jan 1, 2022 •
edited
Loading

Coverage Status

Coverage decreased (-0.4%) to 73.802% when pulling cb456dd on tenzen-y:support-json-format-in-file-metrics-collector into 36d0a57 on kubeflow:master.

@tenzen-y tenzen-y force-pushed the support-json-format-in-file-metrics-collector branch from 543d245 to 2e6eff1 Compare January 1, 2022 11:47
@tenzen-y
Copy link
Member Author

tenzen-y commented Jan 1, 2022

/retest

@tenzen-y tenzen-y force-pushed the support-json-format-in-file-metrics-collector branch 3 times, most recently from fcb2f0a to c5bd812 Compare January 1, 2022 16:11
@tenzen-y
Copy link
Member Author

tenzen-y commented Jan 1, 2022

/retest

@tenzen-y tenzen-y changed the title [WIP] Support JSON format logs in file-metrics-collector Support JSON format logs in file-metrics-collector Jan 1, 2022
@tenzen-y tenzen-y force-pushed the support-json-format-in-file-metrics-collector branch from c5bd812 to 1fb26bb Compare January 1, 2022 21:55
@tenzen-y
Copy link
Member Author

tenzen-y commented Jan 1, 2022

This PR is ready for review, please take a look.
/assign @andreyvelich @gaocegege @johnugeorge

@tenzen-y
Copy link
Member Author

tenzen-y commented Jan 4, 2022

I addressed your suggestion. @gaocegege
Please take another look.

@tenzen-y
Copy link
Member Author

tenzen-y commented Jan 11, 2022 •
edited
Loading

I'll re-run Charmed Katib / Test (pull_request) by force push.

@tenzen-y tenzen-y force-pushed the support-json-format-in-file-metrics-collector branch from 4eb6d6c to 87d2a39 Compare January 11, 2022 22:30
gaocegege
gaocegege reviewed Jan 12, 2022
View reviewed changes
Copy link
Member

@gaocegege gaocegege left a comment

Choose a reason for hiding this comment

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

Sorry for the late reply.

The PR LGTM!

Thanks for your contribution! 🎉 👍

/cc @andreyvelich @johnugeorge

andreyvelich
andreyvelich reviewed Jan 12, 2022
View reviewed changes
Copy link
Member

@andreyvelich andreyvelich left a comment

Choose a reason for hiding this comment

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

@tenzen-y Thanks a lot for implementing this feature!
I left some comments.
Also, please create PR to update Katib Website documentation about the new API and the new Metrics Collector functionality.

As we discussed on our meeting today, we will deliver this feature in the Katib 0.14 release, since it requires some additional testing and discussion.

/hold

pkg/webhook/v1beta1/pod/inject_webhook_test.go Outdated Show resolved Hide resolved
pkg/metricscollector/v1beta1/file-metricscollector/testdata/JSON/good.json Outdated Show resolved Hide resolved
cmd/metricscollector/v1beta1/file-metricscollector/main.go Outdated Show resolved Hide resolved
cmd/metricscollector/v1beta1/file-metricscollector/main.go Outdated Show resolved Hide resolved
cmd/metricscollector/v1beta1/file-metricscollector/main.go Outdated Show resolved Hide resolved
pkg/metricscollector/v1beta1/common/const.go
@@ -46,6 +46,8 @@ const (
// Score=1.23E10
DefaultFilter = `([\w|-]+)\s*=\s*([+-]?\d*(\.\d+)?([Ee][+-]?\d+)?)`

TimeStampJsonKey = "timestamp"
Copy link
Member

Choose a reason for hiding this comment

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

I think we should document this somewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

Does this mean we need to add documentation other than the website documentation? @andreyvelich

Copy link
Member

Choose a reason for hiding this comment

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

I meant just point about it in the website documentation.

pkg/metricscollector/v1beta1/file-metricscollector/file-metricscollector.go Show resolved Hide resolved
pkg/webhook/v1beta1/experiment/validator/validator.go Outdated Show resolved Hide resolved
pkg/webhook/v1beta1/pod/inject_webhook_test.go Outdated Show resolved Hide resolved
test/e2e/v1beta1/run-e2e-experiment.go Outdated Show resolved Hide resolved
@johnugeorge
Copy link
Member

Katib CI is broken due to #1842

@johnugeorge
Copy link
Member

johnugeorge commented Apr 5, 2022 •
edited
Loading

LGTM. Can you rebase the PR and we can then merge

@tenzen-y
Copy link
Member Author

tenzen-y commented Apr 5, 2022

LGTM. Can you rebase the PR and we can then merge

Sure.

tenzen-y and others added 15 commits April 5, 2022 23:22
@tenzen-y
support JSON format logs in file-metrics-collector
c82fdf5
@tenzen-y
review: convert fileFormat to type FileSystemFileFormat
572f071
@tenzen-y @andreyvelich
Update cmd/metricscollector/v1beta1/file-metricscollector/main.go
0265372 
Co-authored-by: Andrey Velichkevich <andrey.velichkevich@gmail.com>
@tenzen-y
review: remove func (f FileSystemFileFormat) String()
c38dd5a
@tenzen-y
review: get metricRegList only when the format is TEXT
703bed8
@tenzen-y
review: change var name in a script for e2e
d95f2a0
@tenzen-y
review: explict specify the cloudml-hypyertune in the Dockerfile
68ffbcc
@tenzen-y
review: use reflect.DeepEqual instead of go-cmp.Diff
2c8aaa1
@tenzen-y
review: stop using 'JSON' directly in error statements
8fb51d4
@tenzen-y
review: install specific version cloudml-hypertune
6930468
@tenzen-y
review: get objType in the updateStopRules function
774dd4b
@tenzen-y
review: save optimalObjValue across multiple stopRules
051952a
@tenzen-y
review: add warning messages to parseTimestamp func
35cfcff
@tenzen-y
review: generate test files with go test command
26f04ef
@tenzen-y
review: change api for new feature
cb456dd
@tenzen-y tenzen-y force-pushed the support-json-format-in-file-metrics-collector branch from e5f2ae4 to cb456dd Compare April 5, 2022 14:22
@tenzen-y
Copy link
Member Author

tenzen-y commented Apr 5, 2022

@johnugeorge
I have rebased this PR to fix the broken CI.

@johnugeorge
Copy link
Member

Thanks @tenzen-y for your patience. Sorry for the delay

/lgtm
/approve

@google-oss-prow google-oss-prow bot added the lgtm label Apr 5, 2022
@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johnugeorge, tenzen-y

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

@johnugeorge
Copy link
Member

/unhold

@google-oss-prow google-oss-prow bot merged commit d443ed3 into kubeflow:master Apr 5, 2022
@tenzen-y tenzen-y deleted the support-json-format-in-file-metrics-collector branch April 5, 2022 22:28
@tenzen-y tenzen-y mentioned this pull request Apr 10, 2022
Merged
@andreyvelich andreyvelich mentioned this pull request Feb 24, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andreyvelich andreyvelich andreyvelich left review comments

@kimwnasptd kimwnasptd kimwnasptd left review comments

@hougangliu hougangliu Awaiting requested review from hougangliu

@sperlingxx sperlingxx Awaiting requested review from sperlingxx

@johnugeorge johnugeorge Awaiting requested review from johnugeorge

@gaocegege gaocegege Awaiting requested review from gaocegege

Assignees

@johnugeorge johnugeorge

@gaocegege gaocegege

@andreyvelich andreyvelich

Labels
approved lgtm size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Proposal] Support JSON format for file-metrics-collector
6 participants
@tenzen-y @coveralls @johnugeorge @andreyvelich @gaocegege @kimwnasptd