-
Notifications
You must be signed in to change notification settings - Fork 384
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.
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
[#2113] feat(pyClient): initial Gravitino Python client module #2676
Conversation
Awesome @zhaoyongjie ! It is flexible, I can use the get_child method on each level, and also I can use the the quick way which is to use the child's name to directly accessing it, this is pretty user-friendly. @zhaoyongjie one small thing, we expect to specify the "metalake_name" when initiating the gravitino client object, then after that, user doesn't need to aware the metalake much, directly list or operate on catalogs; We can refactor this later after this PR be merged. :) Codes linter can be added later also; For the unit test, is a mock tool needed? @coolderli @Lanznx Peidian and Brandon, please take a try as well. Thank you! |
It makes sense. I'll create another constructor in GravitinoClient might call
Given that the purpose of this utility is to serve as a client for Gravitino, using mock payloads might limit the scope for realistic testing. To be honest, I prefer integrating Python testing into Gravitino's integration tests and only maintaining a few basic unit tests within the utility itself. |
@zhaoyongjie Greate! Could you add some unit tests? The |
Sure, will add unit tests and updates the correlative document for the changes. |
Add header for setup.py
This reverts commit ee4403d.
protocol: str = "http", | ||
port: int = 8090, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think maybe we didn't need these parameter protocol
and port
.
Because we already have parameter host
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depending on how the Client class is initialized, host
, protocol
, and port
offer an alternative way to define the class
e.g.:
initialize_metalake("example-domain", "example-metalake", protocal="https", port=8080)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi @zhaoyongjie thank you for your reply.
The host
, protocol
, and port
parameters add to the complexity of the interface.
The host
can be either http://localhost:8090
or localhost
, which seems to be very flexible, but actually increases the cost of understanding. We just need to provide a robust host
parameter.
clients/client-python/Makefile
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gravitino use Gradle to compile, so we didn't need this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's for client test and build. Gradle might run it as well if you want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I commit a PR #2753 to support Gradle to build and test Python client module, This way we can use the Gradle command in a unified way.
./gradlew build
./gradlew clean
./gradlew test
So, I sure we didn't need use this Makefile file.
so.....if I hadn't installed Gradle, there might be a block running such
commands. And the other hand that Makefile wants to provide a way to create
venv, ideally, Makefile might be an entry of the CI.
…On Mon, Apr 1, 2024 at 11:35 AM Xun Liu ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
On clients/client-python/Makefile
<#2676 (comment)>
:
I commit a PR #2753 <#2753>
to support Gradle to build and test Python client module, This way we can
use the Gradle command in a unified way.
1. ./gradlew build
2. ./gradlew clean
3. ./gradlew test
—
Reply to this email directly, view it on GitHub
<#2676 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAPMKUSNSP4TFG2SQZ67W43Y3ES75AVCNFSM6AAAAABFH2LGM2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSNZQHE4DINBWG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
Best regards,
Yongjie
|
Developer didn't need install Gradle, Them only need have Java 8, 11, or 17. |
class TestGravitinoMetalake(unittest.TestCase): | ||
def test_gravitino_metalake(self, *args): | ||
metalake = gravitino_metalake("http://localhost:9000", "metalake_demo") | ||
self.assertEqual(metalake.name, "metalake_demo") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd better assert the comment and audit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the class TestGravitinoMetalake(unittest.TestCase):
inherit from unit test that is in Python standard library and removed pytest
, aim to reduce the dependences for the beginning of a project.
BTW, pytest
is also providing assertEqual statements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
… module (apache#2676)" This reverts commit 4411459.
What changes were proposed in this pull request?
The more detailed introductions for the PR has been uploaded on the bilibili, please jump in there to get a quick view.
#2113
Follow the below command
I will follow next steps to refactor/implement the initial Python Client, also heads up @Lanznx was the initial contributor for the Python Client.
requests
since we are using really simple HTTP client to communicate betweenPython script
andGravitino
, thehttp.client
in Python3 is sufficient.Why are the changes needed?
The motivation of the PR intend to introduce an interactivity and exploratory user-experience between Python terminal and Gravitino Server.
Does this PR introduce any user-facing change?
N/A
How was this patch tested?
Unit tests will cover changes.