feat(schema): generate pydantic based jsonschema for any jina proto #2121
Conversation
Latency summaryCurrent PR yields:
Breakdown
Backed by latency-tracking. Further commits will update this comment. |
85c3ef6 to
2a5ae29
Compare
Codecov Report
@@ Coverage Diff @@
## master #2121 +/- ##
==========================================
+ Coverage 88.34% 89.86% +1.52%
==========================================
Files 211 211
Lines 11104 11154 +50
==========================================
+ Hits 9810 10024 +214
+ Misses 1294 1130 -164
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
I like the conversion from protobuf to pydantic. It is more flexible in contrast to just adding the oneof-fields manually. Good work! @deepankarm |
| } | ||
|
|
||
|
|
||
| def protobuf_to_pydantic_model( |
There was a problem hiding this comment.
is it possible to break this down in smaller functions?
| elif f.message_type.name == 'Timestamp': | ||
| # Proto Field Type: google.protobuf.Timestamp | ||
| field_type = datetime | ||
| default_value = datetime.now() |
There was a problem hiding this comment.
Isn't it better to put an stupid default value? Like this we can see if the timestamp is real or not? Normally timestamp is needed to measure some stuff about the presence of an object in a Pod, like this we try the "false positives" of seeing a meaningful timestamp
There was a problem hiding this comment.
Timestamp field needs further polishing. google.protobuf.Timestamp describes time in seconds & nanos, where datetime.now() is in a different format. Since this schema is only exposed to the users for viewing, for now, I would prefer to keep it as-is for now and upgrade as and when required.
| protobuf_fields = protobuf_model.DESCRIPTOR.fields | ||
|
|
||
| if model_name.endswith('Proto') and model_name in PROTO_TO_PYDANTIC_MODELS: | ||
| return PROTO_TO_PYDANTIC_MODELS[model_name] |
There was a problem hiding this comment.
Is this just an optimization?
Is it worth it the fact of introducing a global variable?
There was a problem hiding this comment.
This helps us in 2 ways
- Keeping this global variable adds all schemas to openapi docs. Hence, auto-rendering of all Jina protos in jsonschema for both swagger & redoc
- Easier handling of recursive schemas.
No description provided.