wrapping problem on transition from 5 to 6 arguments

progman1 commented 4 years ago

I have a 5 arg function that became 6:

pub fn neuro_model_fit(
        mut net: ONetwork,
        data: OTabularDataSet,
        batch_size: u64,
        epochs: u64,
        print_loss: Option<u64>,
        metrics..... new additional argument)...

which then yielded the error:

error: `mut` on a binding may not be repeated
   --> src/lib.rs:333:2
    |
333 |     mut net: ONetwork,
    |     ^^^ help: remove the additional `mut`s

which persisted after update to ocaml-0.13.2 and ocaml-derive-0.13.1

zshipko commented 4 years ago

Okay, I've just made another release after reproducing the issue. It should be fixed now but let me know if you're still having issues!

progman1 commented 4 years ago

this bug has some resilience!

Fatal error: exception Failure("assertion failed: 6usize ==
__ocaml_argc as usize")

emanating from ocaml-derive/src/lib.rs, line 417

I also get this:

warning: variable does not need to be mutable
   --> src/lib.rs:333:2
    |
333 |     mut net: ONetwork,
    |     ----^^^
    |     |
    |     help: remove this `mut`
    |
    = note: `#[warn(unused_mut)]` on by default

but in fact it does need to be mutable.

On 26/06/2020, zach notifications@github.com wrote:

Okay, I've just made another release after reproducing the issue. It should be fixed now but let me know if you're still having issues!

-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/zshipko/ocaml-rs/issues/29#issuecomment-650241891

zshipko commented 4 years ago

hm interesting, is your code available for me to look at? Or could you just paste the whole function that's causing issues?

zshipko commented 4 years ago

I haven't been able to reproduce that issue with the assert!, but I've created a reproduction/fix for the other issue here: https://github.com/zshipko/ocaml-rs/commit/5d2df515fde779ea0cd8d6c53373f9d41c23955e

Let me know what happens when using the latest from master

progman1 commented 4 years ago

the mut moan has gone :) and the assertion has disguised itself as another:

Fatal error: exception Failure("assertion failed: `(left == right)`
  left: `6`,
 right: `7`")

I can't see where that is coming from so easily...

On 26/06/2020, zach notifications@github.com wrote:

I haven't been able to reproduce that issue with the assert!, but I've created a reproduction of the other issue here: https://github.com/zshipko/ocaml-rs/commit/5d2df515fde779ea0cd8d6c53373f9d41c23955e

Let me know what happens when using the latest from master

-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/zshipko/ocaml-rs/issues/29#issuecomment-650332303

progman1 commented 4 years ago

I think it's the same though.

On 26/06/2020, kris cq.personal@gmail.com wrote:

the mut moan has gone :) and the assertion has disguised itself as another:
Fatal error: exception Failure("assertion failed: `(left == right)`
  left: `6`,
 right: `7`")
I can't see where that is coming from so easily...

On 26/06/2020, zach notifications@github.com wrote:

I haven't been able to reproduce that issue with the assert!, but I've created a reproduction of the other issue here: https://github.com/zshipko/ocaml-rs/commit/5d2df515fde779ea0cd8d6c53373f9d41c23955e

Let me know what happens when using the latest from master

-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/zshipko/ocaml-rs/issues/29#issuecomment-650332303

progman1 commented 4 years ago

here's the func:

#[ocaml::func]
pub fn neuro_model_fit(
        mut net: ONetwork,
        data: OTabularDataSet,
        batch_size: u64,
        epochs: u64,
        print_loss: Option<u64>,
    _metrics: Option<ocaml::Value>
) -> ocaml::Value {
        let mut net=net.as_mut() ;
        let data=data.as_ref();
        net.fit(
                data,
                batch_size,
                epochs,
                print_loss,
                Some(vec![metrics::Metrics::Accuracy])
        );
        UNIT
}

On 26/06/2020, kris cq.personal@gmail.com wrote:

I think it's the same though.

On 26/06/2020, kris cq.personal@gmail.com wrote:
the mut moan has gone :) and the assertion has disguised itself as another:
Fatal error: exception Failure("assertion failed: `(left == right)`
  left: `6`,
 right: `7`")
I can't see where that is coming from so easily...

On 26/06/2020, zach notifications@github.com wrote:

I haven't been able to reproduce that issue with the assert!, but I've created a reproduction of the other issue here: https://github.com/zshipko/ocaml-rs/commit/5d2df515fde779ea0cd8d6c53373f9d41c23955e

Let me know what happens when using the latest from master

-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/zshipko/ocaml-rs/issues/29#issuecomment-650332303

zshipko commented 4 years ago

Yeah, I just changed the macro to give a little more information. It looks like you may be calling that function with 7 arguments instead of 6 in OCaml?

progman1 commented 4 years ago

definitely only 6

  external fit: t -> TabularDataSet.t -> Int64.t -> Int64.t -> Int64.t
option -> metrics option -> unit = "neuro_model_fit_bytecode"
"neuro_model_fit"

On 26/06/2020, zach notifications@github.com wrote:

Yeah, I just updated the error to give more information. It looks like you may be calling that function with 7 arguments instead of 6 in OCaml?

-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/zshipko/ocaml-rs/issues/29#issuecomment-650355838

zshipko commented 4 years ago

I don't really know why that is happening, but I've made a new commit with a looser restriction on that assertion... does that work? Otherwise, I'm not really able to reproduce this one. Is this happening on a clean build?

progman1 commented 4 years ago

a clean build didn't shift it. but the unrestriction did and code runs as per native.

a small issue: the example finalizer on the github page:

unsafe extern "C" fn mytype_finalizer(v: ocaml::Value) {
    let ptr: ocaml::Pointer<MyType> = ocaml::Pointer::from_value(v);
    ptr.drop_in_place()
}

the compiler didn't recognise the from_value - I see the FromValue trait is exported, does the impl of it need exporting too?

an even smaller issue: there is the ocaml::bytecode_func attribute which confused me as to whether ocaml::func took care of >5 args until I looked at the code. as it appears amongst the examples perhaps some explanation of it would be helpful.

On 26/06/2020, zach notifications@github.com wrote:

I don't really know why that is happening, but I've made a new commit with a looser restriction on that assertion... does that work? Otherwise, I'm not really able to reproduce this one. Is this happening on a clean build?

-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/zshipko/ocaml-rs/issues/29#issuecomment-650369564

zshipko commented 4 years ago

Great, I'm not sure what's going on there still but glad it's working now.

In the example it also includes:

use ocaml::FromValue

This is needed to enable to from_value function.

Also, there is a mention of the bytecode_func wrapper here: https://docs.rs/ocaml/0.13.3/ocaml/attr.func.html but I've added a note to the README too

zshipko / ocaml-rs

wrapping problem on transition from 5 to 6 arguments #29