Codegen support for bool#1524
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1524 +/- ##
=========================================
+ Coverage 100.0% 100.0% +0.1%
=========================================
Files 228 228
Lines 15629 15658 +29
=========================================
+ Hits 15621 15650 +29
Misses 8 8
Continue to review full report at Codecov.
|
bchen1116
requested review from
ParthivNaresh,
angela97lin,
christopherbunn,
dsherry,
eccabay,
freddyaboulton and
jeremyliweishih
and removed request for
dsherry and
freddyaboulton
freddyaboulton
left a comment
freddyaboulton
left a comment
There was a problem hiding this comment.
@bchen1116 I think this is great! Thanks for making the fix. I left a couple of non-blocking comments about testing and handling non-json-serializable types.
| try: | ||
| json.dumps(v) | ||
| except TypeError: | ||
| raise TypeError(f"Value {v} of type {type(v)} cannot be JSON-serialized") |
There was a problem hiding this comment.
@bchen1116 for what types does this error occur? Should we add test coverage of those cases?
There was a problem hiding this comment.
It errors out for lots of python-specific types. I can add more coverage of that in the tests
4 participants
fix #1495
Add support for pythonic types that JSON doesn't normally support, like boolean and None values.
I didn't add in support/tests for Python objects (like Datetime) or tuples (which get converted to lists by
json.dumpsand remains a list afterjson.loads) since these seem to be edge cases and are unlikely to be params in pipelines/pipeline hyperparameters. Certainly down to discuss this if it is otherwise important to have support for these/other cases!