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 ujson with orjson #29

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

valrus
Copy link

@valrus valrus commented Aug 11, 2024

This PR is an attempt to carry #28 to completion, making the necessary changes to ensure compatibility with orjson and adding tests against both orjson and the stdlib json to ensure that the writer works no matter which is imported.

There were two main pieces necessary for the migration:

  1. orjson doesn't support the sort_keys kwarg to dumps; instead its dumps takes an option kwarg which expects bitwise flags. This PR makes the necessary changes to self._json_dumps_args in JsonRpcStreamWriter.__init__ depending on whether orjson is available. (Unnecessary but hopefully helpful: it also adds the separators=(',', ':') kwarg to the stdlib json case so that it matches orjson's behavior of omitting unnecessary spaces.)
  2. orjson.dumps returns bytes, not a string. This PR encodes the dumped JSON if necessary (i.e. in the stdlib case) and uses bytestring % formatting to populate response in JsonRpcStreamWriter.write.

It was tricky to make sure that this worked with or without orjson. The existing test only tests the orjson case if orjson is available on the system on which the tests are run, but I wanted to be sure to test JsonRpcStreamWriter against the stdlib json too. As such, this PR adds a test_writer_stdlib_json test that uses some funky patching to ensure that JsonRpcStreamWriter.write behaves correctly when orjson is unavailable. It's a bit hacky but I confirmed that it does what's intended in the debugger and it seemed better than risking a developer breaking the writer for users who can't import orjson. (Unfortunately I don't think there's any good way to test the writer against orjson if it can't be imported in the dev's environment!)

@rumpelsepp
Copy link

Would you mind helping me with python-lsp/python-lsp-server#579 as well? That would be wonderful! :)

@valrus
Copy link
Author

valrus commented Aug 12, 2024

@rumpelsepp Yep, was planning to help with that next. I want to push this one over the line first — I see tests are failing.

@valrus
Copy link
Author

valrus commented Aug 12, 2024

The test failure seems unrelated to this change:

Upgrading pip...
Error: dyld[2064]: Library not loaded: /usr/local/opt/gettext/lib/libintl.8.dylib
  Referenced from: <1D2E4423-D3D0-3E57-BCF8-9FA871666A9F> /Users/
Error: runner/hostedtoolcache/Python/3.9.19/x64/bin/python3.9
  Reason: tried: '/usr/local/opt/gettext/lib/libintl.8.dylib' (no such file), '/System/Volumes/Preboot/Cryptexes/OS/usr/local/opt/gettext/lib/libintl.8.dylib' (no such file), '/usr/local/opt/gettext/lib/libintl.8.dylib' (no such file), '/usr/local/lib/libintl.8.dylib' (no such file), '/usr/lib/libintl.8.dylib' (no such file, not in dyld cache)
./setup.sh: line 54:  2064 Abort trap: 6           ./python -m ensurepip
Error: The process '/bin/bash' failed with exit code 134

@ccordoba12 ccordoba12 added this to the v2.0.0 milestone Aug 14, 2024
@ccordoba12 ccordoba12 added the enhancement New feature or request label Aug 14, 2024
@ccordoba12
Copy link
Member

@valrus, thanks for your help with this!

Please rebase on top of the latest develop to get the fixes to our tests in PR #30.

@valrus valrus force-pushed the replace-ujson-with-orjson branch from 0477625 to eec1388 Compare August 17, 2024 00:05
@valrus
Copy link
Author

valrus commented Aug 17, 2024

@ccordoba12 Done; thanks for the fix and sorry for the delay!

@ccordoba12
Copy link
Member

@valrus, it seems that this is ready, thanks!

Now, the next step is to open a PR in the pylsp-server repo that pulls this one and checks our tests pass there with orjson. For that, you can also start with this previous PR from @rumpelsepp: python-lsp/python-lsp-server#579.

@valrus
Copy link
Author

valrus commented Aug 17, 2024

@ccordoba12 OK, I gave it a try: python-lsp/python-lsp-server#591

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

Successfully merging this pull request may close these issues.

3 participants