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

Update: Replace deps without python wheels to allow installing in pyodide #345

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

Conversation

Gizemdem
Copy link

@Gizemdem Gizemdem commented Aug 5, 2024

Description & motivation

Relates to: https://speckle.community/t/running-specklepy-inside-pyodide-browser/12826/3

This PR replaces two dependencies, which conflict when a user tries to import specklepy in pyodide.
Pyodide is a python port that allows running python programs natively in a browser. This PR would allow installing specklepy in a Python environment running in Pyodide and then extend the possibilities of using these APIs, for example inside a python console running locally in the browser like this one.

Changes:

  • Replace usjon -> orjson. This package is the recommended migration by the ujson package. It's also faster (but haven't tested it personally) and safer. (https://pypi.org/project/ujson/)

Users are encouraged to migrate to orjson which is both much faster and less likely to introduce a surprise buffer overflow vulnerability in the future.

  • orjson dumps produces bytes, so calling .encode() in the base.py file becomes unnecessary. I validated this with the test test_base.py

  • Replace stringcase -> casefy. This dependency has the same API of stringcase but offers a python wheel and is actively maintained. It has no other dependencies.

Screenshots:

image

Validation of changes:

I haven't added a new test, but I focused on running tests in the unit/ directory. Specially the test_base.py since there are changes on the base file due to changing ujson to orjson.
image

Checklist:

  • My pull request follows the guidelines in the Contributing guide?
  • My pull request does not duplicate any other open Pull Requests for the same update/change?
  • My commits are related to the pull request and do not amend unrelated code or documentation.
  • My code follows a similar style to existing code.
  • I have added appropriate tests.
  • I have updated or added relevant documentation.

References

https://speckle.community/t/running-specklepy-inside-pyodide-browser/12826/3

The method orjson.dumps(obj) returns bytes so no need to call encode()
stringcase has not been updated since 2017 and does not have a python wheel. A dependecy maintained and with a similar enough api is casefy
@gjedlicska
Copy link
Collaborator

hey @Gizemdem your changes are in a good direction let me know if you need help with fixing the tests.

The pre-commit CI step is failing, cause one of the linters is not satisfied, if you install pre-commit it will automatically fix the issues for you

Copy link

codecov bot commented Aug 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.65%. Comparing base (fe03d96) to head (0616b9a).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #345   +/-   ##
=======================================
  Coverage   87.65%   87.65%           
=======================================
  Files          95       95           
  Lines        5670     5670           
=======================================
  Hits         4970     4970           
  Misses        700      700           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gjedlicska
Copy link
Collaborator

hey @JR-Morgan and @KatKatKateryna i think we're on solid foundations here to go ahead, could you do a bit of testing in blender and qgis before we merge this?

@andrsbtrg
Copy link

hey everyone! I noticed a small thing: this PR is pinning the minimum verson of orjson to 3.10.6, but the latest version of orjson that works in pyodide is 3.10.1 https://pyodide.org/en/stable/usage/packages-in-pyodide.html. I believe the maintainer will bump the orjson version in pyodide in the future, but it would make more sense to specify the minimum version of orjson here to 3.10.1 in pyproject.toml, so this works ootb with pyodide on day 1. @Gizemdem do you think we can lower the minimum version of orjson?

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.

3 participants