Kf operators use GetReplicaFunc (Error Handling)

[Future-Outlier]

Describe your changes

• move GetReplicaFunc to common package
• make mpi, pytorch, and tensorflow use GetReplicaFunc, for better error handling
• add error handling about resource type in pytorch and tensorflow (mpi has done it before)

Check all the applicable boxes

• I updated the documentation accordingly.
• All new and existing tests passed.
• All commits are signed-off.

Related PRs

#4469

#4167

[codecov]

Codecov Report

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

Comparison is base (98dd505) 59.73% compared to head (613a7b7) 59.37%.
Report is 18 commits behind head on master.

Files	Patch %	Lines
flyteplugins/go/tasks/plugins/testing/echo.go	5.00%	94 Missing and 1 partial ⚠️
...ks/pluginmachinery/flytek8s/plugin_exec_context.go	0.00%	75 Missing ⚠️
.../plugins/k8s/kfoperators/common/common_operator.go	1.58%	62 Missing ⚠️
...teplugins/go/tasks/plugins/testing/config_flags.go	16.00%	21 Missing ⚠️
...teadmin/pkg/repositories/transformers/execution.go	64.51%	11 Missing ⚠️
...o/tasks/plugins/k8s/kfoperators/pytorch/pytorch.go	64.28%	6 Missing and 4 partials ⚠️
...lugins/go/tasks/plugins/k8s/kfoperators/mpi/mpi.go	74.28%	5 Missing and 4 partials ⚠️
...s/plugins/k8s/kfoperators/tensorflow/tensorflow.go	83.72%	5 Missing and 2 partials ⚠️
flytepropeller/pkg/controller/controller.go	0.00%	6 Missing ⚠️
flytepropeller/pkg/webhook/init_cert.go	0.00%	6 Missing ⚠️
... and 3 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4471      +/-   ##
==========================================
- Coverage   59.73%   59.37%   -0.36%     
==========================================
  Files         636      638       +2     
  Lines       53855    53960     +105     
==========================================
- Hits        32169    32040     -129     
- Misses      19154    19390     +236     
+ Partials     2532     2530       -2     

Flag	Coverage Δ
unittests	59.37% <31.70%> (-0.36%)	⬇️

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.

[hamersaw]

What is the error here that this is fixing?

[Future-Outlier]

What is the error here that this is fixing?

Theses 2 places
https://github.com/flyteorg/flyte/pull/4471/files#diff-46ab17e48b782b6e73f152b8e3930f07a9585d25846b05feb9710f67a9820d07R221-R224
https://github.com/flyteorg/flyte/pull/4471/files#diff-06c5063166d378b704d2394410b8394f3d120ca088a5367c299f6b3a5efe4ef9R224-R227

[hamersaw]

Theses 2 places

Oh, the name indicates the issue is with parsing replica counts. IIUC these should be set by default, so just trying to make sure this function is necessary.

With regards to adding the type checking. I can see the advantage of being safe here, but the GetTaskPhase function is only called on the resource built by this plugin. So as long as it never creates something eradic there will not be an issue.

[Future-Outlier]

Theses 2 places

Oh, the name indicates the issue is with parsing replica counts. IIUC these should be set by default, so just trying to make sure this function is necessary.

With regards to adding the type checking. I can see the advantage of being safe here, but the GetTaskPhase function is only called on the resource built by this plugin. So as long as it never creates something eradic there will not be an issue.

Yes you are right!

[Future-Outlier]

You can consider that use GetReplicaFunc is a kind of refactor, does this convince you?