-
Notifications
You must be signed in to change notification settings - Fork 59
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
A sketch of DAS integration #446
Conversation
Further work on DAS integration requires some steps on the DAS side, so I'd prefer to merge it for now as a sandbox example |
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 use of the Space API looks fine with the caveats
- The comment RE
remove
andreplace
- I was unable to actually test the code, so I merely read it
I am not able to comment on the use of the DAS protocol.
import re | ||
import json | ||
|
||
from hyperon_das import DasAPI |
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.
Maybe include instructions about where to find the hyperon_das
module / or how to setup & run / connect to the DAS more generally
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.
Unfortunately, their repo is private now, so this PR was more for information than for the in-depth review. Of course, the link to DAS will be provided, once it is open.
class DASpace(AbstractSpace): | ||
|
||
def __init__(self, unwrap=True): | ||
super().__init__() |
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.
Not technically needed since AbstractSpace is pure-virtual, but also harmless.
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.
OK, I see. I'd still keep it here, because if some useful functionality is introduced in the parent class, then it will not be missed in DASpace as well. I'm also ok to remove it if Abstract
part is a strong hint that no concrete functionality will be ever introduced.
else: | ||
self.das.add_link(dc) | ||
|
||
#def remove(self, atom): |
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.
Without remove
and replace
implementations, an exception will be raised if they're called.
Do we want to modify the Space contract so that a Space can gracefully decline to implement these functions in the same way a Space can decline to implement atom_count
and atoms_iter
? My feeling is "no", but you have a more coherent conceptual vision for MeTTa than I do.
It seems to me that remove
and replace
are different from atom_count
and atoms_iter
in that remove
and replace
affect the Space. Even in a space where it's impossible to "forget" (e.g. an append-only journal) it's still possible to indicate that an atom is disused or has been superseded.
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.
Without remove and replace implementations, an exception will be raised if they're called.
This is perfectly ok.
Do we want to modify the Space contract so that a Space can gracefully decline to implement these functions
What behavior will be in this case for remove
, when it is not implemented? I believe it should be exception. A space may not implement some required functionality, but attempts to use the missing functionality should be prevented, and the user should be informed that there is no such functionality and it cannot be used.
in the same way a Space can decline to implement atom_count and atoms_iter?
Do you mean that if atoms_iter
is not implemented, and get-atoms
is executed in MeTTa, a MeTTa Error
-expression is returned, while if remove-atom
is executed the exception is raised, which is not turned into Error
, but the whole MeTTa runner stops? I'm ok with this difference.
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.
if remove-atom is executed the exception is raised, which is not turned into Error, but the whole MeTTa runner stops? I'm ok with this difference.
Does it really work by this way? I believe the exception should be wrapped into Error
as well.
This is a very sketchy and possibly not a canonical (in future) way to integrate DAS into MeTTa, but it might be valuable to have, because there are some potential issues on both sides.