feat: add descriptions without modifying model and protocol digest #111
Conversation
zmezei
changed the title
src/uagents/models.py
Outdated
| orig_descriptions[field_name] = {} | ||
| Model._remove_descriptions(field.type_, orig_descriptions[field_name]) | ||
|
|
||
| @staticmethod |
There was a problem hiding this comment.
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.
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.
In the refactored version I created class methods instead of static methods where possible, please check it.
src/uagents/protocol.py
Outdated
| @@ -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.
Is it guaranteed that this value won't be None?
There was a problem hiding this comment.
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.
| @@ -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.
digest is also computed by the compute_digest static method.
It takes as input the manifest which will contain the descriptions hence producing a different digest.
There was a problem hiding this comment.
I modified this method accordingly and also created test for it in test_fields_descr.py, please check it.
|
Closing this because we will soon be revising the way the model digests are done anyway. |
No description provided.