Skip to content

New config_from_keras_model #690

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

Merged
merged 19 commits into from
Dec 22, 2022
Merged

Conversation

vloncar
Copy link
Contributor

@vloncar vloncar commented Dec 2, 2022

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.

  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Tests

Current tests should suffice

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. -> As discussed, this will be done after review
  • I have added tests that prove my fix is effective or that my feature works.

@vloncar vloncar added the please test Trigger testing by creating local PR branch label Dec 2, 2022
@jmduarte jmduarte self-requested a review December 2, 2022 20:57
@vloncar vloncar added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Dec 7, 2022
@vloncar vloncar added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Dec 11, 2022
@vloncar vloncar requested a review from jmitrevs December 11, 2022 22:09
@vloncar
Copy link
Contributor Author

vloncar commented Dec 11, 2022

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

@jmduarte
Copy link
Member

jmduarte commented Dec 16, 2022

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: google/qkeras#109). I will try adding pip install pyparsing in the Jenkinsfile.

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

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.

@jmduarte jmduarte removed the please test Trigger testing by creating local PR branch label Dec 21, 2022
@jmduarte jmduarte added the please test Trigger testing by creating local PR branch label Dec 21, 2022
@jmitrevs
Copy link
Contributor

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.

Copy link
Member

@jmduarte jmduarte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks great, and my initial testing works. I have some minor questions.

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

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

@vloncar
Copy link
Contributor Author

vloncar commented Dec 22, 2022

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

@jmitrevs
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor Author

vloncar commented Dec 22, 2022

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.

@vloncar vloncar added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Dec 22, 2022
@jmduarte jmduarte added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Dec 22, 2022
@jmduarte jmduarte merged commit 86f583d into fastmachinelearning:main Dec 22, 2022
calad0i pushed a commit to calad0i/hls4ml that referenced this pull request Jul 1, 2023
* Don't set duplicate attributes

* Expand the use of backend-specific attributes

* Reorganize attributes a bit to remove duplicates

* Move string conversion utilities to new module

* Remove unnecessary init for activation attributes

* New config_from_keras_model function

* Remove config from parsers (no converters use it)

* Save activation's quantizer config in the parser

* Use keras parsing fn to build the initial config

* Lowercase 'trace' attribute (removes duplicate)

* Fix tests that fail due to config update

* Update Jenkinsfile

* Use ChoiceAttribute for convolution strategy

* Try loading a different libdl.so on newer Linux boxes

* Fix test output dirs

* To future self: I'm sorry for this, but it was necessary

* black + isort compatability

Co-authored-by: Javier Duarte <[email protected]>
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