-
Notifications
You must be signed in to change notification settings - Fork 54
Allow creation of standalone sessions #855
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow creation of standalone sessions #855
Conversation
✅ Deploy Preview for neo4j-graph-data-science-client canceled.
|
48bad4b to
ef4cdbe
Compare
DarthMax
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good but I wonder if we need to consider some implications to some functionality that requires a database. What happens if the user for example would run a write query?
| show_progress=False, | ||
| database=db_endpoint.database, | ||
| return cls( | ||
| query_runner=session_bolt_query_runner, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we only use the bolt query runner in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would lead to run_cypher being forwarded to the session wouldnt it? I think we should throw a unsupported operation error or something in such cases
|
Good points, I implemented a dedicated query runner now that blocks all database related operations |
FlorentinD
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM :)
only would like to have an integration test as well. Maybe in test_remote_graph_ops.py but with a graph construct.
- a changelog entry
To test what exactly? :) I don't see what an integration test would give us that the unit test didn't capture in this case. |
ceefe91 to
4856fd2
Compare
I was thinking of making sure its wired up correctly (i.e. such as the previous error in the query runner). |
4856fd2 to
a480d02
Compare
4113949 to
ce2932f
Compare
| raise NotImplementedError | ||
|
|
||
| def database(self) -> Optional[str]: | ||
| raise NotImplementedError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thats accessed on graph projection .. makes me wonder if there is a bug for sessions in general as this database can be viewed as either the db database or the session database (which doesnt make sense as its fixed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is not a bug in sessions. The code that uses the db information looks like this:
if len(info) == 0:
raise ValueError(f"There is no projected graph named '{self.name()}'")
if len(info) > 1:
# for multiple dbs we can have the same graph name. But db + graph name is unique
info = info[info["database"] == self._db]
So only when we are getting more than one result back from graph.list where we specify a graph name we will check the db. That could in theory be failing as we are using the database name from the db query runner, however on a session we cannot have the same graphName on multiple projections so we should be safe here 🤞.
We could do better though by maybe having a special Graph implementation for sessions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively we can split the database method into gds_database and db_database.
With its usage only on list this should never be an issue for the session case.
For sessions we anyway plan to rewrite quite a bit of the client and maybe we will endup with a different Graph implementation as well 🤔
82c0b0d to
b9ab7d1
Compare
No description provided.