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

Use Rust implementation for clipboard #2079

Merged
merged 3 commits into from
Aug 12, 2023

Conversation

RunDevelopment
Copy link
Member

Fixes #958.

This PR removes the Python clipboard implementation and replaces it with calls into the Rust implementation powered by aboard. As is tradition with Rust, this makes copying images about 2 to 3 times faster on my windows machine. This will hopefully resolve our platform issues. We definitely need to test the new clipboard on our supported platforms before releasing it.

About the new Clipboard API itself: it's pretty simple. It only supports writing text and images, which is enough for us. Clipboard.create_instance() will throw an error if the current platform isn't supported. So we can potentially make a feature for this node if it turns out that aboard don't support some platform.

This is still a draft because I haven't released a new version of chainner_ext with the clipboard API yet.

@joeyballentine
Copy link
Member

Are you sure we shouldn't publish the clipboard functionality separately? I'd kinda like to put an actually good clipboard implementation out as its own package since there just isn't one

@RunDevelopment
Copy link
Member Author

We can do that later. Reading numpy images isn't quite trivial, and so clipboard currently shares a bit of code with the rest of chainner_ext. We would have to copy that code into that project as well. A general clipboard library would also need a more extensive API than just being able to write to the clipboard... Right now, I just implemented the bare minimum we need to get rid of the python clipboard code.

@joeyballentine
Copy link
Member

I almost wonder if we should publish packages under a namespace or something. Like we could have @chainner/ext and @chainner/clipboard, with the rust repo being a monorepo of all the packages

@RunDevelopment
Copy link
Member Author

  1. PyPi doesn't support that AFAIK. The best we can do is sub modules. So e.g. from chainner_ext.clipboard import XYZ.
  2. Do you want the independent clipboard package still be tied to chainner? I'd rather make it a separate entity that just lives in the chainner-org organization. Kinda like aboard isn't called @password1/aboard. Unnecessarily associating that package with chainner will just make people think that it is something specific to chainner that can't be used elsewhere.

@joeyballentine
Copy link
Member

Yeah I'd rather it be its own package

@RunDevelopment RunDevelopment marked this pull request as ready for review August 12, 2023 17:03
@RunDevelopment
Copy link
Member Author

Jup, but later. Right now, I want to see whether it actually works.

@joeyballentine joeyballentine merged commit 673a4a4 into chaiNNer-org:main Aug 12, 2023
11 checks passed
@RunDevelopment RunDevelopment deleted the rust-clipboard branch August 12, 2023 18:45
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.

Pasteboard has no arm64 wheels
2 participants