Python client v3 (UASTv2)

[dennwc]

Work-in-progress implementation of Python client v3 for the new libuast.

TODO:

• Proper memory management
• Fix install scripts
• Update gRPC client to use v1 and v2 protocols
• Test that the static linking works in Windows and macos and include static files in the libuast package.
• Tests
• Fix some TO-DOs in the code about potential memory problems.
• Update README.md
• Enable failing test testFilterToken once the SDK problem has been fixed.
• Add Node compatibility layer.

Signed-off-by: Denys Smirnov denys@sourced.tech

[juanjux]

Install script should be fixed.

[bzz]

Was trying to test bblfshd/client-python.v3 locally (have libuast.3-rc2 globally installed)

virtualenv -p python3 .venv3
source .venv3/bin/activate
pip3 install -r requirements.txt
 edit setup.py // GET_LIBUAST = False
python3 setup.py install --getdeps --log
python3 setup.py install //build seems to finish fine

but then

python3 -c "import bblfsh"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "~/src-d/bblfsh/client-python/bblfsh/__init__.py", line 1, in <module>
    from bblfsh.client import BblfshClient
  File "~/src-d/bblfsh/client-python/bblfsh/client.py", line 6, in <module>
    from bblfsh.aliases import ParseRequest, DriverStub
  File "~/src-d/bblfsh/client-python/bblfsh/aliases.py", line 15, in <module>
    Node = uast_module.Node
AttributeError: module 'bblfsh.gopkg.in.bblfsh.sdk.v2.uast.generated_pb2' has no attribute 'Node'

@juanjo I belive it has to do with 24fd7b6 as bblfsh.gopkg.in.bblfsh.sdk.v2.uast.generated_pb2 indeed does not have Node, only v1 has.

When I manually roll it back to aliases.py it seems to work.

[juanjux]

@dennwc PTAL at the last commit. Should work on Linux (works on my machine and my Docker image) but as we talked on Slack it'll require the libuast package to include static libraries and it's untested on Windows and Darwin. It also have GET_LIBUAST to False to ease testing this, should be changed before merging.

[juanjux]

@bzz everything in your review should be solved.

[bzz]

@vmarkovtsev would you be so kind to please review the API changes? Thanks!

[zurk]

My minor comments and questions. Main part looks good 👍 .

[vmarkovtsev]

This is not very Pythonic. IDK if it is hard to guess the type, but the following is more Pythonic:

x = next(ctx.filter("boolean(//*[@strtOffset or @endOffset])").iterate())
for name in ctx.filter("name(//*[1])").iterate():
    print(name)

Where iterate() would be an unordered iterator over the resulting values. Or name it results(). Or remove it and iterate directly, anyway.

[juanjux]

Those are typed functions for queries returning boolean/string/integer/float values instead of nodes so the "typed get" is needed when the user doesn't have to do isinstance of the results to check that they're right every time (but if he wants, he can use the normal get()).

[vmarkovtsev]

We should use an enum class or define separate constants instead of raw strings.

[juanjux]

It's mapping the protocol_v2_module.Mode.DESCRIPTOR.values_by_name field. Maybe we could generate variable names from these strings using eval or exec but that's almost worse.

[vmarkovtsev]

So given that we no longer have a Node, we have to rewrite literally everything on our side, because Node is used everywhere in ML code, in nearly half of the files. Alternatively, we will write our own emulation wrapper because we heavily rely on that abstraction anyway and don't want to spend months on debugging very very tricky stuff bound to nodes :)

[vmarkovtsev]

Daaaamn, we have to rewrite bloody months of work in the style-analyzer's feature extraction without this emulation. Very painful.

[juanjux]

@vmarkovtsev That was my point of view, when I disliked the breaking changes. It doesn't make sense for you to write a Node compatibility layer because if there is any it should be here if it's needed (from bblfsh.past import Node or something like that).

[dennwc]

Re "typed get" issue mentioned above, I agree with @vmarkovtsev - I don't see the reason for typed get helpers. Python is a dynamic language, so it should be okay to iterate over nodes directly (that already are strings/ints/whatever). Old client had those helpers because the way libxml was wrapped. It's not necessary right now.

[juanjux]

Python is dynamically typed but not weakly typed so it's not considered good practice on Python to have variant return types from the same function other than None. Also, having typed functions make easier to use mypy and avoid doing "if isinstance(...)" on every value, which would also be slower (or risking having buggy code using types without checking them). Plus is basically free to have them on the API.

[dennwc]

@juanjux OK, thanks for clarifying it. I trust your decision here.

[juanjux]

Update: I started to work on a compatiblity layer with v2 before the holidays, but it's stopped to work on the very prioritary C# driver, I'll continue with it ASAP once the driver is finished so we can finally publish this version.

[juanjux]

Since this passed review from the team, I propose to merge it (WITHOUT releasing v3 yet) so the compatiblity layer PR will be much easier to review as a separate one. Both PRs, once merged, would then be released as V3. What do you think?

[creachadair]

Since this passed review from the team, I propose to merge it (WITHOUT releasing v3 yet) so the compatiblity layer PR will be much easier to review as a separate one. Both PRs, once merged, would then be released as V3. What do you think?

That sounds reasonable to me.

[dennwc]

Sounds good. Let's merge it.