Page MenuHomeSoftware Heritage
Differential D1991

Reimplement REST API in Python with Py4J + aiohttp
ClosedPublic
Actions

Authored by seirl on Sep 17 2019, 2:18 AM.

Details

Reviewers
zack
haltode
Commits
rDGRPHa7a41c37591e: api server: add links to API doc to index
rDGRPHdc62ac9e6980: api client: handle stream decoding in swh.graph
rDGRPH30007537ac2c: cli: fix rpc-serve default port

Diff Detail

Repository
rDGRPH Compressed graph representation
Branch
aiohttp_server
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 7984
Build 11505: tox-on-jenkinsJenkins
Build 11504: arc lint + arc unit

Event Timeline

seirl created this revision.Sep 17 2019, 2:18 AM
Harbormaster failed remote builds in B7804: Diff 6705!Sep 17 2019, 2:19 AM
swh-public-ci added a comment.Sep 17 2019, 2:19 AM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DGRPH/job/tox/48/
See console output for more information: https://jenkins.softwareheritage.org/job/DGRPH/job/tox/48/console

zack requested changes to this revision.Sep 17 2019, 9:17 AM

In addition to the minor comment above, a few bare-bone tests that spawn the aiohttp daemon and try to actually query would be really nice. The toolchain has several moving parts, I can easily see it breaking unexpectedly.

@haltode: I didn't look much at the Java side, can you please have a look when you have a moment? TIA

swh/graph/server/__main__.py
42

So, what is the return format here?
Content-Type says application/json, but the PIDs are streamed as newline-separated.

Is this line-delimited JSON, and would it actually work with common JSON tools, e.g., jq?

I'm fine with any approach to stream JSON, but we should make sure it is not an ad hoc format hard to use in practice.

swh/graph/server/backend.py
32–33

these suffixes should be constants

35

left-over comment

123

