Hide Serialization/Deserialization behind feature gate about argmin HOT 20 CLOSED

Yes, indeed, but as always, it is quite interesting what you can learn on a detour. I have fixed the links.

from argmin.

I started to work on this issue. However, this task does not seem to be that simple at second view.
Just making the parts depending on serde conditional is not possible because of the trait constraints of, e.g. ArgminOp. After some discussions and thinking this over, I would suggest to refactoring the checkpointing part into some separate trait (e.g. CheckPointing) together with some macro to make it easy for the end user to use this trait. Would you promote such an approach?

from argmin.

I think this was exactly the problem I faced when I tried to work on this previously. I'm definitely open to suggestions. Could you elaborate on your planned approach? I'm for instance uncertain how macros come into play.

Looking at ArgminOp I guess the Serialize trait bound is the problem. How do you feel about an "alias" for the constraint:

// serde feature is on
#[cfg(serde1)]
pub trait SerializeAlias: Serialize {}
#[cfg(serde1)]
impl<I> SerializeAlias for T where T: Serialize {}

// serde feature is off
#[cfg(not(serde1))]
pub trait SerializeAlias {}
#[cfg(not(serde1))]
impl<I> SerializeAlias for T {}

and then the ArgminOp trait would be defined like this:

pub trait ArgminOp: Clone + Send + Sync + SerializeAlias {
    ...
}

1. I'm not sure if it works as I haven't tested it
2. I would appreciate a better name ;)
3. I'm unsure how this would work across crates (orphan rule), but I think it should be fine.
4. A similar implementation is likely needed for Deserialize and/or DeserializeOwned

Thank your for tackling this, I really appreciate it.

from argmin.

I think there are two problems to be solved. One is avoiding code duplication, which could probably be solved by your suggested approach.

