Implement native calls from Python Driver to Rust Driver #304

dmikhalin · 2023-08-02T17:06:34Z

What is the goal of this PR?

We replace the direct implementation of the client side of the TypeDB protocol in Python with native calls to the Rust client over FFI.

What are the changes implemented in this PR?

merge TypeDBClient and ClusterClient into single Client, following the protocol merge;
remove Remote variants of Concepts; Concept API calls now require a Transaction to be provided;
make Value "generic" over its value type, which allows us to collapse all the Attribute.<Type>s into a single Attribute class;
associate each top-level class (Client, Session, Concept, etc.) with its native representation on the Rust side;
replace Python method implementations with calls using the native representation over FFI.

typedb-bot · 2023-08-02T17:06:36Z

dmitrii-ubskii

Looks good! These changes definitely make the client a lot tidier.

Left a few style comments.

requirements.txt

dmitrii-ubskii · 2023-08-04T09:22:11Z

typedb/api/answer/numeric_group.py

@@ -18,10 +18,14 @@
 # specific language governing permissions and limitations
 # under the License.
 #
+
+from __future__ import annotations


Is this used?

Yes, we import not all classes used in type annotations.

typedb/api/concept/concept_manager.py

typedb/api/concept/thing/relation.py

dmitrii-ubskii · 2023-08-04T09:37:56Z

typedb/api/concept/concept.py

-    from typedb.api.concept.thing.attribute import Attribute, RemoteAttribute
-    from typedb.api.concept.thing.entity import Entity, RemoteEntity
-    from typedb.api.concept.thing.relation import Relation, RemoteRelation
-    from typedb.api.concept.thing.thing import Thing, RemoteThing
-    from typedb.api.concept.type.attribute_type import AttributeType, RemoteAttributeType
-    from typedb.api.concept.type.entity_type import EntityType, RemoteEntityType
-    from typedb.api.concept.type.relation_type import RelationType, RemoteRelationType
-    from typedb.api.concept.type.role_type import RoleType, RemoteRoleType
-    from typedb.api.concept.type.thing_type import ThingType, RemoteThingType
-    from typedb.api.concept.type.type import Type, RemoteType
-    from typedb.api.connection.transaction import TypeDBTransaction
+from typedb.api.concept import thing, type, value


We should probably keep individual imports. That's what we're using everywhere else.

We need it to avoid circular imports.

Method .of became concept_of function in the separate concept_factory.py file.

dmitrii-ubskii · 2023-08-04T10:29:54Z

tests/behaviour/background/environment_base.py

+    context.option_setters = {option: lambda options, value: setattr(options, option.replace("-", "_"), value) for option in
+                              ["session-idle-timeout-millis", "transaction-timeout-millis"]}


We lost int(value) in this change

value type check has been moved to the step parser.

tests/behaviour/concept/serialization/json/BUILD

dmitrii-ubskii · 2023-08-04T10:36:44Z

tests/behaviour/concept/thing/relation/relation_steps.py

            context.get(var2))
        assert False;
-    except TypeDBClientException:
+    except (TypeDBClientException, RuntimeError):


Can this be more specific than RuntimeError?

There is no more specific exceptions in SWIG's list of possible python exceptions.

tests/behaviour/concept/type/thingtype/BUILD

tests/behaviour/context.py

dmikhalin · 2023-08-07T17:09:08Z

typedb/concept/thing/relation.py

-from typedb.concept.thing.thing import _Thing, _RemoteThing
+from typedb.common.streamer import Streamer
+from typedb.concept.concept_factory import relation_type_of, role_type_of, thing_of
+from typedb.concept.thing.thing import _Thing
 from typedb.concept.type.role_type import _RoleType


We don't need this import here, because we use role_type_of(). But without direct importing of role_type.py Bazel think that we don't need it and removes this file. One should investigate this behaviour.

dmikhalin · 2023-08-07T17:12:31Z

typedb/concept/concept_factory.py

We added this file to have all inconvenient module imports in one place.

WORKSPACE

flyingsilverfin · 2023-08-10T08:37:31Z

dependencies/vaticle/repositories.bzl

 def vaticle_dependencies():
    git_repository(
        name = "vaticle_dependencies",
-        remote = "https://github.com/vaticle/dependencies",
-        commit = "51fdda8c935d5f9fc34bfdf41b0a6ac1ca77e2ee", # sync-marker: do not remove this comment, this is used for sync-dependencies by @vaticle_dependencies
+        remote = "https://github.com/dmikhalin/dependencies",


Don't forget to PR this before we merge this PR

tests/behaviour/background/environment_base.py

flyingsilverfin · 2023-08-10T11:15:21Z

tool/behave_rule.bzl

-                --server.address=127.0.0.1:$PORT \
-                --server.internal-address.zeromq=127.0.0.1:$(($PORT+1)) \
-                --server.internal-address.grpc=127.0.0.1:$(($PORT+2)) \
+                --server.address=localhost:$PORT \


I've added an asana task for Dmitrii U to try to upgrade tonic so we can use either localhost or 127.0.0.1, since having that option is pretty nice

tests/behaviour/concept/thing/relation/relation_steps.py

typedb/common/exception.py

typedb/common/streamer.py

BUILD

typedb/concept/value/value.py

flyingsilverfin · 2023-08-15T08:28:05Z

dependencies/vaticle/repositories.bzl

+        remote = "https://github.com/dmikhalin/typedb-client-java",
+        commit = "8b3dac1e497a66a372450ac64e0be1a145ef8f27"  # sync-marker: do not remove this comment, this is used for sync-dependencies by @vaticle_factory_tracing
+    )
+#    native.local_repository(


delete before merging

