yWorks / yfiles-jupyter-graphs-for-neo4j

The open-source adapter for working with neo4j graphs and cypher queries in jupyter notebooks leveraging the yFiles Graphs for Jupyter plugin.
https://www.yworks.com/jupyter
MIT License
28 stars 3 forks source link

Optional autocomplete of relationships #3

Closed HEnquist closed 5 months ago

HEnquist commented 5 months ago

This is a simple implementation for autocompleting relationships in the graph. Solves https://github.com/yWorks/yfiles-jupyter-graphs-for-neo4j/issues/1, but it's a simple on/off switch so it does not address the part about providing a list of relationship types.

yGuy commented 5 months ago

Interesting, thanks so much for your contribution!

The fact that this depends on apoc is a bit sad. I don't really understand, why this is necessary, though, to be honest.

@fskpf - I think the implemenation that we are using in App Generator uses a much simpler cypher query to get the additional information and it does not depend on apoc. Can you check out our solution and see if we can merge the ideas. I think doing the filtering to relationship types should be done on the api level, too, before we merge this because otherwise this would be a slightly incompatible API change in the future. We can always just perform the check/filter in python, too, if the cypher query gets too complicated.

HEnquist commented 5 months ago

The fact that this depends on apoc is a bit sad. I don't really understand, why this is necessary, though, to be honest.

Not it's not necessary, the neo4j browser does the same thing without it. But it's very convenient, and apoc is often installed (just not always). My thinking was basically that this works now and could be useful for many others too. It's always possible to improve later.

yGuy commented 5 months ago

Couldn't we just use a cypher query like this?

https://github.com/yWorks/yfiles-for-html-demos/blob/7830f4df3ddd33e3517adb2546a00cf478e99b65/demos/toolkit/neo4j/Neo4jDemo.ts#L302

this would also trivially allow us to specify the types of relationships:

MATCH (n)-[rel]-(m)
            WHERE id(n) IN $ids
            AND id(m) IN $ids
            AND startNode(rel) <> endNode(rel)
            RETURN DISTINCT rel 

or

MATCH (n)-[rel:REL_TYPE1|REL_TYPE1|REL_TYPE2]-(m)
            WHERE id(n) IN $ids
            AND id(m) IN $ids
            AND startNode(rel) <> endNode(rel)
            RETURN DISTINCT rel 

we would then need to add these relationships to the ones returned by the original query, of course.

HEnquist commented 5 months ago

Yes this works, that was actually easier than I initially thought. I have implemented this, but I left out AND startNode(rel) <> endNode(rel) because I want to also include relationships that start and end on the same node.

yGuy commented 5 months ago

Thanks! Great! Is this behavior (showing self-loops) in line with what the neo4j browser does? To a degree I would understand if people would be surprised to see edges when all they do is query a single node. If neo4j does it differently, we should make this configurable, IMHO. Other than that, I hope that we can merge this, @fskpf !

HEnquist commented 5 months ago

Thanks! Great! Is this behavior (showing self-loops) in line with what the neo4j browser does? To a degree I would understand if people would be surprised to see edges when all they do is query a single node.

Yes the neo4j browser also behaves like this, self-loops are shown even if you just match a single node.

fskpf commented 5 months ago

@HEnquist thank you for the PR, I've added some notes, please have a second look. Other than the minor comments, it seems to work as intended.

Another question: What is the pythonic way of using the properties?

In my test case, I used

g._autocomplete_relationships = ["ACTED_IN"]

or

g._autocomplete_relationships = True

But just using g.autocomplete_relationships = True without the leading underscore didn't work. Using the underscore feels like setting a private property to me.

HEnquist commented 5 months ago

@HEnquist thank you for the PR, I've added some notes, please have a second look. Other than the minor comments, it seems to work as intended.

Another question: What is the pythonic way of using the properties?

In my test case, I used

g._autocomplete_relationships = ["ACTED_IN"]

or

g._autocomplete_relationships = True

The _autocomplete_relationships is a private property, you should normally not touch anything with a leading underscore. I implemented this as described in https://github.com/yWorks/yfiles-jupyter-graphs-for-neo4j/issues/1, with a setter method called set_autocomplete_relationships().

But just using g.autocomplete_relationships = True without the leading underscore didn't work. Using the underscore feels like setting a private property to me.

It would be no problem to switch from the setter method to a property (https://docs.python.org/3/library/functions.html#property), so you can do g.autocomplete_relationships = True instead of g.set_autocomplete_relationships(True). The driver property is accessed via a setter and getter, which is a good argument for staying with a setter here (or changing both to properties). Just let me know what you prefer.

fskpf commented 5 months ago

Thanks for the explanation. I'd say keep the setter method for now. We can still switch to properties in a future update then.

I'm not deep into Python, what would you say is more common for libraries? Actual properties or setter/getter methods? If there is a case to change it, we should create a different issue then.

HEnquist commented 5 months ago

Thanks for merging and for the great feedback along the say!

I'm not deep into Python, what would you say is more common for libraries? Actual properties or setter/getter methods? If there is a case to change it, we should create a different issue then.

Both ways are used, I couldn't say which one is the more common. I would say that properties are somewhat more pythonic. But it's questionable if that is reason enough to switch :D