Skip to content

Add functionality to use granularity option also for pytorch models #1051

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 14 commits into from
Aug 27, 2024

Conversation

JanFSchulte
Copy link
Contributor

@JanFSchulte JanFSchulte commented Aug 13, 2024

So far the pytorch parser's config_from_pytorch_model has the granularity argument, but complete ignores it. This PR mostly copies the treatment of this parameter from the keras parser to add this functionality.

This requires to slightly rework in the pytorch parser to allow to extract the layer structure already in config_from_pytorch_model, which in turn requires to move the input_shape from convert_from_pytorch_model to config_from_pytorch_model.

Removing the need to explicitly pass this shape and have it inferred from the model itself is on my todo list.

This PR also requires adding torch to the list of requirements.

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)

Tests

Tested on a model and using the granularity='name' option and adjusting the precision of a layer, this precision gets successfully propagated to the hls4ml project.

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.

@JanFSchulte
Copy link
Contributor Author

pre-commit.ci autofix

@JanFSchulte JanFSchulte added the please test Trigger testing by creating local PR branch label Aug 13, 2024
@JanFSchulte JanFSchulte added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Aug 13, 2024
@JanFSchulte JanFSchulte added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Aug 21, 2024
@JanFSchulte JanFSchulte added this to the v1.0.0 milestone Aug 21, 2024
@JanFSchulte JanFSchulte added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Aug 21, 2024
@@ -303,6 +301,7 @@ def convert_from_pytorch_model(

model_config = hls_config.get('Model', None)
config['HLSConfig']['Model'] = _check_model_config(model_config)
config['InputShape'] = hls_config.get('InputShape', None)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we shouldn't have a default of None for input shape because this will propagate further and then lead to errors which won't make it clear what is the original cause

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, we're now raising an exception if that parameter is not found. No point in continuing.

@@ -321,6 +323,77 @@ def config_from_pytorch_model(
model_config['Strategy'] = 'Latency'

config['Model'] = model_config
config['PytorchModel'] = model
config['InputShape'] = input_shape
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add a check if the passed input shape makes sense. Later on if it doesn't it's not easy to figure out why.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have been thinking about that. In Pytorch it seems like the exact input shape is not determined by the model architecture, so it's not possible to completely infer it during parsing. We we can still check some general features, like the number of dimensions of the input, based on the type of the first layer. I'll implement something along those lines.

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually meant much simpler, just check if input shape is a list/iterable and not None. Because technically you can pass None and you'll get a strange error later. Since this is one of the top user-facing functions, it makes sense we do some user input validation here so we have more confidence later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, got it. I changed it to enforce that the input shape is a tuple for a single input or a list of tuples for multiple inputs.

setup.cfg Outdated
@@ -32,6 +32,7 @@ install_requires =
tabulate
tensorflow
tensorflow-model-optimization<=0.7.5
torch
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this? This will force all installations of hls4ml to have both tf and torch, and since these have huge dependencies themselves it may bork user's environment with the partial updates. Ideally we would have hls4ml with optional extras installed to support various frontends (e.g., pip install hls4ml[torch]), but this may not be easy with the current codebase.

Also, if we go down this route, we should remove the checks for existence of pytorch on the system in converters/__init__.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can get away with not making torch a requirement by moving the from hls4ml.converters.pytorch_to_hls import parse_pytorch_model from the converters/__init__.py directly into config.py. The only reason why I had put the import there is that it's there for keras. The question is, is there a reason to place these imports into the converters/__init__.py? If not, I can just move it and get rid of the requirement.

@@ -284,6 +285,7 @@ def config_from_pytorch_model(

Args:
model: PyTorch model
input_shape (list): The shape of the input tensor. First element is the batch size, needs to be None
Copy link
Contributor

Choose a reason for hiding this comment

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

We have a nice opportunity here to get rid of the first None in the shape. Just ask users to not put it, but then put it in ourselves 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, that was always very ugly. Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

In general I think we need to be careful about the batch dimensions. For ONNX it's usually just 1 (but can really be any value, e.g. 10), not None. I think we try to get rid of the batch dimension pretty quickly.

@JanFSchulte JanFSchulte added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Aug 22, 2024
@JanFSchulte
Copy link
Contributor Author

The test failure is due to no space left on device for one of the pytests. Otherwise things are working without making torch a requirement for the tool :)

@vloncar vloncar added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Aug 27, 2024
@vloncar vloncar merged commit 2cb6fe1 into fastmachinelearning:main Aug 27, 2024
10 checks passed
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