A prefix="swh-graph." for TemporaryDirectory would be nice here. (Yes, it's already in the FIFO name later, but having it in the dir name would be nice to ls /tmp users.

This revision now requires changes to proceed.Sep 17 2019, 9:17 AM
zack added a comment.Sep 17 2019, 9:18 AM

FWIW, Jenkins fails on this diff with:

Checking patch java/server/src/main/java/org/softwareheritage/graph/Entry.java...
error: java/server/src/main/java/org/softwareheritage/graph/Entry.java: does not exist in index

Looks like a spurious CI error?

seirl added a comment.Sep 18 2019, 7:45 PM

Yes, the CI cherrypicked the commit with an incorrect base.

swh/graph/server/__main__.py
42

I think you might have misread the code, I don't set the Content-Type to application/json anywhere for these views.

seirl updated this revision to Diff 6746.Sep 18 2019, 7:59 PM

Fix review

Harbormaster failed remote builds in B7854: Diff 6746!Sep 18 2019, 7:59 PM
swh-public-ci added a comment.Sep 18 2019, 7:59 PM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DGRPH/job/tox/49/
See console output for more information: https://jenkins.softwareheritage.org/job/DGRPH/job/tox/49/console

seirl marked 4 inline comments as done.Sep 18 2019, 7:59 PM
zack accepted this revision.Sep 19 2019, 9:37 AM
zack added inline comments.
swh/graph/server/__main__.py
42

oh it was only for stats indeed, my bad

This revision is now accepted and ready to land.Sep 19 2019, 9:37 AM
haltode added inline comments.Sep 19 2019, 10:04 AM
java/server/src/main/java/org/softwareheritage/graph/Entry.java
30–32

We need the snake case conversion to be applied to *all* endpoints, not only the stats. For example in traversal endpoints you have the nbEdgesAccessed metadata that needs to be transformed in snake case.

seirl updated this revision to Diff 6848.Sep 27 2019, 4:51 PM
  • Move example dataset in common tests/dataset directory, add binary maps
  • Server API: handle visits of paths
  • Server API: reorganize app/main
  • API client: update to use the new streaming lines format
  • Update tests to work with the new Python server
seirl updated this revision to Diff 6849.Sep 27 2019, 4:54 PM

rebase

seirl updated this revision to Diff 6850.Sep 27 2019, 4:55 PM

rebase

Harbormaster failed remote builds in B7984: Diff 6848!Sep 27 2019, 5:07 PM
swh-public-ci added a comment.Sep 27 2019, 5:07 PM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DGRPH/job/tox/54/
See console output for more information: https://jenkins.softwareheritage.org/job/DGRPH/job/tox/54/console

Harbormaster failed remote builds in B7985: Diff 6849!Sep 27 2019, 5:07 PM
swh-public-ci added a comment.Sep 27 2019, 5:07 PM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DGRPH/job/tox/55/
See console output for more information: https://jenkins.softwareheritage.org/job/DGRPH/job/tox/55/console

Harbormaster failed remote builds in B7986: Diff 6850!Sep 27 2019, 5:08 PM
swh-public-ci added a comment.Sep 27 2019, 5:08 PM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DGRPH/job/tox/56/
See console output for more information: https://jenkins.softwareheritage.org/job/DGRPH/job/tox/56/console

seirl updated this revision to Diff 6851.Sep 27 2019, 5:33 PM

Fix test

Harbormaster failed remote builds in B7987: Diff 6851!Sep 27 2019, 5:33 PM
swh-public-ci added a comment.Sep 27 2019, 5:33 PM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DGRPH/job/tox/57/
See console output for more information: https://jenkins.softwareheritage.org/job/DGRPH/job/tox/57/console

seirl updated this revision to Diff 6852.Sep 27 2019, 6:44 PM

java: remove useless StreamTraversal.java

Harbormaster failed remote builds in B7988: Diff 6852!Sep 27 2019, 6:44 PM
swh-public-ci added a comment.Sep 27 2019, 6:44 PM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DGRPH/job/tox/58/
See console output for more information: https://jenkins.softwareheritage.org/job/DGRPH/job/tox/58/console

seirl added a parent revision: D2039: api client: add a stream_lines option.Sep 27 2019, 6:51 PM
zack requested changes to this revision.Sep 27 2019, 8:28 PM

Nice work!

I've only commented on minor stuff here and there.

java/server/src/main/java/org/softwareheritage/graph/Entry.java
30–32

Ping on this comment by @haltode. I think you two discussed it on IRC after it, but I'm not 100% sure.

67

a named constant would be nice(r) than -1

75

The FIFO is created by the client, right? (if not, we have a race condition…)

If so this message should be something like "Cannot open client FIFO".

swh/graph/cli.py
106

(nice catch, thanks)

swh/graph/server/app.py
1

the usual copyright/license header is missing here

… and while we are at it, a brief docstring stating that this is a proxy server talking to java via py4j + fifo would be welcome too.

16

maybe add a link to https://docs.softwareheritage.org/devel/swh-graph/api.html in the response body?

swh/graph/server/backend.py
51

I've seen this list of simple traversal method names at least twice. Would be nice to have a constant with the list in one of the two module, and used in both cases.

55

… oh, look, here's another place where you can reuse the path separator constant :-)

74

we have this list in swh.model.identifiers.PID_TYPES

This revision now requires changes to proceed.Sep 27 2019, 8:28 PM
seirl marked 2 inline comments as done.Sep 28 2019, 1:01 PM
seirl added inline comments.
java/server/src/main/java/org/softwareheritage/graph/Entry.java
30–32

Yes, this comment is not applicable to my code because I no longer return these metadata in the streaming APIs

75

Yes, good catch.

zack added inline comments.Sep 28 2019, 3:03 PM
swh/graph/server/__main__.py
15

Even if you don't personally like click, it is the standard in the swh codebase and uniformity rocks.
So please drop argparse in favor of click. (Sorry.)

18

Given the multi-file on disk representation of compressed graph, users will have no idea what they should pass here. The help strings should be expanded to explain that.

I've tried passing the basename (e.g., all, for a graph stored as all.{compressed,obl,…}, but it didn't work and there was no meaningful error message.

zack added inline comments.Sep 28 2019, 3:42 PM
swh/graph/server/__main__.py
15

Oh, and in fact this main.py should be branched as a subcommand of the existing swh graph in cli.py.
Other modules use swh MODULE rpc-serve for similar needs.

seirl marked 12 inline comments as done.Sep 30 2019, 5:21 PM
seirl updated this revision to Diff 6863.Sep 30 2019, 5:24 PM
  • server: proper JAR deployment method
  • server: refactor some constants
  • server: refactor handler proxying to backed
  • server: move serve command to cli.py
Harbormaster failed remote builds in B8005: Diff 6863!Sep 30 2019, 5:25 PM
swh-public-ci added a comment.Sep 30 2019, 5:25 PM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DGRPH/job/tox/59/
See console output for more information: https://jenkins.softwareheritage.org/job/DGRPH/job/tox/59/console

zack requested changes to this revision.Sep 30 2019, 8:20 PM

looks great ! I've only noticed minor stuff (this time for real ;-))

Makefile.local
4 ↗(On Diff #6863)

maybe it's just phabricator playing tricks here, but I see two spaces while it should be 1 TAB

setup.py
39 ↗(On Diff #6863)

I'm a bit worried about what will happen if multiple *.jar files, with different version numbers (e.g., due to previously built versions), are around.

Does mvn compile ensure that previous versions will be removed? Is there a way to make it so?

swh/graph/cli.py
127

looks like a copy-paste error in the port number, it should be 5009

swh/graph/server/app.py
16

this is marked as done but it's not actually done in the diff (?)

This revision now requires changes to proceed.Sep 30 2019, 8:20 PM
seirl updated this revision to Diff 6883.Oct 1 2019, 10:02 AM
seirl marked an inline comment as done.
  • cli: fix rpc-serve default port
  • api client: handle stream decoding in swh.graph
  • api server: add links to API doc to index
Harbormaster failed remote builds in B8028: Diff 6883!Oct 1 2019, 10:02 AM
swh-public-ci added a comment.Oct 1 2019, 10:02 AM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DGRPH/job/tox/60/
See console output for more information: https://jenkins.softwareheritage.org/job/DGRPH/job/tox/60/console

seirl added inline comments.Oct 1 2019, 10:03 AM
Makefile.local
4 ↗(On Diff #6863)

Yes, it's already a tab.

setup.py
39 ↗(On Diff #6863)

Here we just install everything. I think the proper way of doing that, theoretically, is to sync the versions between maven and the pypi package through a version.txt or similar, and then just load this one. I can investigate how to do that with our current setup.

zack accepted this revision.Oct 1 2019, 10:07 AM
This revision is now accepted and ready to land.Oct 1 2019, 10:07 AM
This revision was landed with ongoing or failed builds.Oct 1 2019, 10:10 AM
Closed by commit rDGRPH30007537ac2c: cli: fix rpc-serve default port (authored by seirl). · Explain Why
This revision was automatically updated to reflect the committed changes.
seirl added a commit: rDGRPH30007537ac2c: cli: fix rpc-serve default port.
seirl added a commit: rDGRPHdc62ac9e6980: api client: handle stream decoding in swh.graph.
seirl added a commit: rDGRPHa7a41c37591e: api server: add links to API doc to index.

Revision Contents
Changeset List

PathSize
java/
server/
5 lines
src/
main/
java/
org/
softwareheritage/
graph/
137 lines
algo/
11 lines
14 lines
12 lines
77 lines
91 lines
backend/
42 lines
t/
24 lines
43 lines
24 lines
test/
dataset/
img/
output/
java/
org/
softwareheritage/
graph/
2 lines
1 line
swh/
graph/
2 lines
29 lines
server/
29 lines
79 lines
157 lines
tests/
40 lines
293 lines
tests/
dataset/
java/
server/
src/
test/
dataset/
4 lines
23 lines
21 lines
27 lines
img/
1 line
9 lines
82 lines
output/
35 lines
4 lines
21 lines
4 lines
21 lines
35 lines
20 lines
tests/
dataset/
output/

Diff 6848

View Options

java/server/pom.xml

Loading...
View Options

java/server/src/main/java/org/softwareheritage/graph/Entry.java

Loading...
View Options

java/server/src/main/java/org/softwareheritage/graph/algo/NodeIdConsumer.java

Loading...
View Options

java/server/src/main/java/org/softwareheritage/graph/algo/NodeIdsConsumer.java

Loading...
View Options

java/server/src/main/java/org/softwareheritage/graph/algo/PathConsumer.java

Loading...
View Options

java/server/src/main/java/org/softwareheritage/graph/algo/StreamTraversal.java

Loading...
View Options

java/server/src/main/java/org/softwareheritage/graph/algo/Traversal.java

Loading...
View Options

java/server/src/main/java/org/softwareheritage/graph/backend/Pp.java

Loading...
View Options

java/server/src/main/java/org/softwareheritage/graph/t/WriteLong.java

Loading...
View Options

java/server/src/main/java/org/softwareheritage/graph/t/aiohttp_rest.py

Loading...
View Options

java/server/src/main/java/org/softwareheritage/graph/t/data.bin

Loading...
View Options

java/server/src/main/java/org/softwareheritage/graph/t/read_long.py

Loading...
View Options

java/server/src/test/dataset/.gitignore

Loading...
View Options

java/server/src/test/dataset/example.edges.csv

Loading...
View Options

java/server/src/test/dataset/example.edges.csv.gz

Loading...
View Options

java/server/src/test/dataset/example.nodes.csv

Loading...
View Options

java/server/src/test/dataset/example.nodes.csv.gz

Loading...
View Options

java/server/src/test/dataset/generate_graph.sh

Loading...
View Options

java/server/src/test/dataset/img/.gitignore

Loading...
View Options

java/server/src/test/dataset/img/Makefile

Loading...
View Options

java/server/src/test/dataset/img/example.dot

Loading...
View Options

java/server/src/test/dataset/output/example-transposed.graph

Loading...
View Options

java/server/src/test/dataset/output/example-transposed.obl

Loading...
View Options

java/server/src/test/dataset/output/example-transposed.offsets

Loading...
View Options

java/server/src/test/dataset/output/example-transposed.properties

Loading...
View Options

java/server/src/test/dataset/output/example.graph

Loading...
View Options

java/server/src/test/dataset/output/example.indegree

Loading...
View Options

java/server/src/test/dataset/output/example.mph

Loading...
View Options

java/server/src/test/dataset/output/example.node2pid.csv

Loading...
View Options

java/server/src/test/dataset/output/example.node2type.map

Loading...
View Options

java/server/src/test/dataset/output/example.obl

Loading...
View Options

java/server/src/test/dataset/output/example.offsets

Loading...
View Options

java/server/src/test/dataset/output/example.order

Loading...
View Options

java/server/src/test/dataset/output/example.outdegree

Loading...
View Options

java/server/src/test/dataset/output/example.pid2node.csv

Loading...
View Options

java/server/src/test/dataset/output/example.properties

Loading...
View Options

java/server/src/test/dataset/output/example.stats

Loading...
View Options

java/server/src/test/java/org/softwareheritage/graph/GraphTest.java

Loading...
View Options

requirements.txt

Loading...
View Options

swh/graph/cli.py

Loading...
View Options

swh/graph/client.py

Loading...
View Options

swh/graph/server/__main__.py

Loading...
View Options

swh/graph/server/app.py

Loading...
View Options

swh/graph/server/backend.py

Loading...
View Options

swh/graph/tests/conftest.py

Loading...
View Options

swh/graph/tests/test_api_client.py

Loading...
View Options

tests/dataset/.gitignore

Loading...
View Options

tests/dataset/example.edges.csv

Loading...
View Options

tests/dataset/example.edges.csv.gz

Loading...
View Options

tests/dataset/example.nodes.csv

Loading...
View Options

tests/dataset/example.nodes.csv.gz

Loading...
View Options

tests/dataset/generate_graph.sh

Loading...
View Options

tests/dataset/img/.gitignore

Loading...
View Options

tests/dataset/img/Makefile

Loading...
View Options

tests/dataset/img/example.dot

Loading...
View Options

tests/dataset/output/example-transposed.graph

Loading...
View Options

tests/dataset/output/example-transposed.obl

Loading...
View Options

tests/dataset/output/example-transposed.offsets

Loading...
View Options

tests/dataset/output/example-transposed.properties

Loading...
View Options

tests/dataset/output/example.graph

Loading...
View Options

tests/dataset/output/example.indegree

Loading...
View Options

tests/dataset/output/example.mph

Loading...
View Options

tests/dataset/output/example.node2pid.bin

Loading...
View Options

tests/dataset/output/example.node2pid.csv

Loading...
View Options

tests/dataset/output/example.node2type.map

Loading...
View Options

tests/dataset/output/example.obl

Loading...
View Options

tests/dataset/output/example.offsets

Loading...
View Options

tests/dataset/output/example.order

Loading...
View Options

tests/dataset/output/example.outdegree

Loading...
View Options

tests/dataset/output/example.pid2node.bin

Loading...
View Options

tests/dataset/output/example.pid2node.csv

Loading...
View Options

tests/dataset/output/example.properties

Loading...
View Options

tests/dataset/output/example.stats

Loading...
About · Developer information · Legal