-
Notifications
You must be signed in to change notification settings - Fork 215
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
feat: add descriptions without modifying model and protocol digest #111
Changes from 19 commits
b4220cc
fba70dc
94ee30d
d5b85f2
9c9b9f4
746146c
0bf470f
9147527
59ea5ac
55d523f
1252445
f3e11f0
163c772
f942268
9860db3
83ffcec
7a3fb7d
1f96584
4c6db8e
e93c2b3
9bf84ac
069d70c
f552b91
0b2a1ff
ccbf531
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -176,7 +176,7 @@ def manifest(self) -> Dict[str, Any]: | |
|
||
for schema_digest, model in all_models.items(): | ||
manifest["models"].append( | ||
{"digest": schema_digest, "schema": model.schema()} | ||
{"digest": schema_digest, "schema": model.schema_no_descriptions} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it guaranteed that this value won't be None? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the refactored version I call the schema_no_descriptions(), that method also uses a class attribute as a cache and that class attribute could possibly be None, but in the schema_no_descriptions() method I guarantee to set the value of that class variable now if that is None. |
||
) | ||
|
||
for request, responses in self._replies.items(): | ||
|
@@ -199,6 +199,12 @@ def manifest(self) -> Dict[str, Any]: | |
encoded = json.dumps(manifest, indent=None, sort_keys=True).encode("utf8") | ||
metadata["digest"] = f"proto:{hashlib.sha256(encoded).digest().hex()}" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. digest is also computed by the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I modified this method accordingly and also created test for it in test_fields_descr.py, please check it. |
||
|
||
manifest["models"] = [] | ||
for schema_digest, model in all_models.items(): | ||
manifest["models"].append( | ||
{"digest": schema_digest, "schema": model.schema()} | ||
) | ||
|
||
final_manifest: Dict[str, Any] = copy.deepcopy(manifest) | ||
final_manifest["metadata"] = metadata | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,87 @@ | ||
import unittest | ||
from pydantic import Field | ||
from uagents import Model, Protocol | ||
|
||
|
||
protocol_no_descr = Protocol(name="test", version="1.1.1") | ||
protocol_with_descr = Protocol(name="test", version="1.1.1") | ||
|
||
|
||
def create_message_no_descr(): | ||
class Message(Model): | ||
message: str | ||
|
||
return Message | ||
|
||
|
||
def create_message_with_descr(): | ||
class Message(Model): | ||
message: str = Field(description="message field description") | ||
|
||
return Message | ||
|
||
|
||
def get_digests(protocol: Protocol): | ||
model_digest = next(iter(protocol.models)) | ||
proto_digest = protocol.digest | ||
return (model_digest, proto_digest) | ||
|
||
|
||
class TestFieldDescr(unittest.TestCase): | ||
def setUp(self) -> None: | ||
self.protocol_no_descr = protocol_no_descr | ||
self.protocol_with_descr = protocol_with_descr | ||
return super().setUp() | ||
|
||
def test_field_description(self): | ||
message_with_descr = create_message_with_descr() | ||
|
||
Model.build_schema_digest(message_with_descr) | ||
|
||
message_field_info = message_with_descr.__fields__["message"].field_info | ||
self.assertIsNotNone(message_field_info) | ||
self.assertEqual(message_field_info.description, "message field description") | ||
|
||
def test_model_digest(self): | ||
model_digest_no_descr = Model.build_schema_digest(create_message_no_descr()) | ||
model_digest_with_descr = Model.build_schema_digest(create_message_with_descr()) | ||
|
||
self.assertEqual(model_digest_no_descr, model_digest_with_descr) | ||
|
||
def test_protocol(self): | ||
@self.protocol_no_descr.on_message(create_message_no_descr()) | ||
def _(_ctx, _sender, _msg): | ||
pass | ||
|
||
self.assertEqual(len(self.protocol_no_descr.models), 1) | ||
self.assertNotIn( | ||
"description", | ||
self.protocol_no_descr.manifest()["models"][0]["schema"]["properties"][ | ||
"message" | ||
], | ||
) | ||
(model_digest_no_descr, proto_digest_no_descr) = get_digests( | ||
self.protocol_no_descr | ||
) | ||
|
||
@self.protocol_with_descr.on_message(create_message_with_descr()) | ||
def _(_ctx, _sender, _msg): | ||
pass | ||
|
||
self.assertEqual(len(self.protocol_with_descr.models), 1) | ||
self.assertEqual( | ||
self.protocol_with_descr.manifest()["models"][0]["schema"]["properties"][ | ||
"message" | ||
]["description"], | ||
"message field description", | ||
) | ||
(model_digest_with_descr, proto_digest_with_descr) = get_digests( | ||
self.protocol_with_descr | ||
) | ||
|
||
self.assertEqual(model_digest_no_descr, model_digest_with_descr) | ||
self.assertEqual(proto_digest_no_descr, proto_digest_with_descr) | ||
|
||
|
||
if __name__ == "__main__": | ||
unittest.main() |
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.
From my point of view these methods shouldn't be static most of the times the model is even passed in why not use
self
then?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 model param passed to these newly created methods is actually not an instance of a Model subclass but rather a Model subclass type (so a class object) that's why I couldn't implement these methods as instance methods (so using self.). However you are right in the sense that these method could be class methods using cls.
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 the refactored version I created class methods instead of static methods where possible, please check it.