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 #19

Closed

Conversation

Luc-Mcgrady
Copy link
Contributor

@Luc-Mcgrady Luc-Mcgrady commented Aug 22, 2023

Hopefully with this it will make it easier to integrate into the add-on.

Requires Maturin.
Allows you to call train from python as shown in py/test.py.

Run

make test

Or manually create a .venv and run maturin develop

Should run as if it was called from rust
image

@L-M-Sherlock
Copy link
Member

Hopefully with this it will make it easier to integrate into the add-on.

If we integrate FSRS into Anki client, should we still need to integrate the training module into the add-on?

@dae
Copy link
Collaborator

dae commented Aug 22, 2023

I'm afraid I don't think this PR is a good idea.

  • This repo is less than a week old, and the API is still in flux.
  • You've added a requirement on Python. If Anki is to be able to integrate this in the future, the crate needs to be importable by Anki's Rust library, which should work without Python.
  • The plan is to expose training in a way that the UI can show. The current training method requires a console to show progress, which most Anki users will not have.

I would recommend waiting until the API gets decided on, and this gets integrated into Anki's Rust library - then users will be able to access it directly and will not even need an add-on. If you did still want to expose it separately in Python for some reason, once the API is determined, you could do so by creating a separate Rust crate that exposes this crate's public API, like how Anki has a normal Rust crate and a separate pyo3 crate.

@Luc-Mcgrady
Copy link
Contributor Author

Oh are we integrating it into Anki itself? I'd always assumed the add-on was the goal.
This could be integrated into the add-on but if its going to be integrated into Anki itself where the native rust code can call it I agree there's little point to this.

@Luc-Mcgrady
Copy link
Contributor Author

I quickly added it into the addon as a proof of concept.
It does work but:

  • is still limited to the command line progress bar
  • would need different builds for different os's
  • Anki hangs when its running.

image

If you want to test it clone it recursively then run make.

Again if this is going to be added into anki there isnt much point in this.

@dae
Copy link
Collaborator

dae commented Aug 22, 2023

Yep, the plan is to integrate this crate in Anki.

@Luc-Mcgrady
Copy link
Contributor Author

Sounds great to me 👍.

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