ritchie46 / lsh-rs

Locality Sensitive Hashing in Rust with Python bindings
MIT License
109 stars 21 forks source link

Does not build #9

Closed ijsnow closed 3 years ago

ijsnow commented 3 years ago

I'm trying to run the examples and it seems like the project doesn't build at the moment. The compiler is reporting a few places where what appears to be a private serde module is being used. Did serde update and remove that export? Or am I missing something in order to import private modules?

error[E0603]: module `export` is private
   --> lsh-rs/src/hash.rs:8:12
    |
8   | use serde::export::PhantomData;
    |            ^^^^^^ private module
    |
note: the module `export` is defined here
   --> /Users/isaac/.cargo/registry/src/github.com-1ecc6299db9ec823/serde-1.0.120/src/lib.rs:275:5
    |
275 | use self::__private as export;
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0603]: module `export` is private
   --> lsh-rs/src/table/sqlite.rs:9:12
    |
9   | use serde::export::PhantomData;
    |            ^^^^^^ private module
    |
note: the module `export` is defined here
   --> /Users/isaac/.cargo/registry/src/github.com-1ecc6299db9ec823/serde-1.0.120/src/lib.rs:275:5
    |
275 | use self::__private as export;
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0603]: module `export` is private
   --> lsh-rs/src/data.rs:4:12
    |
4   | use serde::export::fmt::{Debug, Display};
    |            ^^^^^^ private module
    |
note: the module `export` is defined here
   --> /Users/isaac/.cargo/registry/src/github.com-1ecc6299db9ec823/serde-1.0.120/src/lib.rs:275:5
    |
275 | use self::__private as export;
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^
ritchie46 commented 3 years ago

Hi.. Could you tell me which example you are trying to run? I notice that the blas dependencies can be troublesome.

ijsnow commented 3 years ago

Sorry, coulda been more specific. I get the error when running make build in examples/neural-network

ritchie46 commented 3 years ago

I believe the examples are not up to date anymore indeed. The lsh-rs library still builds on my side (I hope on yours as well). However the examples don't work anymore because my blas libraries don't compile. At the moment I am too busy too work that out. But as the main library still builds I believe it should work with updating some dependencies / toggling off blas. If you believe there are any issues with the main library let me know, I'll figure it out.

bwindsor22 commented 3 years ago

Also seeing this error when I add lsh-rs = "0.4.0" to cargo.toml and run the install.

bwindsor22 commented 3 years ago

Hey, so I managed to compile by pulling the code out of lsh-rs/lsh-rs/src/ and copying into my src, with some module changes.. you can see this commit here if it's helpful https://github.com/bwindsor22/thistle/commit/477cd936d9b3bbb039798142283c8dcc6128e197

As an aside, for vector similarity lookup, you might also check out granne, hnswlib-rs, and hnsw libraries

bwindsor22 commented 3 years ago

@ritchie46 thanks for the comment on blas, was helpful -- thoughts on shipping the examples separately? Something like Cargo.toml -> RemoveMe.Cargo.toml for a quick fix

bwindsor22 commented 3 years ago

@ritchie46 err, looking back, some of the errors that @ijsnow saw were things I fixed locally -- e.g. changing this to std::marker::PhantomData

8 | use serde::export::PhantomData; | ^^^^^^ private module

bwindsor22 commented 3 years ago

... however when git clone and cargo make locally, I do get the blas error. So I think the blas error is an issue, but as I can get the code to compile without solving it, it might be possible to ship a working version of this lib only by quarantining the blas bit and separately solving the serde errors ijsnow was seeing

ritchie46 commented 3 years ago

@ritchie46 err, looking back, some of the errors that @ijsnow saw were things I fixed locally -- e.g. changing this to std::marker::PhantomData

8 | use serde::export::PhantomData; | ^^^^^^ private module

Yes, that's definitly a good one. Could you make a PR for that fix?

ritchie46 commented 3 years ago

uarantining the blas bit and

This make sense, BLAS is only for squeezing out max performance, but should definitely be opt in.

bwindsor22 commented 3 years ago

Great, submitted.