typedb/api/concept/thing/attribute.py

typedb/api/connection/credential.py

typedb/concept/answer/concept_map.py

typedb/connection/transaction.py

BUILD

typedb/query/query_manager.py

typedb/user/user.py

typedb/user/user_manager.py

tests/behaviour/typeql/typeql_steps.py

typedb/common/native_object_mixin.py

typedb/concept/answer/concept_map.py

typedb/api/connection/credential.py

typedb/api/connection/options.py

This reverts commit f4985e7.

dmikhalin added type: refactor priority: high labels Aug 2, 2023

dmikhalin self-assigned this Aug 2, 2023

dmikhalin requested a review from dmitrii-ubskii August 3, 2023 16:56

typedb-bot assigned dmitrii-ubskii Aug 3, 2023

dmikhalin marked this pull request as ready for review August 3, 2023 16:56

dmikhalin requested review from alexjpwalker and flyingsilverfin as code owners August 3, 2023 16:56

typedb-bot assigned alexjpwalker and flyingsilverfin Aug 3, 2023

dmitrii-ubskii suggested changes Aug 4, 2023

View reviewed changes

dmikhalin requested a review from dmitrii-ubskii August 7, 2023 16:52

dmikhalin commented Aug 7, 2023

View reviewed changes

typedb/concept/concept_factory.py

Copy link

Contributor Author

dmikhalin Aug 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We added this file to have all inconvenient module imports in one place.

dmikhalin mentioned this pull request Aug 10, 2023

Update bazel rule sets and allow python packaging to include generated files typedb/bazel-distribution#382

Merged

flyingsilverfin suggested changes Aug 10, 2023

View reviewed changes

flyingsilverfin reviewed Aug 10, 2023

View reviewed changes

BUILD Outdated Show resolved Hide resolved

flyingsilverfin reviewed Aug 10, 2023

View reviewed changes

BUILD Outdated Show resolved Hide resolved

flyingsilverfin suggested changes Aug 10, 2023

View reviewed changes

BUILD Outdated Show resolved Hide resolved

BUILD Outdated Show resolved Hide resolved

typedb/concept/value/value.py Show resolved Hide resolved

sync-by-unito bot unassigned flyingsilverfin Aug 14, 2023

dmikhalin requested a review from flyingsilverfin August 14, 2023 17:24

typedb-bot assigned flyingsilverfin Aug 14, 2023

flyingsilverfin suggested changes Aug 15, 2023

View reviewed changes

flyingsilverfin reviewed Aug 15, 2023

View reviewed changes

typedb/user/user.py Outdated Show resolved Hide resolved

flyingsilverfin reviewed Aug 15, 2023

View reviewed changes

typedb/user/user.py Outdated Show resolved Hide resolved

flyingsilverfin reviewed Aug 15, 2023

View reviewed changes

typedb/user/user_manager.py Outdated Show resolved Hide resolved

dmikhalin requested a review from flyingsilverfin August 16, 2023 11:46

flyingsilverfin reviewed Aug 17, 2023

View reviewed changes

tests/behaviour/typeql/typeql_steps.py Outdated Show resolved Hide resolved

typedb/common/native_object_mixin.py Outdated Show resolved Hide resolved

typedb/concept/answer/concept_map.py Outdated Show resolved Hide resolved

flyingsilverfin reviewed Aug 17, 2023

View reviewed changes

typedb/api/connection/credential.py Outdated Show resolved Hide resolved

typedb/api/connection/options.py Outdated Show resolved Hide resolved

dmikhalin added 27 commits August 23, 2023 16:58

Revert "Check building only behaviour tests"

8f1ad44

This reverts commit f4985e7.

Disable two tests in BUILD

b5f1e1d

pip_install -> pip_parse

f38b358

Fix PR issues

73b00c9

Use SWIG-generated TypeDBClientException

015b53c

Clean up the errors. Callbacks (wip)

a5e7c92

Streamer -> IteratorWrapper

0018cf5

Rename rules

35337bb

Rename rules and files

19f156e

smth_of -> wrap_smth

389fd89

Check transaction existence on getting native object

d21fa88

Remove redundant error messages

239f2d9

Remove *_explicit methods

057f6c2

Attribute.get_value() returns primitive type

7b64e1f

Remove file path check from Credential

35ff36e

Import fix

f01d140

__repr__ for Explainable

fd4bf0b

Introduce NativeObjectMixin

aa256ab

Rename rules

e16bb79

Optimize imports

0dcf550

_not_blank()

332be1b

Update dependencies

2d1938e

Fix PR issues

71af9a1

Bazel version 6.2.0

8906d52

Update dependencies

499eb42

Update dependencies

1fa1726

Fix rebase

507325f

dmikhalin force-pushed the ffi branch from 8963e19 to 507325f Compare August 23, 2023 16:36

flyingsilverfin merged commit 119a6d7 into typedb:master Aug 23, 2023

flyingsilverfin mentioned this pull request Sep 5, 2023

Client exposes too many methods to create Options object #294

Closed

		context.option_setters = {option: lambda options, value: setattr(options, option.replace("-", "_"), value) for option in
		["session-idle-timeout-millis", "transaction-timeout-millis"]}

Implement native calls from Python Driver to Rust Driver #304

Implement native calls from Python Driver to Rust Driver #304

Uh oh!

Conversation

dmikhalin commented Aug 2, 2023

What is the goal of this PR?

What are the changes implemented in this PR?

Uh oh!

typedb-bot commented Aug 2, 2023 • edited by flyingsilverfin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review Checklist

Uh oh!

dmitrii-ubskii left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

typedb-bot commented Aug 2, 2023 •

edited by flyingsilverfin

Loading