The other problem is that user types which implement ArgminOp should be allowed to work without implementing Serialize and its friends, even if the feature serde1 is selected, as long as checkpointing is not used. The reason is that otherwise adding a feature would remove functionality, and that should not be the case. Consider for example that a user as implement ArgminOp for some custom type, using argmin without the serde1 feature. If he than adds another crate which also depends on argmin, but with serde1 switched on, this will break the original code, since only one copy of the argmin crate (now with serde1 switched on) will make it into the final compilation.
Therefore, I would suggestion a two stage approach, which would require an additional trait (e.g. Checkpointing, that's actually implements the checkpointing feature and defaults to a dummy, if serde1 is disabled. If serde1 is switched on, Checkpointing still defaults to a dummy, unless the default is overridden by the implementation of this trait for the custom type, in which case the Serialize trait is enforced. Since the implementation of the Checkpointing is kind of generic, it should be implemented by a macro to make it easier for the end user to enable checkpointing.

I hope that gives you an idea of what I have in mind. I would like to implement a test case in to see if this would work.

from argmin.

Ah, thats a very good point. I think I understand what you mean and I believe this could be a good solution!

from argmin.

I have just pushed a version of argmin and argmin-core to https://github.com/xemwebe/argmin resp. https://github.com/xemwebe/argmin-core that works with the feature serde1 enabled or disabled. I.e., all test cases pass with either serde as direct dependency or not. Test explicitly testing the checkpointing feature are disabled, if feature serde1 was not selected.

However, this does not remove the dependency on serde completely, because of indirect dependencies. ndarray-linalg depends on cauchy which depends (unconditionally) on serde, same for slog-json and modcholesky. I followed basically your approach of implementing a SerializeAlias and DeserializeOwnedAlias, which works pretty well, actually.

The tricky part is still to allow test functions, which do not implement the traits Serialize and Deserialize. The implementation of an intermediate trait Checkpointing does not turn out to be a good idea at all, since we have to many unknown types at compile time, e.g. all the types defined
by ArgminOp and the types derived by them. Therefore, I came up with the idea to implement dummies for the traits Serialize and Deserialize if the feature serde1 is enabled but the test function is not serializable. This wouldn't break existing code, since currently serializability is a requirement. For Serialization, this would look like:

#[cfg(feature="serde1")]
impl Serialize for TestFunc {
    fn serialize<S>(&self, _serializer: S) -> Result<S::Ok, S::Error>
    where
        S: Serializer
        {
            Err(serde::ser::Error::custom(format!("serialization is disabled")))
        }
}

This could be hidden within a macro that implements the dummies if serde1 is switched on and does nothing otherwise. Unfortunately, it would be required to apply this macro not only to the test function itself, but also to all custom types of ArgminOp, for which serde does not provide an implementation of these traits by default (which might be acceptable, since it should be a rare case). The (new) example noserbrent.rs demonstrates this by hardcoding these dummy traits.

from argmin.

I just had a look at your commits and from what I can see this looks really good! I'm surprised that eventually the changes weren't as drastic as I feared.
The only thing that bothers me a bit is that the examples and the code in the docs is cluttered with #[cfg(feature = "serde1")] and such. These examples are for end users and they should not need to care about this implementation detail (they either have the feature enabled or not). But I think this is a minor issue that should be solvable. One solution, which is not ideal but may be a bit less confusing may be to have two main functions in the examples:

#[cfg(feature = "serde1")]
fn main() {
    if let Err(ref e) = run() {
        println!("{} {}", e.as_fail(), e.backtrace());
    }
}

#[cfg(not(feature = "serde1"))]
fn main() {
    println!("This example must be compiled with the feature `serde1`.");
}

I'm not sure how to deal with the indirect dependencies. In particular for slog_json I doubt that there is a way to make serde optional. However, all code related to logging json could also be behind the serde1 feature gate. The same could apply for modcholesky. In general, I think the only way to remove serde completely is to also disallow the ndarrayl feature if serde1 is turned off. I'm not sure if we want this.
I guess it depends on what our goal is. When I opened this issue, my main goal was to not force users to derive Serialize and Deserialize for their problems if they don't want to use the checkpointing feature. I believe that this is now possible with your implementation. Saving compilation time and binary size by removing a dependency is a nice plus. What is your motivation?

Regarding the dummy implementations of Serialize and DeserializeOwned: I like the idea, but I'm not sure if I fully understand it. To my understanding, the user has to call a macro (say fake_serialize!(TestFunc);) which will implement Serialize and DeserializeOwned for TestFunc (like you outlined in your example). I think this requires argmin to somehow expose serde to the user (I have no problems with that, I think), otherwise the user has to add serde as a dependency which would be counter-intuitive.
Also I think the #[cfg(feature="serde1")] can be removed because if a user calls the macro, serde1 will not be available (maybe #[cfg(feature="argmin/serde1")] would work, but in general I think it is not necessary at all).
In general I would prefer it if this macro was not necessary, but currently I don't see a way past it (other than going for a procedural macro, maybe).

Once again thank you, I really appreciate the discussions and all the work you put into this!

from argmin.

The documentation need to be cleaned, I agree. It's possible to hide the code embedded in docs, i.e. it will be executed but not displayed to the user. The docs just should make clear that the examples only work with feature serde1 switched on as it is displayed, and than show only this example, hiding the other code which is required in order to pass the automatic tests.

Regarding the indirect dependencies, I guess it's feasible to put logging behind the serde1 feature gate, but not so sure about ndarrayl, since this is required for some of the examples. Indeed, I was also motivated to implement this feature gate because I would like to use the lib for cases, where I don't need checkpointing, but have to deal with test function which require lots of data I would prefer borrowing instead of cloning. As it turns out, implementing dummy traits for Serialize and Deserialize is sufficient, and so minimizing code size and compilation time is a nice add-on, my other project wouldn't profit from it, since I use serde anyway elsewhere.

The idea with the macro was that it does nothing, if serde1 is switched off, but otherwise does nothing. Therefore, the user would never see the #[cfg(feature="serde1")] lines. I haven't put much thought on the default of the implementation, though. However, given that
this two implementations are quite simple and generic, an alternative would be to just explain in the documentation how to deal with this cases.

from argmin.

The documentation need to be cleaned, I agree. It's possible to hide the code embedded in docs, i.e. it will be executed but not displayed to the user. The docs just should make clear that the examples only work with feature serde1 switched on as it is displayed, and than show only this example, hiding the other code which is required in order to pass the automatic tests.

This sounds good!

Regarding the indirect dependencies, I guess it's feasible to put logging behind the serde1 feature gate, but not so sure about ndarrayl, since this is required for some of the examples.
Indeed, I was also motivated to implement this feature gate because I would like to use the lib for cases, where I don't need checkpointing, but have to deal with test function which require lots of data I would prefer borrowing instead of cloning. As it turns out, implementing dummy traits for Serialize and Deserialize is sufficient, and so minimizing code size and compilation time is a nice add-on, my other project wouldn't profit from it, since I use serde anyway elsewhere.

This is exactly what I was thinking as well. I guess most users are fine with the serde1 feature turned on, even if they don't need it. But having to implement Serialize and DeserializeOwned even if one doesn't use checkpointing is a pain. However, having to sacrifice the ndarrayl feature in order to not need to implement serialization is a difficult (and, I believe, unreasonable) trade-off. I'm quite happy with your current implementation and I don't think there is (for now) a need to further reduce the dependency on serde.

The idea with the macro was that it does nothing, if serde1 is switched off, but otherwise does nothing. Therefore, the user would never see the #[cfg(feature="serde1")] lines.

I'm sorry, I formulated my concerns too confusingly. I think #[cfg(feature="serde1")] won't work in the code of the user, because the user likely doesn't have a serde1 feature defined. The macro would need to access the argmin-serde1-feature like this: #[cfg(feature="argmin/serde1")], but I'm not sure if this is possible. If it is in fact possible, than I think it is the way to go. If it is not possible, I believe removing it would still work, it just could potentially create unnecessary code.

I haven't put much thought on the default of the implementation, though. However, given that this two implementations are quite simple and generic, an alternative would be to just explain in the documentation how to deal with this cases.

Just to illustrate (and hopefully to be less confusing this time), here is how I think this might work in a macro:

#[cfg(feature="argmin/serde1")]
impl argmin::serde::Serialize for TestFunc {
    fn serialize<S>(&self, _serializer: S) -> Result<S::Ok, S::Error>
    where
        S: argmin::serde::Serializer
        {
            Err(argmin::serde::ser::Error::custom(format!("serialization is disabled")))
        }
}

This requires argmin to expose serde as argmin::serde. I think this is fine as long as serde is hidden in the argmin docs.

from argmin.

Well, I think a procedural macro is required here. In order to prevent further confusion, I have just pushed a new version of argmin with a new sub crate that implements the macro. As you can see from the example file noserbrent.rs, no explicit reference to the feature serde1 is required any more.

I have included the import of the serde traits in the macro definition, thus there is no direct reference to serde in the example file. The only drawback is that if the user imports these traits for something else, compilation fails because of duplicate imports. I am not sure how to get rid of this.

from argmin.

Well, I think a procedural macro is required here. In order to prevent further confusion, I have just pushed a new version of argmin with a new sub crate that implements the macro. As you can see from the example file noserbrent.rs, no explicit reference to the feature serde1 is required any more.

This looks pretty good!

The only drawback is that if the user imports these traits for something else, compilation fails because of duplicate imports. I am not sure how to get rid of this.

I think you should be able to avoid imports entirely if you use serde::Serialize and so on directly:

impl serde::Serialize for $name {
    fn serialize<S>(&self, _serializer: S) -> Result<S::Ok, S::Error>
    where
        S: serde::Serializer
    {
        Err(serde::ser::Error::custom(format!("serialization is disabled")))
    }
}

impl<'de> serde::Deserialize<'de> for $name {
    fn deserialize<D>(_deserializer: D) -> Result<Self, D::Error>
    where
        D: serde::Deserializer<'de>
    {
        Err(serde::de::Error::custom(format!("deserialization is disabled")))
    }
}

I just noticed that TestFunc is hardcoded in your procedural macro, which may be a typo.

from argmin.

Oh, yes of course, I removed the import and fixed the hard-coded name. It seems to work now.

The question remains what is to be done to finally close this issue. I see the at least the following options:

1. Since the dependency on serde cannot be removed completely, just accept the fact and leave it as it is, but with the sub-options a) add the fake_serialize_macro b) just document how to deal with non-serializable test functions (I may publish this macro in its own crate)
2. Implement the serde1 feature gate anyway as it is now
3. Implement the serde1 feature gate but first move logging behind this gate, too

