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

Python bindings #1234

Closed
wants to merge 11 commits into from
Closed

Python bindings #1234

wants to merge 11 commits into from

Conversation

nicomt
Copy link

@nicomt nicomt commented Mar 7, 2023

I implemented python bindings for a project at work and was wondering if there is interest to have to host this in the main repo.
The bindings are mostly based on the js emscripten native implementation with some minor changes to work with pybind11 and take advantage of Python's memory management.

I had to manually change a couple of lines in the test after they were generated but I assume that is okay because it seems to be the case for other languages too.
4481b1e

@nicomt
Copy link
Author

nicomt commented Mar 7, 2023

Just so you know, the project builds using setuptools with the pybind11 extension to build the project.
If pip 10+ is installed, you should be able to run pip install . or pip wheel . from the root directory to install and build
There is more info here
https://pybind11.readthedocs.io/en/stable/compiling.html#pep-518-requirements-pip-10-required

@NickGerleman
Copy link
Contributor

NickGerleman commented Mar 8, 2023

I implemented python bindings for a project at work and was wondering if there is interest to have to host this in the main repo.

This is some great work, I need to think a little bit more about this.

Bindings contributed to the main repo have historically languished after contribution. Especially when the build was not continuously tested (e.g. I would expect we exercise the build and tests as part of any new bindings as part of GitHub Actions). So, what has historically happened for e.g. the C# bindings is that they were contributed, then any changes to the core engine did not make bindings updates. If we are validating, it also means cross-cutting changes need to change code for more bindings as well.

These builds will also naturally degrade and need maintenance over time. E.g. as language versions get deprecated, OS images change, PyPI tokens get updated (and we would need to manage those as part of getting publish infra back up). So without that maintenance from us, we end up in a state where we have folks with expectations we are supporting this, but we may not have experience to be.

Historically, when the repo saw less attention, going through the main repo was a barrier to innovating on the bindings, or velocity to contribute to them.

On the plus side, it creates exposure to the bindings which are legitimately useful (contributions are shared to one place), and means they are easier to keep in sync with main Yoga.

I think a solution if we did want that level of support is if we clarify a support level on the bindings in the repo which are externally supported. So that we do have a centralized solution, but are more hands off with it, beyond hosting it in the single place. What do you think of this?

@nicomt
Copy link
Author

nicomt commented Mar 8, 2023

I was thinking about this and I'm not sure.
I don't think we can control user expectations, if the code is hosted here people will assume it has official support.
Even if there was a disclaimer I still see people posting issues, pull requests, etc and you and your team would have to engage with all of those things to some degree to keep the community happy.
I can help support this within reason, so if you're okay with the above maybe there is a way we can make it work.
But if that is too much load for you right now I would suggest just having a link in the readme for an unofficial python implementation instead of merging this.

@NickGerleman
Copy link
Contributor

Sorry for the very late response to this (was on vacation for several weeks). But I think my preference for this would be to keep this out of tree. I do like your suggestion of pointing to the project in the Yoga README or Yoga website though (and there are other bindings which might be worth pointing out as well).

Copy link

@ankit-gautam23 ankit-gautam23 left a comment

Choose a reason for hiding this comment

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

I would like to ask to split PR so it will be easy for reviewing.

@nicomt
Copy link
Author

nicomt commented Apr 25, 2023

I would like to ask to split PR so it will be easy for reviewing.

@ankit-gautam23
I'm not sure I understand but maybe looking through the commits would be useful.
https://github.com/facebook/yoga/pull/1234/commits
Not sure if there is another way to split this that makes sense.

@nicomt
Copy link
Author

nicomt commented Apr 25, 2023

Sorry for the very late response to this (was on vacation for several weeks). But I think my preference for this would be to keep this out of tree. I do like your suggestion of pointing to the project in the Yoga README or Yoga website though (and there are other bindings which might be worth pointing out as well).

@NickGerleman
No worries. And it makes sense.
Thanks for all the good work on Yoga!
I'm closing this for now but let me know if there is anything I can help with going forward.

@nicomt nicomt closed this Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants