[SDK] fix grpc related bugs in Python SDK #2398

Electronic-Waste · 2024-07-26T14:41:52Z

What this PR does / why we need it:

This PR fixes the bug described in issue #2395

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 #2395

Checklist:

Docs included if any changes are user facing

Electronic-Waste · 2024-07-28T15:42:53Z

I passed all the UTs in my local cluster:

================================================================================ 1 passed in 2.32s =================================================================================
cp ./pkg/apis/manager/v1beta1/python/api_pb2.py ./sdk/python/v1beta1/kubeflow/katib/katib_api_pb2.py
cp ./pkg/apis/manager/v1beta1/python/api_pb2_grpc.py ./sdk/python/v1beta1/kubeflow/katib/katib_api_pb2_grpc.py
sed -i "s/api_pb2/kubeflow\.katib\.katib_api_pb2/g" ./sdk/python/v1beta1/kubeflow/katib/katib_api_pb2_grpc.py
pytest ./sdk/python/v1beta1/kubeflow/katib
=============================================================================== test session starts ================================================================================
platform linux -- Python 3.10.14, pytest-7.2.0, pluggy-1.5.0
rootdir: /home/ws/katib/sdk/python/v1beta1
collected 14 items                                                                                                                                                                 

sdk/python/v1beta1/kubeflow/katib/api/katib_client_test.py ........                                                                                                          [ 57%]
sdk/python/v1beta1/kubeflow/katib/api/report_metrics_test.py ......                                                                                                          [100%]

================================================================================ 14 passed in 0.49s ================================================================================
rm ./sdk/python/v1beta1/kubeflow/katib/katib_api_pb2.py ./sdk/python/v1beta1/kubeflow/katib/katib_api_pb2_grpc.py

But it failed in the CI/CD:

==================================== ERRORS ====================================
__________ ERROR collecting kubeflow/katib/api/report_metrics_test.py __________
ImportError while importing test module '/home/runner/work/katib/katib/sdk/python/v1beta1/kubeflow/katib/api/report_metrics_test.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/opt/hostedtoolcache/Python/3.11.9/x64/lib/python3.11/importlib/__init__.py:126: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
sdk/python/v1beta1/kubeflow/katib/api/report_metrics_test.py:3: in <module>
    from kubeflow.katib import report_metrics
E   ImportError: cannot import name 'report_metrics' from 'kubeflow.katib' (/home/runner/work/katib/katib/sdk/python/v1beta1/kubeflow/katib/__init__.py)
=========================== short test summary info ============================
ERROR sdk/python/v1beta1/kubeflow/katib/api/report_metrics_test.py
!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!
=============================== 1 error in 0.60s ===============================

@andreyvelich Could you help me with figuring out what's wrong with the report_metrics?

Electronic-Waste · 2024-07-29T10:45:41Z

Changes I made:

Copy api_pb2_grpc.py to Python SDK and make some compatibility changes to setup.py and Makefile
Refactor the logics concerned with grpc
Fix a field error in katib_client.py
Add UTs for report_metrics and get_trial_metrics

Signed-off-by: Electronic-Waste <[email protected]>

Electronic-Waste · 2024-07-30T07:10:14Z

I passed all the UTs in my local cluster:

================================================================================ 1 passed in 2.32s =================================================================================
cp ./pkg/apis/manager/v1beta1/python/api_pb2.py ./sdk/python/v1beta1/kubeflow/katib/katib_api_pb2.py
cp ./pkg/apis/manager/v1beta1/python/api_pb2_grpc.py ./sdk/python/v1beta1/kubeflow/katib/katib_api_pb2_grpc.py
sed -i "s/api_pb2/kubeflow\.katib\.katib_api_pb2/g" ./sdk/python/v1beta1/kubeflow/katib/katib_api_pb2_grpc.py
pytest ./sdk/python/v1beta1/kubeflow/katib
=============================================================================== test session starts ================================================================================
platform linux -- Python 3.10.14, pytest-7.2.0, pluggy-1.5.0
rootdir: /home/ws/katib/sdk/python/v1beta1
collected 14 items                                                                                                                                                                 

sdk/python/v1beta1/kubeflow/katib/api/katib_client_test.py ........                                                                                                          [ 57%]
sdk/python/v1beta1/kubeflow/katib/api/report_metrics_test.py ......                                                                                                          [100%]

================================================================================ 14 passed in 0.49s ================================================================================
rm ./sdk/python/v1beta1/kubeflow/katib/katib_api_pb2.py ./sdk/python/v1beta1/kubeflow/katib/katib_api_pb2_grpc.py

But it failed in the CI/CD:

==================================== ERRORS ====================================
__________ ERROR collecting kubeflow/katib/api/report_metrics_test.py __________
ImportError while importing test module '/home/runner/work/katib/katib/sdk/python/v1beta1/kubeflow/katib/api/report_metrics_test.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/opt/hostedtoolcache/Python/3.11.9/x64/lib/python3.11/importlib/__init__.py:126: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
sdk/python/v1beta1/kubeflow/katib/api/report_metrics_test.py:3: in <module>
    from kubeflow.katib import report_metrics
E   ImportError: cannot import name 'report_metrics' from 'kubeflow.katib' (/home/runner/work/katib/katib/sdk/python/v1beta1/kubeflow/katib/__init__.py)
=========================== short test summary info ============================
ERROR sdk/python/v1beta1/kubeflow/katib/api/report_metrics_test.py
!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!
=============================== 1 error in 0.60s ===============================

@andreyvelich Could you help me with figuring out what's wrong with the report_metrics?

@andreyvelich @johnugeorge @tenzen-y I've resolved this bug. PTAL👀

Electronic-Waste · 2024-08-01T17:34:55Z

This PR is related to my GSoC project. ref issue: #2340

sdk/python/v1beta1/kubeflow/katib/api/katib_client_test.py

andreyvelich · 2024-08-07T13:39:36Z

sdk/python/v1beta1/setup.py

+
+    with open("kubeflow/katib/katib_api_pb2_grpc.py", 'r+') as file:
+        content = file.read()
+        new_content = content.replace("api_pb2", "kubeflow.katib.katib_api_pb2")


Is there a way to avoid this tricky import change ?
Maybe we could add those protobuf files as part of SDK __init__.py ?
https://github.com/kubeflow/katib/blob/8b095add55d7dd8983eba3d3b4bab34d51dc2f35/sdk/python/v1beta1/kubeflow/katib/__init__.py

Yeah, we can achieve this by not renaming those files, i.e. copy api_pb2.py to python SDK as api_pb2.py rather than katib_api_pb2.py.

@andreyvelich I think we may need to do this tricky import change, because adding import kubeflow.katib.api_pb2 as api_pb2 to __init__.py makes no difference in resolving the import error:

__init__.py and api_pb2_grpc.py are different modules in this directory. Importing api_pb2 in __init__.py will not affect package import in api_pb2_grpc.py.

Python3 has banned relative package import like import api_pb2. Python will search for api_pb2.py in the sys.path rather than the same directory. If we want to use relative import, we need to specify the package path like from . import <filename>.

I see that makes sense, @droctothorpe @tenzen-y @johnugeorge any thoughts ?
I guess, we still need to modify the import to make sure we import the api_pb2.py from kubeflow.katib package.

@Electronic-Waste This import error happened only in the CI, and you have not seen it in your local, right?

Sure，I'll investigate it.

@tenzen-y @andreyvelich I've found a related issue describing this: protocolbuffers/protobuf#7061

The python_package option is not on the developing schedule up to now. So I think we can't avoid this hack approach now :)

@Electronic-Waste Please can you add the TODO comment in your post-gen script to track this issue: protocolbuffers/protobuf#7061, so maybe in the future we can remove this import re-write.

@andreyvelich I've added the TODO comment to the post-gen scripts now :)

Thanks for investigating this problem.

Signed-off-by: Electronic-Waste <[email protected]>

andreyvelich · 2024-08-22T17:38:57Z

Please run pre-commit to fix lint errors.

Signed-off-by: Electronic-Waste <[email protected]>

Electronic-Waste · 2024-08-22T17:56:20Z

@andreyvelich Lint error has been fixed now.

andreyvelich

Thanks @Electronic-Waste!
/lgtm
/assign @tenzen-y @johnugeorge

tenzen-y

Thanks.
/lgtm
/approve

tenzen-y · 2024-08-22T18:33:33Z

sdk/python/v1beta1/setup.py

+
+    with open("kubeflow/katib/katib_api_pb2_grpc.py", 'r+') as file:
+        content = file.read()
+        new_content = content.replace("api_pb2", "kubeflow.katib.katib_api_pb2")


Thanks for investigating this problem.

google-oss-prow · 2024-08-22T18:33:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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~~ [tenzen-y]

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

google-oss-prow bot requested review from andreyvelich, anencore94 and gaocegege July 26, 2024 14:41

google-oss-prow bot added the size/L label Jul 26, 2024

Electronic-Waste mentioned this pull request Jul 28, 2024

[GSoC] Project6: Push-based Metrics Collection for Katib #2340

Open

6 tasks

Electronic-Waste force-pushed the fix/grpc-bug branch from 6f3ec80 to 73af86d Compare July 30, 2024 06:31

Electronic-Waste added 10 commits July 30, 2024 06:39

fix: fix bugs in report_metrics.

bf2673e

Signed-off-by: Electronic-Waste <[email protected]>

fix: fix bugs in tune.

69e3d62

