Add OpArgDef::default_value and allowed_values

[adamcrume]

This will require adding a new enum which looks like the generated protos::AttrValue_oneof_value (in protos/attr_value.rs), and default_value will need to be optional.

[whereistejas]

Hi @adamcrume! Do you think I can take up this issue instead of #289 ? Could you please give me some pointers on how to get started with this one?

[adamcrume]

Yeah, you can definitely take this. This needs a new enum in src/lib.rs (probably called AttrValue) which looks like AttrValue_oneof_value in src/protos/attr_value.rs. Some of the types already have non-proto equivalents, like tensorflow::Shape instead of tensorflow::protos::tensor_shape::TensorShapeProto. It looks like we need a NameAttrList struct as well. These will need conversions to/from protos, and you can see how we've done others like OpDef.

[whereistejas]

Hi @adamcrume, I thought I would outline the changes I'm going to make in a comment before actually starting to code.

There is a struct called OpArgDef: https://github.com/tensorflow/rust/blob/ceefb76ad8aa7b7e2fe0724279fead86ffe35539/src/lib.rs#L1712-L1723

To fill the fields default_value and allowed_values in OpArgDef we need a new enum called AttrValue. This AttrValue enum looks like the enum AttrValue_oneof_value: https://github.com/tensorflow/rust/blob/ceefb76ad8aa7b7e2fe0724279fead86ffe35539/src/protos/attr_value.rs#L42-L53

Do I need add/remove any variants from this enum to in AttrValue or can I take it as it is? Where do I need to create this enum? Should I create it in src/lib.rs? The reason I'm asking this is, I saw that attr_value.rs is a generated file, so can we edit it?

Here, the two new fields are from the struct OpDef_AttrDef: https://github.com/tensorflow/rust/blob/ceefb76ad8aa7b7e2fe0724279fead86ffe35539/src/protos/op_def.rs#L1176-L1188

[adamcrume]

You shouldn't need to add or remove any variants. Don't edit the generated file, or the changes will be wiped out when we regenerate it. The new enum can go in src/lib.rs near OpDef. We should probably go ahead and mark it with #[non_exhaustive] so that if TensorFlow adds new fields to the oneof, it'll be a backwards-compatible change for us to add enum variants.

[ghost]

hi @adamcrume i wonder if this issue is still not ressolved can i work on it?

[adamcrume]

https://github.com/tensorflow/rust/pull/293 hasn't been updated in a couple of years, and whereistejas hasn't said anything, so I'd say it's fair game.

[ghost]

#[non_exhaustive]
#[derive(Debug)]
pub enum AttrValue {
    S(::std::vec::Vec<u8>),
    I(i64),
    F(f32),
    B(bool),
    FieldType(DataType),
    Shape(Shape),
    Tensor(super::tensor::TensorProto),
    List(AttrValueListValue),
    Func(NameAttrList),
    Placeholder(::std::string::String),
}
#[derive(Debug)]
pub struct NameAttrList {
    pub name: std::string::String,
    pub attr: std::collections::HashMap<::std::string::String, AttrValue>,
}
#[derive(Debug)]
pub struct AttrValueListValue {
    // message fields
    pub s: ::protobuf::RepeatedField<::std::vec::Vec<u8>>,
    pub i: ::std::vec::Vec<i64>,
    pub f: ::std::vec::Vec<f32>,
    pub b: ::std::vec::Vec<bool>,
    pub field_type: ::std::vec::Vec<DataType>,
    pub shape: ::protobuf::RepeatedField<Shape>,
    pub tensor: ::protobuf::RepeatedField<super::tensor::TensorProto>,
    pub func: ::protobuf::RepeatedField<NameAttrList>,
}

just the super::tensor::TensorProto is remaining im kinda confused what should be alternative of that protos in lib.rs @adamcrume