orjson replacement with json package #72
Conversation
| bytes( | ||
| json.dumps( | ||
| { | ||
| "instances": list(all_vms_by_id.values()), | ||
| "timelines": all_timelines, | ||
| } | ||
| ), | ||
| "utf-8", |
There was a problem hiding this comment.
I recall you saying that json dumps to a str while orjson dumps to bytes. Does the bytes format need to be adhered to here because the s3 write requires bytes? Just curious if/how this fails if you don't format it as bytes.
There was a problem hiding this comment.
Yeah, if it's not cast into bytes you get a boto3 error that says something along the lines of file obj needs to be in bytes for s3. I don't recall the exact verbiage, but it was definitely a fatal error
There was a problem hiding this comment.
Well actually, that's true for this line
Line 755 in f97ebc0
TypeError: a bytes-like object is required, not 'str'. It looks like dbfs expects file content to be in b64.
gorskysd
left a comment
gorskysd
left a comment
There was a problem hiding this comment.
Looks like there are a few more orjson usages you missed.
Also, don't forget to bump the version!
Oh man, looks like some files weren't saved but the changes were made locally so search isn't useful. Delightful. I'll go through and close all the tabs in my vscode and make sure they're saved. The good news is I was testing with these changes actually made |
gorskysd
left a comment
gorskysd
left a comment
There was a problem hiding this comment.
Nice, everything looks good to me!
|
This last change fixes a bug where the _monitor_cluster methods were unable to write out the instance data because it needed it's own datetime encoder class in the json utils file. I've done some janky tests where I run diff on the new _monitor_cluster output and everything looks identical. The cli is unaffected, I've tested the clients and the encode_json method is working correctly for both clients. |
gorskysd
left a comment
gorskysd
left a comment
There was a problem hiding this comment.
Ran a successful test on both AWS and Azure. Looks good to me!
Passes all current tests, tested a few of the methods that changed individually (locally) but haven't done an exhaustive test of each method. It seems like everything should be working. The package differences are the options for encoding datetimes and the fact that json.dumps is a str and orjson.dumps is bytes. The echo calls in the cli don't care if the output is bytes or strings, and all other dumps calls have been cast to bytes. The utils folder has a custom class that encodes datetimes the same as orjson.