fastmachinelearning / hls4ml

Machine learning on FPGAs using HLS
https://fastmachinelearning.org/hls4ml
Apache License 2.0
1.22k stars 396 forks source link

New config_from_keras_model #690

Closed vloncar closed 1 year ago

vloncar commented 1 year ago

Description

This PR introduces the new config_from_keras_model function that fixes the shortcomings of the old one, namely the need to keep in sync the supported layers and their configurable attributes. It relies on the attribute system to automatically discover tunable attributes of each layer and include them in the config. Config is now more expressive, and includes backend-specific parameters.

Type of change

For a new feature or function, please create an issue first to discuss it with us before submitting a pull request.

Note: Please delete options that are not relevant.

Tests

Current tests should suffice

Checklist

vloncar commented 1 year ago

@jmitrevs @jmduarte This is now redesigned and ready for review

jmduarte commented 1 year ago

For the test that fail, i think it's related to this error from the keras model conversion step, which happens when we try to import hls4ml:

WARN: Unable to import optimizer(s) from qkeras.py: No module named 'pyparsing'
WARN: Unable to import optimizer(s) from quantization_templates.py: No module named 'pyparsing'
WARN: Unable to import optimizer(s) from quantization_templates.py: No module named 'pyparsing'
WARN: Unable to import optimizer(s) from quantization_templates.py: No module named 'pyparsing'

It's weird because qkeras uses this library but it doesn't have it as a dependency perhaps due to a mixup of names (See: https://github.com/google/qkeras/issues/109). I will try adding pip install pyparsing in the Jenkinsfile.

jmitrevs commented 1 year ago

I confirmed that this works in our use of extension API (after removing the unused config). Let me look once more, but I think this is a good change.

jmitrevs commented 1 year ago

Let see the tests finish, but if people don't object, I think we can merge this PR. In general, though, I would prefer more documentation on the attributes, what are the common attributes for different categories, etc.

jmitrevs commented 1 year ago

Assuming the CI looks good, I'll merge this later today, unless someone objects.

vloncar commented 1 year ago

Wait, we need to do the pre-commit stuff. I'm working on it, but there are A LOT of issues.

jmitrevs commented 1 year ago

My understanding from @jmduarte was that it would automatically format things, so you don't need to manually do it, though I am not 100% sure.

jmduarte commented 1 year ago

some are fixed automatically (black formatting, isort sorting imports, etc.), some are not (flake8 issues). @vloncar is fixing (most) of the remaining issues now, and i will take a look at the rest, then we can merge

vloncar commented 1 year ago

I fixed all of the issues from flake8, but I cheated a bit by ignoring the print statement warnings. pre-commit hook is changed to ignore this, but we should make a note of this and replace all print statements with proper logging (is on my TODO list, but way back). For some reason this still fails the black and isort stages. Javier understands this better.