- 
                Notifications
    
You must be signed in to change notification settings  - Fork 712
 
NXP backend: Update aot_neutron_compile.py example pipeline #14142
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
Conversation
          
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/14142
 Note: Links to docs will display an error until the docs builds have been completed. ❌ 2 New FailuresAs of commit 815a3f7 with merge base fd9f176 ( NEW FAILURES - The following jobs have failed:
 
 This comment was automatically generated by Dr. CI and updates every 15 minutes.  | 
    
| 
           @pytorchbot label "release notes: nxp"  | 
    
        
          
                examples/nxp/aot_neutron_compile.py
              
                Outdated
          
        
      | 
               | 
          ||
| edge_program_manager = NeutronEdgePassManager()(edge_program_manager) | ||
| 
               | 
          ||
| if args.remove_quant_io_ops: | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This step is done before delegation, but in executorch_pipeline.py it is done after delegation. In our local repo it is placed before delegation. Why are there differences?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be placed before the delegation. @MartinPavella, can you confirm it since you are the author of the updated executorch_pipeline.py?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In executorch_pipeline, the removal of I/O quantization after partitioning was up-streamed by you, with @skywall as the original author (cf2f170).
I did not add the IO removal, so I'm not sure which order is preferred. Perhaps Lukas can comment. (But I am leaning towards keeping it consistent with main-nxp.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, I will change it.
Edit: Done ✅
        
          
                examples/nxp/aot_neutron_compile.py
              
                Outdated
          
        
      | if args.delegate: | ||
| logging.info("Executing Neutron Partitioner and Delegate") | ||
| edge_program_manager = edge_program_manager.to_backend( | ||
| NeutronPartitioner( | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please unify edge_program_manager.to_backend(), compile_spec and partitioner creation with version in executorch_pipeline.py`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done ✅
6d033df    to
    ddf400c      
    Compare
  
    | 
               | 
          ||
| if remove_quant_io_ops: | ||
| edge_program_manager = edge_program_manager.transform( | ||
| [RemoveIOQuantOpsPass(edge_program_manager=edge_program_manager)] | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unrelated to this PR but why is this not part of the NeutronEdgePassManager?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I probably did put the RemoveIOQuantOpsPass call on a wrong place. I will double check it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The RemoveIOQuantOps is intentionally not part of the NeutronPassManager default passes, as this changes the model API:
Normally when the quantization is performed and consequently the delegation to Neutron backend the model input and output type is untouched - e.g. FP32. There is a Quantize node doing the input quantization for the rest of the (optionally) Neutron compute.
This pass removes the Quantize and Dequantize nodes from the I/O tensors, and changes the datatype to int8 .
It can be applied by the NeutronPassManager, but with user choice option. Not by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I said, we can add an optional arg to the PassManager for this pass. It feels like someone has to go out of their way to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved the application to the PassManager itself. Now it is an opt-in boolean.
        
          
                examples/nxp/aot_neutron_compile.py
              
                Outdated
          
        
      | if args.remove_quant_io_ops: | ||
| edge_program = edge_program.transform( | ||
| [RemoveIOQuantOpsPass(edge_program_manager=edge_program)] | ||
| edge_program_manager = edge_program_manager.transform( | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as I said before can we move the pass inside the passmanager and add a remove_quant_io as an arg to it?
a038916    to
    90c851c      
    Compare
  
    | 
           I refactored the   | 
    
c4f2d68    to
    e3cdc16      
    Compare
  
    e3cdc16    to
    bd17839      
    Compare
  
    | 
           Sorry everyone for the tag, I somehow managed to mess up a rebase.  | 
    
bd17839    to
    bdf54da      
    Compare
  
    bdf54da    to
    815a3f7      
    Compare
  
    | 
           The fail with Samsung's tests is unrelated and appears in other PRs. The second red signal is some problem in the runner setup, and I don't see any relation to changes made in this PR.  | 
    
| 
           @roman-janik-nxp please re-review if you agree with the implementation of the requested change.  | 
    
Summary
Updates example in
aot_neutron_compile.pyto reflect current state of NXP backend.Test plan
AOT examples should run automatically in CI. You can manually test it using
backends/nxp/run_aot_example.shcc @digantdesai @JakeStevens @robert-kalmar @MartinPavella @roman-janik-nxp