fastmachinelearning / hls4ml

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

Introduce optional description to layer attributes #1127

Open vloncar opened 1 week ago

vloncar commented 1 week ago

Description

Layer attributes are currently used for validation that the layers of the ModelGraph are properly constructed and have all the expected values so optimizers don't need to check for them. (This applies to most, but not all layers at the moment.) However, they can also be a documentation of the IR itself, especially for configurable attributes that users can tweak. This PR extends the attribute system to have an optional description.

The descriptions themselves are strings defined in a separate module, so as to not clutter the layer/backend modules, and to allow simpler multi-line descriptions. Currently only a few descriptions were added, mainly around the configurable ones that are user facing.

Included in the docs is an example of a python script that can generate a documentation page. I encourage the reviewers to run the script to see the output :wink:. I did not add the generated doc to the main docs yet, maybe we should first discuss the format of it. Would be nice to have it in 1.0.0 since it is a low impact change. We can automate the process of generating the page and and include it in the doc building step, but we would need to change the GH action for that to include an environment that has hls4ml and not just sphinx.

Type of change

Tests

Doesn't add a feature that need testing.

Checklist

jmitrevs commented 1 week ago

The updated README PR (#1100) changes some of the documentation format. I think we can find a good place for the output of the script in that format. Maybe it can be added at that step.

vloncar commented 1 week ago

It definitely needs to be added to the index page so it appears on the side along with other pages. Before we do that, we should agree on the format of the produced document. Currently it lists all base attributes, types, weights and configurable attributes, in that order, followed by backend-specific extensions (which are currently all configurable, but this may not always be true). So it is a complete reference. What's slightly ugly is that due to our backends sharing a lot of stuff, many of the backend-specific attributes are repeated. We could instead focus on user-facing configurable attributes, and list only those. That would reduce the clutter, but would arguably be worse for developers working on parsers wanting to check what's expected. I'm fine with either approach, what do the others think?

JanFSchulte commented 1 week ago

I did test this offline just now and it seems to be working great. I do agree that the repeated arguments for each backend makes the page very cluttered. I think it makes sense to treat the doc pages as more user-facing and reduce the information to what is exposed to them. As a developer there's always the code itself to look at to figure these things out.

jmitrevs commented 1 week ago

I think the page should be more user-specific, maybe with a link the the full description? I think the main part should be only configurable attributes.

Can the python script be made smarter, maybe combining backend-specific options that are common across multiple backends?

vloncar commented 1 week ago

Perhaps an in-between solution, where we list all attributes, but configurable attributes will list backends to which they apply, as opposed to listing backend specific attributes under each backend? For example, attribute conv_implementation will say available in: Vivado, Vitis, Catapult (but not in Quartus and oneAPI) and will be listed only once, not three times. Another example is softmax_implementation which will list all backends (except SR), but won't be listed 5 times.

I'm not sure "looking at the code" is easy enough to find all the attributes, maybe only the base ones. To a newcommer developer or an existing power user it may not be obvious what can be tweaked regarding softmax, the fact that it can be skipped and where is the implementation attribute defined...

JanFSchulte commented 1 week ago

Perhaps an in-between solution, where we list all attributes, but configurable attributes will list backends to which they apply, as opposed to listing backend specific attributes under each backend?

That sounds very good to me.

vloncar commented 5 days ago

I made the changes according to the discussion above. There's also an optional way to write only the user-configurable attributes, you can check out how that one looks, though after looking at it all, I'm somewhat leaning more towards including the "all attributes" version in the docs. But if more people in this thread prefer the "configurable attributes" version, I'm fine.

I had to make some changes to the underlying types and attributes classes, to make sure they can appear in dicts and sets (we need the __hash__ function) as well as clean up an oddity in CodeAttribute. None of these should influence any existing workflows, this is more for futureproofing. I'm not sure if we currently have this, but in theory backend-specific attributes can have the same name but different properties (different default, different choice values, different description...) so we can't compare on name alone, we need to compare the whole attribute object, and if they are different, print the different versions.

bo3z commented 5 days ago

Perhaps an in-between solution, where we list all attributes, but configurable attributes will list backends to which they apply, as opposed to listing backend specific attributes under each backend? For example, attribute conv_implementation will say available in: Vivado, Vitis, Catapult (but not in Quartus and oneAPI) and will be listed only once, not three times. Another example is softmax_implementation which will list all backends (except SR), but won't be listed 5 times.

I agree with this approach. The only questions the becomes whether the options assignable to the attribute remain the same across the backends? Or is that not specified?