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

Introduce optional description to layer attributes #1127

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

Conversation

vloncar
Copy link
Contributor

@vloncar vloncar commented Nov 13, 2024

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 😉. 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

  • Documentation update
  • New feature (non-breaking change which adds functionality)

Tests

Doesn't add a feature that need testing.

Checklist

  • I have done all the checks

@JanFSchulte JanFSchulte added this to the v1.0.0 milestone Nov 15, 2024
@jmitrevs
Copy link
Contributor

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

vloncar commented Nov 18, 2024

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

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

jmitrevs commented Nov 18, 2024

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

vloncar commented Nov 18, 2024

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

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

vloncar commented Nov 20, 2024

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.

@vloncar vloncar added the please test Trigger testing by creating local PR branch label Nov 20, 2024
@bo3z
Copy link
Contributor

bo3z commented Nov 20, 2024

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?


# Common attributes

reuse_factor = (
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe worth saying something along the lines RF = 1 means all multiplications happen in parallel, i.e. one cc. Analog, a RF = k, corresponds to multiplications being spread out over k cc. Just to make it a bit more clear what the RF value can be set to.

)

index = 'Internal node counter used for bookkeeping and variable/tensor naming.'
trace = 'Enables saving of layer output (tracing).'
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe worth saying that this only applies for hls4ml_model.predict() (to avoid any confusion with potential saving of layer outputs during simulation)


# Convolution-related attributes

conv_pf = (
Copy link
Contributor

Choose a reason for hiding this comment

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

conv_pf only applies to io_parallel, right? Maybe worth mentioning if that's the case

'convolution kernel occuring in parallel. '
'Higher number results in more parallelism (lower latency and II) at the expense of resources used.'
)
conv_implementation = '"LineBuffer" implementation is preferred over "Encoded" for most use cases.'
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, just for io_stream

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.

4 participants