Support old-style TensorFlow events (tensorboard)

[garymm]

Fixes: #2466

[garymm]

I didn't see a place to add tests for this code. Is there a good place to add tests? If so, I'm happy to do it.

Also unrelated to this bug, but while testing I found it pretty surprising that you consider an event to match as long as the tag name starts with the metric name (e.g. a TF Event with tag "foobar" will match a metric name "foo"). Is this intended? Seems sort of surprising. If that's not intended, I can open another PR to fix.

[garymm]

@Electronic-Waste @andreyvelich could someone please review?

[Electronic-Waste]

Basically LGTM. Thanks for this @garymm! Just a few comments.

/assign @andreyvelich @helenxie-bit @mahdikhashan

[google-oss-prow]

@Electronic-Waste: GitHub didn't allow me to assign the following users: mahdikhashan.

Note that only kubeflow members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

Basically LGTM. Thanks for this @garymm! Just a few comments.

/assign @andreyvelich @helenxie-bit @mahdikhashan

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[mahdikhashan]

would you please make sure that this funcationality is also tested? there is an existing unit test at test/unit/metricscollector/test_tfevent_metricscollector.py.

Thanks for your time @garymm

[garymm]

@mahdikhashan @Electronic-Waste done

[mahdikhashan]

@mahdikhashan @Electronic-Waste done

thanks for your contribution @garymm . I kindly ask @andreyvelich @Electronic-Waste for their help by triggering the ci so then we can make sure that everything is fine.

[Electronic-Waste]

/rerun-all

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[garymm]

@Electronic-Waste I fixed the pre-commit issues. I doubt the e2e test failure is my fault.

[Electronic-Waste]

/rerun-all

[Electronic-Waste]

@garymm could you please resolve these conflicts?

[Electronic-Waste]

/rerun-all

[garymm]

@Electronic-Waste done

[andreyvelich]

@Electronic-Waste @mahdikhashan @garymm @kubeflow/wg-training-leads Do we want to cherry-pick this PR to the release-0.18 branch ?

[garymm]

Given how rarely there are releases it seems like it'd be a shame to not include this in 0.18, but up to you

[mahdikhashan]

@Electronic-Waste @mahdikhashan @garymm @kubeflow/wg-training-leads Do we want to cherry-pick this PR to the release-0.18 branch ?

yes, i agree with you. do we need to add any document for it?

cc: @garymm

[andreyvelich]

Yes, I would appreciate if @garymm or @mahdikhashan can update docs for the TensorFlowEvent Metrics Collector here:
https://www.kubeflow.org/docs/components/katib/user-guides/metrics-collector/

[garymm]

kubeflow/website#3999

[andreyvelich]

/lgtm
/approve

[google-oss-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andreyvelich

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:

• OWNERS [andreyvelich]

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

[andreyvelich]

/cherry-pick release-0.18

[google-oss-robot]

@andreyvelich: #2467 failed to apply on top of branch "release-0.18":

Applying: Support old-style TensorFlow events (tensorboard)
Applying: format
Applying: test
Using index info to reconstruct a base tree...
M	test/unit/v1beta1/requirements.txt
Falling back to patching base and 3-way merge...
Auto-merging test/unit/v1beta1/requirements.txt
CONFLICT (content): Merge conflict in test/unit/v1beta1/requirements.txt
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0003 test
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-0.18

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[tenzen-y]

@Electronic-Waste @mahdikhashan @garymm @kubeflow/wg-training-leads Do we want to cherry-pick this PR to the release-0.18 branch ?

I'm ok

[andreyvelich]

@garymm @mahdikhashan @Electronic-Waste Please can you help us to resolve conflicts and manually cherry-pick this commit to the release-branch ?

[mahdikhashan]

@garymm @mahdikhashan @Electronic-Waste Please can you help us to resolve conflicts and manually cherry-pick this commit to the release-branch ?

I'll do so.

[mahdikhashan]

@garymm @mahdikhashan @Electronic-Waste Please can you help us to resolve conflicts and manually cherry-pick this commit to the release-branch ?

PTAL: #2517