Add Operation Instances for accessing graph edges during graph construction #360

Corallus-Caninus · 2022-04-19T05:40:03Z

Expose Output types for each Operation as mentioned in issue #358 and create an object we can add features to such as Input types for runtime (graph build time) access of Operation properties.

google-cla · 2022-04-19T05:40:09Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

Corallus-Caninus · 2022-04-19T05:43:09Z

signed the CLA let me know what else I have to do this is my first Google contrib.

Corallus-Caninus · 2022-04-19T17:51:28Z

Need to rename commit message from odd to add, I'll do this after work. Also maybe this should be a struct with Operation and generated Outputs with names instead of a tuple with HashMap of which I'm not a fan.

dskkato · 2022-04-19T22:49:24Z

Need to rename commit message from odd to add,

In this case, you can use git commit --amend and modify the message. Then, git push --force to update your repository's branch.

adamcrume

This does look like a very useful improvement, but I think we could make it a bit better. Rather than returning a map of strings to outputs, it probably makes more sense to return a new type for every op. For example, for the Batch operation, it would make sense to do:

impl Batch {
    pub fn build_outputs<O0: ::std::convert::Into<crate::Output>>(
        &self,
        in_tensors: O0,
        scope: &mut crate::Scope,
    ) -> crate::Result<BatchOutputs> {
        ...
    }
}

pub struct BatchOutputs {
  operation: crate::Operation,
}

impl BatchOutputs {
    pub fn operation(&self) -> &crate::Operation {
        &self.operation
    }

    pub fn batched_tensors(&self) -> crate::Output {
        crate::Output {
            operation: self.operation.clone(),
            index: 0,
        }
    }

    pub fn batch_index(&self) -> crate::Output {
        crate::Output {
            operation: self.operation.clone(),
            index: 1,
        }
    }

    pub fn id(&self) -> crate::Output {
        crate::Output {
            operation: self.operation.clone(),
            index: 2,
        }
    }
}

This means that users would be able to do:

let batch = Batch::new().build_outputs(...)?;
let batched_tensors: Output = batch.batched_tensors();

rather than:

let (batch_op, batch_outputs) = Batch::new().build_outputs(...)?;
let batched_tensors: Output = batch_outputs.get(&"batched_tensors".to_string()).ok_or(some_error())?.clone();

In other words, generating actual methods rather than using a map is much clearer, automatically generates better docs, and means that users don't have to worry about misspelling keys or error handling for missing map entries.

adamcrume · 2022-04-20T04:08:40Z

tensorflow-op-codegen/src/main.rs

@@ -221,6 +226,98 @@ fn write_attr<W: Write>(w: &mut W, attr: &Attr) -> Result<(), io::Error> {
    Ok(())
 }

+//Corallus-Caninus: same as above but return a HashMap of output names to Outputs so the


You don't need to include your name in the comments.

I just wrote something like this let me update the PR.

Edit: my commit is slightly different. It still uses a HashMap but with getters, I can look into a codegen solution but I'm still learning the low level bindgen to the pbtxt.

Alright please tell me any other formatting standards I'll remove the comment names.

I think I can make this happen give me awhile to chew on this.

Corallus-Caninus · 2022-04-25T05:00:36Z

alright I had some difficulties with git but please let me know what you think of the most recent commit I pushed thanks.

adamcrume

Moving in the right direction!

adamcrume · 2022-04-27T04:09:17Z

src/graph.rs

