-
Notifications
You must be signed in to change notification settings - Fork 27
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
Fix mg_graph for the usage under ANALYTICAL mode #220
base: main
Are you sure you want to change the base?
Conversation
@antoniofilipovic & @antepusic any upfront idea on how to support this would be great 👀 🤔 😄 |
class Lang(Enum): | ||
PYTHON = "Python" | ||
CPP = "Cpp" | ||
RUST = "Rust" | ||
|
||
@staticmethod | ||
def from_str(lang_str): | ||
if lang_str.lower() == "python": | ||
return Lang.PYTHON | ||
elif lang_str.lower() == "cpp": | ||
return Lang.CPP | ||
elif lang_str.lower() == "rust": | ||
return Lang.RUST | ||
else: | ||
raise BaseException("Wrong value for the lang parameter.") | ||
|
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 is part of different PR, right?
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.
yes, this is just merged here to make use of it, but in the end this won't be part of this PR
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.
here #226
// TODO(gitbuda): GetWeight fails in the ANALYTICAL, according to the current usage it's ok to return 0, but it | ||
// would be probably be better to return optional. |
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.
In community_detection_online
it seems to me there is Update
function which should propagate changes to graph. I will have a sync with @antepusic to check what is happening on Monday and to have sync about the return value.
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.
@gitbuda I am waiting @antepusic to return and when we sync we will update you here
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
If you have a constant stream of updates + run a global algorithm like community detection + try to update all the nodes, it's very likely that there will be a serialization error in the transactional mode. On the other hand, in the analytical mode, the current error handling inside
cpp/mg_utility/mg_graph.hpp
is not appropriate, thethrow
happens for the wrong reason.Related PR is memgraph/memgraph#771 because it will improve the thrown error messages which are not very useful at the moment.
Insired by
TODOs
GetWeight
callANALYTICAL
andTRANSACTIONAL
modemaster
Recreate
Notes
gensim
andgekko
on Ubuntu 22.04 -> some unit test failtorch
-> if it's not installed via pip -> memgraph fails to start withfree
errorReviewer checklist (the reviewer checks this part)
Module/Algorithm