Updating the docs is still open, but depends on the decision taken.

Since I prefer to keep things as simple as possible, I would probably opt for 1, since the primary reason why someone wants this feature gate (e.g. using non-serializable test functions) could be solved without that. But I would also support option 2 or 3, if this is your preference.

What do you think?

from argmin.

I think the solution with the fake_serialize_macro is the best possible one and I would certainly opt for that. After quite some thought I think I agree with you regarding option 2 and 3; there is no reason keep this feature gate. However, since you have already implemented it, we may want to keep it. But I'm unsure ;) If we keep it, then I think the serde1 feature should be part of the default features.

I think the fake_serialize_macro crate is general enough to be published independently of the argmin/argmin-core crate(s). What do you think?

from argmin.

Well, I won't delete the the github repos with the feature gate if you want to keep them ;-) Actually, I wouldn't use code just because it's there already. But if so, it should certainly be a default feature for backward compatibility, but for testing I found it either to have it opt-in instead of opt-out.

II plan to publish the macro independently anyway since it may be useful in other contexts to, but without the "do nothing in case of missing serde1 feature" part, since this wouldn't make sense for a general implementation.

from argmin.

Actually, I wouldn't use code just because it's there already.

You are right. It's probably best to remove it, otherwise it may just cause confusion without a real benefit.

II plan to publish the macro independently anyway since it may be useful in other contexts to, but without the "do nothing in case of missing serde1 feature" part, since this wouldn't make sense for a general implementation.

This sounds good!

from argmin.

I have now created a standalone crate that implements the derive macros, available on
crates.io and github.

from argmin.

This is great, thanks a lot! We took quite a detour on the way to this simple but satisfying solution ;)
This will be useful in the case of "unserializable" solvers as well.

(btw: something seems to be wrong with the links in your posts)

from argmin.

Just so you know, this solution doesn't work when using lifetimes.

implicit elided lifetime not allowed here

help: indicate the anonymous lifetime: `<'_>`

I had to implement Serialize and Deserialize myself.

from argmin.

Thanks @nilgoyette for letting us know! I hope that we'll be able to remove this requirement completely with #34 and #36.

Another hack to mitigate this for now is to derive Serialize and Deserialize while skipping all problematic fields with #[serde(skip)] or #[serde(skip_serialize)]. I'm not sure how well this works when lifetimes are involved.

from argmin.

Since #36 is merged I believe this issue is solved. Feel free to reopen.

from argmin.