@@ -1059,6 +1069,7 @@ impl Operation {

    /// Returns the type of a specific input.
    pub fn input_type(&self, index: usize) -> DataType {
+        /// and the return value is the source operation and the index into its output array.


Was this supposed to go above in the docs for output?

adamcrume · 2022-04-27T04:14:34Z

src/graph.rs

+        Ok(result)
+    }
+}
+impl ::core::ops::Index<i64> for OutputSlice {


For consistency with the rest of the codebase, this should use std::ops::Index at the top of the file and then impl Index<i64> here.

adamcrume · 2022-04-28T01:30:16Z

src/graph.rs

@@ -1750,6 +1761,98 @@ impl Output {

 ////////////////////////

+/// An OutputSlice allows slicing an variable length Operation to retrieve its outputs.
+#[derive(Debug, Clone)]
+pub struct OutputSlice {


Is there any particular reason to use this rather than Vec<Output> and a function to create them?

nope, patching now.

adamcrume · 2022-04-28T01:35:08Z

tensorflow-op-codegen/src/main.rs

+    write!(w, "impl {}Outputs {{\n", op_name)?;
+
+    for (i, output) in outputs.iter().enumerate() {
+        if output.number_attr.is_some() {


Instead of calling unwrap() below, this should do:

if let Some(number_attr) = &output.number_attr {

patching now.

adamcrume · 2022-04-28T01:36:54Z

tensorflow-op-codegen/src/main.rs

+                output.rust_name
+            )?;
+            //create an Output for this index
+            write!(w, "        let forward_output = crate::Output {{\n")?;


forward_output doesn't need to be created at all. Instead of forward_output.operation.clone() below, it can just use self.op.clone().

yes thank you I have fixed this.

adamcrume · 2022-04-28T01:38:49Z

tensorflow-op-codegen/src/main.rs

+            //now get that Outputs operation and create an OutputSlice given the runtime length
+            write!(
+                w,
+                r#"        crate::graph::OutputSlice::new(forward_output.operation.clone(), self.op.get_attr_int("{}").unwrap())"#,


Instead of unwrap(), this function should return Result<Vec<Output>>.

affirmative.

adamcrume · 2022-04-28T01:51:35Z

tensorflow-op-codegen/src/main.rs

+) -> Result<(), io::Error> {
+    let mut escaper = Escaper::new(keywords);
+    let escaped_args: Vec<_> = args.iter().map(|arg| escaper.escape(&arg)).collect();
+    write!(w, "    fn build_impl_outputs(&self, ")?;


I think this should be called build_outputs_impl.

I think so too.

adamcrume · 2022-04-28T01:52:55Z

tensorflow-op-codegen/src/main.rs

+    write!(w, "            }}\n")?;
+    for attr in attrs {
+        write_set_attr(w, attr, &node_var)?;
+        //TODO: also get type and value of each number_attr by looking up the corresponding attribute


Is this intended to be done in this PR or later?

adamcrume · 2022-04-28T03:17:15Z

tensorflow-op-codegen/src/main.rs

+            //now get that Outputs operation and create an OutputSlice given the runtime length
+            write!(
+                w,
+                r#"        crate::graph::OutputSlice::new(forward_output.operation.clone(), self.op.get_attr_int("{}").unwrap())"#,


These are always starting at index 0, but that's not correct if this is not the first output arg.

Also, for both this and the else branch, the index needs to be adjusted for the size of prior outputs. In other words, if the first output_arg has size 4, and the second has size 5, then the first needs to take outputs 0-3 and the second needs to take 4-8.

To do this, you'll probably need to read all the relevant int attributes as per the TODO above and either pass the Outputs or the indices into the FooOutputs struct.

thank you, patching this now.

adamcrume · 2022-04-28T03:40:13Z

tensorflow-op-codegen/src/main.rs

+            number_attr_opt = Some(number_attr.clone());
+        }
+        op_outputs.push(Output {
+            //NOTE: we should be able to reuse attr keywords, correct me if im wrong.


This should probably use a new Escaper. Reusing attr_escaper means that if an op has an attr named foo and an output named foo then the generated code would be BlahOutputs::foo_() (i.e. with an underscore), even though they shouldn't actually clash.

patching now, Ill also do this for Inputs in the next PR

Corallus-Caninus · 2022-04-29T20:36:25Z

Thank you for the code review. I will be attending to these errors as well as the following:

So looking at the .pbtxt, I think I need to expose inputs too or we will end up doing this eventually. I am thinking about restructuring this PR to create operation specific structs and leaving the builder functions as generic operation types that return these "inherited" operation specific structs for inputs and outputs at least as a starting point. This would also go in another module (eventually) since the code is becoming spaghetti with this PR feature. I've updated this PRs name to reflect this objective.

adamcrume

Hm, I don't know why the format checker refuses to run. Please run cargo fmt on src/ops/ops_impl.rs.

Just a few smallish changes, and then I think this will be good to merge.

adamcrume · 2022-05-05T03:38:24Z

tensorflow-op-codegen/src/main.rs

+    op_name: &str,
+    attrs: &[Attr],
+    args: &[String],
+    outputs: Vec<Output>,


This function doesn't use attrs, args, or outputs.

adamcrume · 2022-05-05T03:53:38Z

tensorflow-op-codegen/src/main.rs

+        //counts holds the number of times an entry is repeated in offset
+        let counts = dynamic_offset
+            .iter()
+            .fold(HashMap::new(), |mut counts, String| {


String should be lower case, since it's a variable.

adamcrume · 2022-05-07T01:37:23Z

tensorflow-op-codegen/src/main.rs

+                "        let dynamic_offset = ({}) as i32;\n",
+                scalar_offsets
+            )?;
+            write!(w, "        let mut Outputs = vec![];\n",)?;


Outputs should be lower case since it's a variable.

adamcrume · 2022-05-07T01:38:33Z

tensorflow-op-codegen/src/main.rs

+            if dynamic_offset.is_empty() {
+                write!(
+                    w,
+                    "        for i in {}..self.op.get_attr_int(\"{}\")? as i32{{\n",


Instead of \"{}\", it's safer to use {:?} for these, because it will automatically escape any special characters.

adamcrume · 2022-05-07T01:54:25Z

tensorflow-op-codegen/src/main.rs

+    write!(w, "}}\n")?;
+
+    Ok(())
+}


I think this can be simplified a bit, but I can hack on it after the PR is merged.

adamcrume · 2022-05-07T02:00:41Z

tensorflow-op-codegen/src/main.rs

+        let mut number_attr_opt = None;
+        if !number_attr.is_empty() {
+            number_attr_opt = Some(number_attr.clone());
+        }


These five lines can be done in one go:

let number_attr_opt = if output.number_attr.is_empty() { None } else { Some(output.number_attr.clone()) };

which also reduces the amount of cloning.

… construction

Corallus-Caninus · 2022-05-13T06:38:48Z

alright Ive followed up with your review and promised myself I wouldn't add any more features until I open a new PR. Thanks again for your help @adamcrume .

adamcrume · 2022-05-16T03:42:13Z

Thanks! This should be useful for a number of things.

Corallus-Caninus changed the title ~~Master~~ Expose named operations as a builder alternative Apr 19, 2022

adamcrume requested changes Apr 20, 2022

View reviewed changes

Corallus-Caninus force-pushed the master branch from cee8013 to f189811 Compare April 20, 2022 05:21

Corallus-Caninus changed the title ~~Expose named operations as a builder alternative~~ Expose Outputs for built Operations Apr 25, 2022

Corallus-Caninus force-pushed the master branch from 8c67e95 to e46b266 Compare April 27, 2022 06:01

Corallus-Caninus requested a review from adamcrume April 27, 2022 06:08

adamcrume requested changes Apr 28, 2022

View reviewed changes

Corallus-Caninus changed the title ~~Expose Outputs for built Operations~~ Add Operation Instances for accessing graph edges during graph construction May 1, 2022

Corallus-Caninus force-pushed the master branch from 27df7e8 to c37c055 Compare May 1, 2022 00:29

Corallus-Caninus requested a review from adamcrume May 1, 2022 01:29

Corallus-Caninus force-pushed the master branch 7 times, most recently from d38585f to f593359 Compare May 1, 2022 02:29

adamcrume requested changes May 7, 2022

View reviewed changes

Corallus-Caninus force-pushed the master branch 3 times, most recently from a03c7e0 to 1e9df26 Compare May 13, 2022 06:04

Add Operation Instances for accessing Outputs and Inputs during graph…

8bbf9f9

… construction

Corallus-Caninus force-pushed the master branch from 1e9df26 to 8bbf9f9 Compare May 13, 2022 06:37

Corallus-Caninus requested a review from adamcrume May 13, 2022 06:39

adamcrume merged commit 110b139 into tensorflow:master May 16, 2022

Add Operation Instances for accessing graph edges during graph construction #360

Add Operation Instances for accessing graph edges during graph construction #360

Conversation

Corallus-Caninus commented Apr 19, 2022 • edited Loading

google-cla bot commented Apr 19, 2022

Corallus-Caninus commented Apr 19, 2022

Corallus-Caninus commented Apr 19, 2022

dskkato commented Apr 19, 2022

adamcrume left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Corallus-Caninus Apr 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Corallus-Caninus commented Apr 25, 2022

adamcrume left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Corallus-Caninus commented Apr 29, 2022 • edited Loading

adamcrume left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Corallus-Caninus commented May 13, 2022 • edited Loading

adamcrume commented May 16, 2022

Corallus-Caninus commented Apr 19, 2022 •

edited

Loading

adamcrume left a comment •

edited

Loading

Corallus-Caninus Apr 20, 2022 •

edited

Loading

Corallus-Caninus commented Apr 29, 2022 •

edited

Loading

Corallus-Caninus commented May 13, 2022 •

edited

Loading