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

Fix orjson loading error #4562

Merged
merged 7 commits into from
Apr 15, 2024
Merged

Fix orjson loading error #4562

merged 7 commits into from
Apr 15, 2024

Conversation

emilykl
Copy link
Contributor

@emilykl emilykl commented Mar 29, 2024

Closes #3567
Also addresses plotly/dash#2797

Summary

Address the error described in #3567 by adjusting the module loading logic in get_module.

The error can be reproduced by following these steps.

As best I can tell, the error is caused by a race condition between the multiple threads in the Flask hot reloader, and the fact that Python adds a module to sys.modules before the module has been executed [1] [2]. If one thread attempts to load orjson after it has been added to sys.modules but before it has been executed, we'll get the error.

Fix

  • Change logic in get_module so that when should_load=True, we just call import_module right away instead of checking sys.modules first.
  • This resolves the issue because import_module contains logic to verify that the module is fully initialized
  • Logic for when should_load=False remains the same: If the module is in sys.modules we return it, otherwise we return None

Code PR

  • I have read through the contributing notes and understand the structure of the package. In particular, if my PR modifies code of plotly.graph_objects, my modifications concern the codegen files and not generated files.
  • I have added tests (if submitting a new feature or correcting a bug) or
    modified existing tests.
  • For a new feature, I have added documentation examples in an existing or
    new tutorial notebook (please see the doc checklist as well).
  • I have added a CHANGELOG entry if fixing/changing/adding anything substantial.
  • For a new feature or a change in behaviour, I have updated the relevant docstrings in the code to describe the feature or behaviour (please see the doc checklist as well).

@emilykl emilykl requested a review from T4rk1n March 29, 2024 20:52
@emilykl emilykl changed the title Import orjson on initial load Fix orjson loading error Apr 3, 2024
logger.exception(msg)
return sys.modules.get(name, None)

else:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The else is not necessary after return.

Copy link

@T4rk1n T4rk1n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💃

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💃 very nice diagnosis & fix!

@archmoj
Copy link
Contributor

archmoj commented Apr 10, 2024

Some tests are failing on CI.
Over to @LiamConnors 🙏

@emilykl emilykl merged commit b909d48 into master Apr 15, 2024
5 checks passed
@ndrezn ndrezn deleted the fix-3567-orjson-import branch April 15, 2024 14:56
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.

With newer versions of orjson, users need to specify the json engine explicitly (bug?)
4 participants