Signed-off-by: Electronic-Waste <[email protected]>

fix: fix bugs in get_trial_metrics.

a3d2e9f

Signed-off-by: Electronic-Waste <[email protected]>

fix: update .gitignore and setup.py.

22fe26a

Signed-off-by: Electronic-Waste <[email protected]>

fix: update Makefile.

570584c

Signed-off-by: Electronic-Waste <[email protected]>

feat: add report_metrics_test.py.

bfbb5b7

Signed-off-by: Electronic-Waste <[email protected]>

fix: fix lint error.

6f0e9ed

Signed-off-by: Electronic-Waste <[email protected]>

feat: add UTs for get_trial_metrics.

462347f

Signed-off-by: Electronic-Waste <[email protected]>

fix: update post_gen.py.

a7a845f

Signed-off-by: Electronic-Waste <[email protected]>

refactor: rebase to master.

8b095ad

Signed-off-by: Electronic-Waste <[email protected]>

Electronic-Waste force-pushed the fix/grpc-bug branch from 73af86d to 8b095ad Compare July 30, 2024 06:45

andreyvelich reviewed Aug 7, 2024

View reviewed changes

test(sdk): use single katib_client.

f6997f7

Signed-off-by: Electronic-Waste <[email protected]>

Electronic-Waste requested review from andreyvelich and tenzen-y August 15, 2024 08:39

Electronic-Waste and others added 2 commits August 22, 2024 17:27

fix(sdk): add TODO for import rewrite.

a0902cb

Signed-off-by: Electronic-Waste <[email protected]>

Merge branch 'master' into fix/grpc-bug

239a55f

Electronic-Waste added 2 commits August 22, 2024 17:41

fix(sdk): fix lint error with black.

ff5031a

Signed-off-by: Electronic-Waste <[email protected]>

fix(sdk): fix lint error with isort.

74eabaa

Signed-off-by: Electronic-Waste <[email protected]>

Electronic-Waste force-pushed the fix/grpc-bug branch from b9bf22e to cfa26d5 Compare August 22, 2024 17:49

fix(sdk): reformat import in katib_client_test.py.

7b6fd85

Signed-off-by: Electronic-Waste <[email protected]>

Electronic-Waste force-pushed the fix/grpc-bug branch from cfa26d5 to 7b6fd85 Compare August 22, 2024 17:51

andreyvelich reviewed Aug 22, 2024

View reviewed changes

google-oss-prow bot assigned johnugeorge, tenzen-y and andreyvelich Aug 22, 2024

google-oss-prow bot added the lgtm label Aug 22, 2024

tenzen-y reviewed Aug 22, 2024

View reviewed changes

google-oss-prow bot added the approved label Aug 22, 2024

google-oss-prow bot merged commit a524f33 into kubeflow:master Aug 23, 2024
63 checks passed

Electronic-Waste mentioned this pull request Sep 1, 2024

[SDK] Fix the tricky import changes in Python SDK #2416

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SDK] fix grpc related bugs in Python SDK #2398

[SDK] fix grpc related bugs in Python SDK #2398

Electronic-Waste commented Jul 26, 2024

Electronic-Waste commented Jul 28, 2024

Electronic-Waste commented Jul 29, 2024

Electronic-Waste commented Jul 30, 2024

Electronic-Waste commented Aug 1, 2024

andreyvelich Aug 7, 2024

Electronic-Waste Aug 8, 2024

Electronic-Waste Aug 15, 2024

andreyvelich Aug 15, 2024

tenzen-y Aug 15, 2024

Electronic-Waste Aug 17, 2024

Electronic-Waste Aug 21, 2024 •

edited

Loading

andreyvelich Aug 22, 2024 •

edited

Loading

Electronic-Waste Aug 22, 2024

tenzen-y Aug 22, 2024

andreyvelich commented Aug 22, 2024

Electronic-Waste commented Aug 22, 2024

andreyvelich left a comment

tenzen-y left a comment

tenzen-y Aug 22, 2024

google-oss-prow bot commented Aug 22, 2024

[SDK] fix grpc related bugs in Python SDK #2398

[SDK] fix grpc related bugs in Python SDK #2398

Conversation

Electronic-Waste commented Jul 26, 2024

Electronic-Waste commented Jul 28, 2024

Electronic-Waste commented Jul 29, 2024

Electronic-Waste commented Jul 30, 2024

Electronic-Waste commented Aug 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Electronic-Waste Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

andreyvelich Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreyvelich commented Aug 22, 2024

Electronic-Waste commented Aug 22, 2024

andreyvelich left a comment

Choose a reason for hiding this comment

tenzen-y left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

google-oss-prow bot commented Aug 22, 2024

Electronic-Waste Aug 21, 2024 •

edited

Loading

andreyvelich Aug 22, 2024 •

edited

Loading