Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

better repalce_node fn #934

Merged
merged 5 commits into from
Dec 19, 2023

Conversation

calad0i
Copy link
Contributor

@calad0i calad0i commented Dec 3, 2023

Description

Make ModelGraph.replace_node method work with nodes work with replacing nodes, which itself/parents/children have multi-input/outputs.

#932 will be fixed with this PR.

This fix is rather long but keeps the previous behavior in most cases. A much shorter version will be simply doing

        new_node.inputs = old_node.inputs
        new_node.outputs = old_node.outputs

but the final variable names will be different from the current version.

Type of change

  • Bug fix (non-breaking change that fixes an issue)

Tests

Integrated with test_repack_precision, renamed to test_repack_stream

Checklist

  • I have read the guidelines for contributing.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • I have installed and run pre-commit on the files I edited or added.
  • I have added tests that prove my fix is effective or that my feature works.

@jmitrevs jmitrevs added the please test Trigger testing by creating local PR branch label Dec 4, 2023
@jmitrevs
Copy link
Contributor

So the order of the nodes would stay the same, right? Is there a possibility to reorder the nodes? (I am not saying it is needed; just trying to understand.)

@jmitrevs jmitrevs added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Dec 15, 2023
@jmitrevs
Copy link
Contributor

I will tentatively put this as 0.8.1 milestone, but I would be fine deferring to 1.0.

@jmitrevs jmitrevs added this to the v0.8.1 milestone Dec 15, 2023
@jmitrevs jmitrevs added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Dec 15, 2023
@calad0i
Copy link
Contributor Author

calad0i commented Dec 15, 2023

So the order of the nodes would stay the same, right? Is there a possibility to reorder the nodes? (I am not saying it is needed; just trying to understand.)

They should stay the same for the current implementation (copy in order w/ replacing a single k/v pair).

What do you mean by reorder in this case? Shuffling the nodes will break the graph in general I guess.

@jmitrevs
Copy link
Contributor

I was thinking if there was a case where the new node expected the order of inputs to be different than the old node. It's more hypothetical, without a specific case in mind. ONNX nodes often have multiple inputs with specific roles assigned to specific inputs, but we usually transform the special inputs early.

@jmitrevs
Copy link
Contributor

Do the tests ever produce a case when the length or the inputs or outputs is not one? I tried looking with a debugger and didn't see it in the tests, but I could have missed it.

@jmitrevs
Copy link
Contributor

By the way, to answer my question about the replaced nodes having one input and output only in the test, I think that's true, but this fix properly handles the case when the node after the replaced node has multiple inputs, which the test demonstrates. So after properly setting the output directories, I think I can merge this PR and make 0.8.1.

@jmitrevs jmitrevs added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Dec 19, 2023
@jmitrevs jmitrevs merged commit 93e759c into fastmachinelearning:main Dec 19, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
please test Trigger testing by creating local PR branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants