Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace JSON serialization with cloudpickle and make reference semantics explicit. #153

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

tbenthompson
Copy link

@tbenthompson tbenthompson commented Feb 6, 2023

See #138 for older discussion.

Copied from the changelog:

  • replaced JSON serialization with cloudpickle. This allows extracting a much
    wider range of objects from the notebook subprocess.
  • Reference semantics have changed.
    • Old behavior of tb.get(name) and tb[name]:
      • a reference would be returned for non-JSON-serializable objects.
      • a value would be returned for JSON-serializable objects.
    • Old behavior of tb.ref(name) was identical to tb.get(name).
    • However, now almost all objects are serializable and as a result, under
      the old semantics, a reference would almost never be returned. Therefore,
      when a reference is desired, we now require explicitly requesting a
      reference. The new behavior of tb.get(name) and tb[name] is to always
      return the deserialized object and to never return a reference. The new
      behavior of tb.ref(name) is to always return a reference.

I think this is a substantial improvement because:

  1. this PR allows serializing a much wider range of objects
  2. makes the API substantially clearer about when a reference will or will not be returned.

I understand that this is a mildly breaking change and adds a dependency and that must be weighed against the benefits. I would argue that the change is very worthwhile!

In my case, I wanted to be able to extract pandas DataFrames and various custom objects from a notebook under test.

@codecov-commenter
Copy link

Codecov Report

Merging #153 (0b65427) into main (62d7bd9) will increase coverage by 6.93%.
The diff coverage is 88.23%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #153      +/-   ##
==========================================
+ Coverage   91.13%   98.07%   +6.93%     
==========================================
  Files           7        8       +1     
  Lines         361      363       +2     
==========================================
+ Hits          329      356      +27     
+ Misses         32        7      -25     

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants