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

Proposal: Simplify Rust Bindings #39

Open
mvisani opened this issue Sep 26, 2024 · 2 comments
Open

Proposal: Simplify Rust Bindings #39

mvisani opened this issue Sep 26, 2024 · 2 comments

Comments

@mvisani
Copy link
Contributor

mvisani commented Sep 26, 2024

Hi @xrl,

I’ve realized that we don’t need to manually rewrite all functions from every class as we’re currently doing. By including the RDKit directory in the build.rs file, CXX can automatically find not just the functions in our wrappers, but also those in the RDKit library itself. This can significantly speed up development since we can directly reference the functions in the cxx::bridge.

I’ve created a small example repository to demonstrate how straightforward this is.

Highlights of the Current Wrapper

wrapper.h:

#pragma once
#include "rust/cxx.h"
#include <GraphMol/Atom.h>
#include <memory>

namespace RDKit {
std::shared_ptr<Atom> make_shared(std::unique_ptr<Atom> atom);
std::unique_ptr<Atom> newAtom();
std::unique_ptr<Atom> newAtomFromAtomicNum(int atomicNum);
std::unique_ptr<Atom> newAtomFromSymbol(const std::string &symbol);
std::unique_ptr<Atom> newAtomFromOther(const Atom &other);
rust::String getSymbolAsString(const Atom &atom);
bool MatchRust(const Atom &atom, std::unique_ptr<Atom> other);
int calcExplicitValence(Atom &atom, bool strict = true);
int calcImplicitValence(Atom &atom, bool strict = true);
} // namespace RDKit

wrapper.cc:

#include "rdkit-rust-ffi/include/wrapper.h"

namespace RDKit {
std::shared_ptr<Atom> make_shared(std::unique_ptr<Atom> atom) { return std::shared_ptr<Atom>(atom.release()); }
std::unique_ptr<Atom> newAtom() { return std::unique_ptr<Atom>(new Atom()); }
std::unique_ptr<Atom> newAtomFromAtomicNum(int atomicNum) { return std::unique_ptr<Atom>(new Atom(atomicNum)); }
// Additional wrapper functions...
}

Using RDKit's Built-in Functions

As seen in the repository, there are more functions listed in lib.rs than in the wrapper, but all are callable and can be used directly. For example, passing self: &Atom allows us to call functions directly like atom.getTotalValence(). This removes the need for boilerplate code like:

pub fn get_is_aromatic(&self) -> bool {
    ro_mol_ffi::get_is_aromatic(self.ptr.as_ref())
}

pub fn get_atomic_num(&self) -> i32 {
    ro_mol_ffi::get_atomic_num(self.ptr.as_ref())
}

This simplifies usage in the rdkit crate and makes the code more maintainable.

Next Steps

We have two options:

  1. Refactor the existing repo to follow this approach. While it may take some time initially, it will speed up development and reduce potential errors.
  2. Continue building on the example repo I’ve created and start a new crate.

What are your thoughts?

Best regards,
Marco

P.S. I’ve added an explanation on how to download and compile RDKit and link the C++ library to our project. This should also add Windows support (although I haven’t tested it yet).

@mvisani mvisani changed the title Proposal: Simplify Rust Bindings by Leveraging RDKit's Existing Functions Proposal: Simplify Rust Bindings Sep 26, 2024
@xrl
Copy link
Contributor

xrl commented Oct 10, 2024

This is very promising if it works as I'm understanding it. I am going to pull down your test repo (very nice idea!) and play around with it.

Are you interested in converting some of the existing bindings to this simplified flavor?

@mvisani
Copy link
Contributor Author

mvisani commented Oct 10, 2024

@xrl I've continued working a bit in this small repo. It actually saves a lot of time ! I'll create a pull request with a couple of modifications in the rdkit-sys crate. I'll wait for your feedback.

This was referenced Oct 10, 2024
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

No branches or pull requests

2 participants