dynamic types support array, dict, ndarray #304
Conversation
mlem/core/data_type.py
Outdated
|
|
||
| type: ClassVar[str] = "d_dict" | ||
|
|
||
| key_type: DataType |
There was a problem hiding this comment.
keys should be immutable. and json keys are always strings. Since you write it as json, how would you recreate arbitrary type?
There was a problem hiding this comment.
Yes. Should have been PrimitiveType. Added key_type as PrimitiveType with validator ptype in [str, int and float].
For json, while reading deserializing the data so that str keys get converted to correct datatype.
Also wanted to ask, do you see any more issue with writing as json? Is there a reason DictWriter does not use json?
There was a problem hiding this comment.
Because values can be not json-serializable (eg, Dict[str, pd.DataFrame])
There was a problem hiding this comment.
if we first serialize(and deserialize) using DataSerializer as done in this PR....values with non primitive data types can also be then json writable and readable...
|
I re-run some failing tests locally, they pass. Must be auth/creds issue. |
|
Ok, I think I found the reason of this auth issue @maheshambule. Let me try to rerun the tests. |
|
@aguschin , Ok based on my observation secrets are not available for forked repo. Facing same issue in other PRs as well |
|
Ok, I was pointed towards this issue iterative/cml#574 by @0x2b3bfa0 |
Codecov Report
@@ Coverage Diff @@
## main #304 +/- ##
==========================================
- Coverage 90.25% 90.16% -0.10%
==========================================
Files 78 79 +1
Lines 5882 6082 +200
==========================================
+ Hits 5309 5484 +175
- Misses 573 598 +25
Continue to review full report at Codecov.
|
|
Finally, this works! Thank you @maheshambule and @0x2b3bfa0. Merging after tests pass! |
|
Looks like there are some issues unrelated to auth. @maheshambule, do you mind checking them out? |
|
looks like windows and numpy issue... working on it... |
|
Great, finally we can merge this! @maheshambule, do you want me to go ahead or you will do it yourself? |
|
It seems I cant merge it as I dont have write access. So please go ahead. |
|
Sorry! Thought you could do that 🤦♂️ |
Fixes #299