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

WIP: Improved Python bindings #3829

Merged
merged 9 commits into from
Oct 16, 2024
Merged

WIP: Improved Python bindings #3829

merged 9 commits into from
Oct 16, 2024

Conversation

ahankinson
Copy link
Contributor

Updates the Python bindings with a few new capabilities:

  • Uses the -fastproxy and -olddeps options. In my testing it made the Python bindings a bit faster, but confirmation would be appreciated
  • Added type hints for all the methods in the bindings using a .pyi file.
  • Corrected some typos and inconsistencies

This option generates a Python binding that executes the proxied C++ code faster.

Adding the "olddeps" option generates a Python proxy with the correct method signatures so that calling code can treat it like Python, but the actual call is done with the proxied property.

Note that another option, "builtin" was also tried, but this broke existing implementations so it was not suitable.

For a comparison of the different methods see: https://www.swig.org/Doc4.2/Python.html#Python_optimization
Unlike the verovio.i file, where the type hints were in the proxy, but no other "direct" methods were typed, the .pyi file contains type hints for all the methods in the Python Verovio binding.

The py.typed file signals that this package is typed. See: https://peps.python.org/pep-0561/
- Added a few methods to ignore that were anyway broken in the Python package and that used ostream arguments (difficult to know what type of data Python expects here)
- The first argument for class methods is, by convention, 'self'. Modified this since the .pyi file used self for the "automatically" proxied code, and "toolkit" for the ones from the .i file.
- Made "xml_id" consistent with "xmlId" arguments in the automatically-proxied code.
- Added a proxy for "ValidatePAEFile" that turns the return string into a Python dictionary (same as "ValidatePAE")
@ahankinson ahankinson changed the title Update swig python Improve Python bindings Oct 15, 2024
@ahankinson ahankinson changed the title Improve Python bindings Improved Python bindings Oct 15, 2024
@ahankinson
Copy link
Contributor Author

WIP: Still needs a few things

@ahankinson ahankinson changed the title Improved Python bindings WIP: Improved Python bindings Oct 15, 2024
@ahankinson ahankinson marked this pull request as draft October 15, 2024 13:29
@rettinghaus
Copy link
Contributor

Maybe then mark it as draft?

@ahankinson
Copy link
Contributor Author

Hm, now why didn't I think of that? ;-D

As far as we know, this is no longer necessary?
@ahankinson ahankinson marked this pull request as ready for review October 15, 2024 15:01
@ahankinson
Copy link
Contributor Author

OK, ready for review now.

Copy link
Contributor

@rettinghaus rettinghaus left a comment

Choose a reason for hiding this comment

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

Looks good to me.

setup.py Show resolved Hide resolved
def initClock(self) -> None: ...
def resetClock(self) -> None: ...
def getRuntimeInSeconds(self) -> float: ...
def logRuntime(self) -> None: ...
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any particular logic to the order? Alphabetical sorting would make hand-editing much easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's the order that's in toolkit.h, which is in semi-alphabetical order.

But the ones at the end are kinda questionable, since these are annotated with @nodocs and don't appear in the Verovio book, so they're not technically "public."

@lpugin lpugin merged commit 8aac867 into develop Oct 16, 2024
9 checks passed
@lpugin lpugin deleted the update-swig-python branch October 16, 2024 10:14
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