[GSoC] Compatibility Changes in Trial Controller

[Electronic-Waste]

What this PR does / why we need it:

I made some compatibility changes to the Trial Controller. Design details: https://github.com/kubeflow/katib/blob/master/docs/proposals/push-based-metrics-collection.md#compatibility-changes-in-trial-controller

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 #

Checklist:

• Docs included if any changes are user facing

[Electronic-Waste]

@andreyvelich @johnugeorge PTAL👀 if you are available. Thanks!

ref issue: #2340

[andreyvelich]

Thank you for this @Electronic-Waste!
cc @kubeflow/wg-automl-leads

[andreyvelich]

/rerun-all

[andreyvelich]

/assign @johnugeorge @tenzen-y
Please review it when you have time.

[tenzen-y]

/assign @johnugeorge @tenzen-y Please review it when you have time.

ACK

[tenzen-y]

Basically, lgtm

[johnugeorge]

/lgtm

[tenzen-y]

/area gsoc

[Electronic-Waste]

@andreyvelich @tenzen-y I've fixed the flaky error by separating the UTs and adjusting the EXPECT() mock clause!

AFAIK, the flaky error is caused by the uncertain triggering times of the reconciliation, thus giving rise to the uncertainty of the times we call the function. The most annoying issue was that we must call GetObservationLog and ReportObservationLog in turn and we didn't know the times they were triggered when we use Push MC.

So I reserved some EXPECT() function in the gomock.InOrder and added some new .AnyTimes() clause outside it. Please let me know if there are any modifications need to be made to this PR. Thanks!

[Electronic-Waste]

@kubeflow/wg-automl-leads it seems that the coverage reports have some accidents. Could you please re-rerun these test cases and check them again?

[andreyvelich]

@Electronic-Waste I think, you can re-trigger tests by add this comment: /rerun-all

[andreyvelich]

/rerun-all

[Electronic-Waste]

Thanks @andreyvelich
I think what I mean is running those few failed test cases. Sorry for not realizing that I can re-run them all without you. I'll re-rerun them on my own in the future.

[andreyvelich]

Not sure why coveralls fails on report tho.
@Electronic-Waste Please can you try to investigate it ?
cc @kubeflow/wg-training-leads

[Electronic-Waste]

@andreyvelich AFAIK It fails sometimes. It may turn normal in a few hours.

[Electronic-Waste]

@andreyvelich There is an issue describing it: lemurheavy/coveralls-public#1716

It seems that the coverage report will fail if we rerun the old CI build: lemurheavy/coveralls-public#1716 (comment)

[Electronic-Waste]

Some test cases are so flaky.
@Electronic-Waste Could you investigate them?

@tenzen-y I think the flaky issue of UTs has been solved now. Could take a look if that looks good to you?

[Electronic-Waste]

@andreyvelich There is an issue describing it: lemurheavy/coveralls-public#1716

It seems that the coverage report will fail if we rerun the old CI build: lemurheavy/coveralls-public#1716 (comment)

@tenzen-y FYR, the coverage report will fail if we rerun the old CI build.

Maybe we should restart all CI builds and test them all.

[tenzen-y]

/rerun-all

[Electronic-Waste]

@tenzen-y The /rerun-all command only restarts Go Test. We need to restart all CI builds since the bug report is:

{"message":"Can't add a job to a build that is already closed. Build 10789114390 is closed. See docs.coveralls.io/parallel-builds","error":true}

Maybe this comment lemurheavy/coveralls-public#1716 (comment) is useful for reference.

[Electronic-Waste]

I made some tiny changes in the comment lines. It will retrigger all CI builds.

Maybe this can help you check the robustness of UTs in Go Test @tenzen-y :)

[Electronic-Waste]

/rerun-all

[Electronic-Waste]

@andreyvelich @tenzen-y I've fixed the flaky error by separating the UTs and adjusting the EXPECT() mock clause!

AFAIK, the flaky error is caused by the uncertain triggering times of the reconciliation, thus giving rise to the uncertainty of the times we call the function. The most annoying issue was that we must call GetObservationLog and ReportObservationLog in turn and we didn't know the times they were triggered when we use Push MC.

So I reserved some EXPECT() function in the gomock.InOrder and added some new .AnyTimes() clause outside it. Please let me know if there are any modifications need to be made to this PR. Thanks!

@tenzen-y I'm sure that the flaky error has been addressed now. May I ask whether you need to check it again or not? I can trigger the CI builds again by pushing more tiny changes since the coverage report only works when we retrigger all CI builds.

[tenzen-y]

@andreyvelich @tenzen-y I've fixed the flaky error by separating the UTs and adjusting the EXPECT() mock clause!
AFAIK, the flaky error is caused by the uncertain triggering times of the reconciliation, thus giving rise to the uncertainty of the times we call the function. The most annoying issue was that we must call GetObservationLog and ReportObservationLog in turn and we didn't know the times they were triggered when we use Push MC.
So I reserved some EXPECT() function in the gomock.InOrder and added some new .AnyTimes() clause outside it. Please let me know if there are any modifications need to be made to this PR. Thanks!

@tenzen-y I'm sure that the flaky error has been addressed now. May I ask whether you need to check it again or not? I can trigger the CI builds again by pushing more tiny changes since the coverage report only works when we retrigger all CI builds.

Thank you for driving this! Throughout this 2 week CI result, I am sure that we succeeded to get rid of flakiness root causes.

https://github.com/kubeflow/katib/actions/workflows/test-go.yaml

[tenzen-y]

The most things lgtm

[tenzen-y]

That was great improvments!
Thank you for doing this!

/lgtm
/approve

[google-oss-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andreyvelich, 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:

• OWNERS [andreyvelich,tenzen-y]

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

[Electronic-Waste]

Thank you for your detailed review @tenzen-y!

This PR is holding now. Can you remove the hold label so that this PR can be merged?

[tenzen-y]

Thank you for your detailed review @tenzen-y!

This PR is holding now. Can you remove the hold label so that this PR can be merged?

Sure.
/hold cancel