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

Configuration editor #784

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

vloncar
Copy link
Contributor

@vloncar vloncar commented May 8, 2023

Description

Changing configuration in hls4ml requires deep understanding of the conversion process in order to be successful. New layers can be introduced, which pick up default precision definition causing problems for subsequent layers, unexpected allocation of DSPs etc. To solve this, users have to anticipate new layer names and with a meticulous trial-and-error procedure nail the initial configuration that will result in perfect conversion. This is way too cumbersome but until we have a better configuration system it is the only way. This PR proposes an alternative, to be able update the configuration of an existing hls4ml model interactively, via a GUI. It came out of a crazy idea I had last night after a few beers. See it in action:

Screenshot from 2023-05-08 21-26-23

It is very simple to use, make your changes and click "Update", the model will be updated and the changes should be reflected on the architecture plot on the left side.

This builds upon the recent automatic configuration support, by exposing only the configurable attributes (precision, strategy, RF etc). Some parts of it are a bit hacky for now, until we improve the configuration itself (ideally, we should have an model.get_config()/model.set_config() API).

The GUI is built using PySimpleGUI library, you can install it with pip install pysimplegui. Since the feature is optional, I also added an installation extra, so installation of pip install hls4ml[gui] will ensure the dependency is there. I considered a pure Tk implementation using tkinter from the standard library to avoid the new dependency, but the equivalent GUI had 400 lines of code and no support for mouse scroll events so I abandoned it.

The GUI is part of the utilities, to use it, run:

model = hls4ml.converters.convert_from_keras_model(...)
hls4ml.utils.edit_model_configuration(model)

A GUI should open up. If you use this through SSH or VS code server it may be slow to open. I don't know what is causing this, possibly an issue with the GUI library itself. Pure tkinter implementation didn't have this issue.

In the future, we can expand it to support creating the initial configuration, based on Keras/PyTorch/ONNX model.

Type of change

Also fixes a minor bug in definition of the trace attribute (was string, should be bool) and adds strategy attribute to supported layers.

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change which adds functionality)

Tests

We currently don't have any way of testing GUIs and this is a can of worms, so I suggest we skip it for now.

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 this to the v0.8.0 milestone May 12, 2023
@jmitrevs
Copy link
Contributor

Can this work inside a jupyter notebook?

@@ -1,3 +1,8 @@
from hls4ml.utils.config import config_from_keras_model, config_from_onnx_model, config_from_pytorch_model # noqa: F401
from hls4ml.utils.example_models import fetch_example_list, fetch_example_model # noqa: F401
from hls4ml.utils.plot import plot_model # noqa: F401

try:
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if there's a better way to do this because now if the import fails, you just gen an error while using it of:

AttributeError: module 'hls4ml.utils' has no attribute 'edit_model_configuration'

ideally the error message would be better.

@jmitrevs jmitrevs modified the milestones: v0.8.0, v1.0.0 Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants