hls4ml icon indicating copy to clipboard operation
hls4ml copied to clipboard

fix nondefault project name handling

Open jmitrevs opened this issue 2 years ago • 7 comments

Description

As discussed in #624, convert_from_keras_model with project_name set to nondefault value (e.g. mynewprojname) fails to build, because in build.tcl the variable ${mynewprojname} is not defined. This fixes the problem my always leaving the variable called myproject, but just changing the value of the variable. Some changes in the tcl code were requried since now since the variable name does not always match the value it contains. The build.tcl indentation was fixed, and the parsing of the Vivado report was fixed. Note that this is broken in main even in the default case currently.

Type of change

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

Tests

Testing was done offline by modifying and running part1 of the hls4ml-tutorial, as mentioned in #624. Nevertheless, we probably should add a test. The complication is that the test requires synthesis, and generally our pytests don't do synthesis. Plus, synthesis is long. Do we want to add another synthesis test for this case or modify an existing one?

Checklist

  • [x] I have read the guidelines for contributing.
  • [x] I have commented my code, particularly in hard-to-understand areas.
  • [x] I have made corresponding changes to the documentation.
  • [x] My changes generate no new warnings.
  • [ ] I have added tests that prove my fix is effective or that my feature works.

jmitrevs avatar Aug 01 '22 22:08 jmitrevs

The qkeras pytest failure is a known problem that exists in the main branch even without this change.

jmitrevs avatar Aug 02 '22 11:08 jmitrevs

Thanks for the fix, I don't think we need a new test for this

thesps avatar Aug 02 '22 11:08 thesps

The last push was not a complete fix. I'll send a message again when I think it's ready.

jmitrevs avatar Aug 03 '22 19:08 jmitrevs

I still need to test vsynth=True and also that I didn't mess up the FIFO optimizations.

jmitrevs avatar Aug 03 '22 21:08 jmitrevs

Actually, looking in the main branch as a starting point, I see

 'VivadoSynthReport': {'LUT': '0',
  'FF': '0',
  'BRAM_18K': '0',
  'URAM': '0',
  'DSP48E': '0'}

when I do part1 of the hls4ml-tutorial with hls_model.build(vsynth=True) so vsynth might be broken in the main branch. Will try to investigate.

jmitrevs avatar Aug 03 '22 21:08 jmitrevs

This has been broken for some time now, I observed it as well while playing with Vitis. Thanks for looking into it.

vloncar avatar Aug 03 '22 21:08 vloncar

This seems to break the VivadoAccelerator backend.

jmitrevs avatar Aug 03 '22 23:08 jmitrevs

I believe this is ready for merging.

jmitrevs avatar Aug 16 '22 15:08 jmitrevs