RelationalAI / rai-sdk-python

The RelationalAI Software Development Kit (SDK) for Python.
Apache License 2.0
17 stars 4 forks source link

Protobuf metadata #94

Closed NRHelmi closed 2 years ago

NRHelmi commented 2 years ago

This PR adds support to:

/:output/String ('hi',)

/:output/Int64 (1,) (2,) (3,)


* transaction async integration tests added
* CI github workflow
* flake8 linter
denisgursky commented 2 years ago

Hm, string_val: "output" is comes out as a string?

denisgursky commented 2 years ago

Does it make sense to include metadata as a part of results? Basically the same thing as in https://github.com/RelationalAI/rai-sdk-javascript/pull/44

NRHelmi commented 2 years ago

Hm, string_val: "output" is comes out as a string?

I believe it's printed correctly because no json serialization performed here

NHDaly commented 2 years ago

Hm, string_val: "output" is comes out as a string?

are we using python2 or python3? The spec says that bytes should become a "bytes" type in Python3: bytes (Python 3) https://developers.google.com/protocol-buffers/docs/proto3#scalar

NRHelmi commented 2 years ago

Hm, string_val: "output" is comes out as a string?

are we using python2 or python3? The spec says that bytes should become a "bytes" type in Python3: bytes (Python 3) https://developers.google.com/protocol-buffers/docs/proto3#scalar

The SDK uses python 3, python 2 was deprecated on January 1, 2020 :cry:

NRHelmi commented 2 years ago

Does it make sense to include metadata as a part of results? Basically the same thing as in RelationalAI/rai-sdk-javascript#44

@denisgursky since I did also some code refactoring in this PR I think it would be better to address this change in a separate PR (probably when working on ResultTable for this SDK) for the sake of simplicity :pray:

NRHelmi commented 2 years ago

if no other updates are required, I'll merge by the end of the day thank you :pray:

larf311 commented 2 years ago

In the future, please separate out code formatting changes into their own PR. It makes it next to impossible to review the actual changes.

NRHelmi commented 2 years ago

In the future, please separate out code formatting changes into their own PR. It makes it next to impossible to review the actual changes.

Yeah sorry for that :pray: , didn't notice that this PR is getting really complicated to review, maybe a quick guideline on how to review this PR could help:

Please ignore the other changes since they are mainly related to code formatting Thank you :pray:

larf311 commented 2 years ago

Installing from source and trying to run the examples gives me:

➜  rai-sdk-python git:(hnr-metadata-proto) python3 ./examples/get_transaction_metadata.py cf686a5e-7b8f-54c8-ff2b-d8ebdb68618d
Traceback (most recent call last):
  File "/Users/trevorpaddock/Dev/rai-sdk-python/./examples/get_transaction_metadata.py", line 19, in <module>
    from railib import api, config, show
  File "<frozen zipimport>", line 259, in load_module
  File "/usr/local/lib/python3.9/site-packages/rai_sdk-0.6.4-py3.9.egg/railib/api.py", line 24, in <module>
ModuleNotFoundError: No module named 'requests_toolbelt

I need to run:

python3 -m pip install -r requirements.txt

Then everything works. Do we need to add that to the README or do you need to update setup.py?

NRHelmi commented 2 years ago

Thanks @larf311 just updated the README file, it was missing the dependencies installation

pip install -r requirements.txt
NRHelmi commented 2 years ago

If no other updates are required, I'll merge this PR by the end of the day thank you :pray: