Skip to content

Fix no_update caching issue (#1014) #1019

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

Closed

Conversation

ProbonoBonobo
Copy link

@ProbonoBonobo ProbonoBonobo commented Nov 21, 2019

Contributor Checklist

optionals

  • I have added entry in the CHANGELOG.md
  • If this PR needs a follow-up in dash docs, community thread, I have mentioned the relevant URLS as follow
    • this github #PR number updates the dash docs
    • here is the show and tell thread in plotly dash community

This closes #1014 by modifying the type signature of dash._NoUpdate to enum.Enum and pointing dash.no_update to an enumerated attribute of this class. This ensures that is still works on cached responses, which currently lose their identity when memoized with pickle.dumps, and gives stronger assurance that a is b returns True when a and b are both aliases of dash.no_update.

Does this change fit within the scope of an existing test fixture? I wanted to add a unit test, but I didn't see a test fixture that pertained to HTTP Responses (and grepping "no_update" returned nothing). To make it easier to test this, it's probably sufficient to check that a no_response object obeys singleton mechanics after pickled to a string, e.g.:

import enum
import pickle

class _NoUpdate(enum.Enum):
    no_update = enum.auto()

no_update = _NoUpdate.no_update
serialized = pickle.dumps(no_update)
deserialized = pickle.loads(serialized)

print(f"deserialized is no_update returns: {deserialized is no_update}")

(Note that it willl print False when executed in a Python REPL due to the implementation of enum.Enum, which binds to the module scope. When executed from a file, though, it prints True as expected.)

@ProbonoBonobo
Copy link
Author

Hmm. It's failing some CI tests because its definition of enum.Enum doesn't include the auto method, which apparently was introduced in Python 3.4.

@alexcjohnson
Copy link
Collaborator

@ProbonoBonobo thanks for bringing up the issue, and for giving the fix a shot! I decided to just go for an isinstance check as mentioned in #1014 (comment) -> #1037

@ProbonoBonobo
Copy link
Author

Probably the better solution, I agree. I wanted the dash.dash.no_update object to behave like a singleton if possible because I periodically pass it around in my own code -- a signal, usually, to suppress execution of server-side update handlers before ultimately returning that same object in the response. In theory the enum object provides that assurance, but resulted in some peculiar errors (Flask related) when I later tried running an application with it. Between the two, making a mental note to identify no_update objects by type rather than identity is the lesser of two evils, so I'll go with it 👍

@alexcjohnson The contributor checklist is super useful by the way, but I'm wondering if Plotly has published a more comprehensive "How To" guide to setting up the testing environment? I had trouble running the primary test suite, even after locating and building dash-renderer-test-components from source (since it's not available in PyPi). No problems running the unit tests in tests/unit/development though.

@alexcjohnson
Copy link
Collaborator

I had trouble running the primary test suite

Ah interesting - dash-renderer-test-components I know about, we've talked about how that should just be brought into this repo but somehow never made an issue for it -> #1039

Is there anything else specific you can share about what additional steps you had to figure out to get the integration tests to run? Or, even better, would you like to make a PR to improve CONTRIBUTING.md? I think that's the right place for it, rather than a separate more complete guide.

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.

[BUG] PlotlyJSONEncoder cannot serialize dash.dash._NoUpdate responses; throws TypeError
2 participants