-
Notifications
You must be signed in to change notification settings - Fork 35
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
Improve error messages when IPU config is not compatible with model #210
Improve error messages when IPU config is not compatible with model #210
Conversation
Docs are broken because this PR is coming from a fork which has a different name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Could you run a few training jobs just to validate that eveyrthing works since you edited the parallelize
methods?
Yep, no problem. In your opinion, would it be enough to run the |
I ran
|
Yes that's great, thanks! |
@michaelbenayoun, I don't have write access to the repository so I can't merge it, could you either merge or grant me write access? |
What does this PR do?
This PR improves error messages which can occur when the
layers_per_ipu
attribute of the config does not create an array with enough entries for the number of layers in the model.Before this PR a raw
IndexError
was returned to the user, with no suggestion of what might be the cause.This PR introduces two layers of error handling:
get_ipu_layers
with an additional argument which is used for checking if the number of layers is too much. That is still not ideal as it requires models to call this properly (although could remove the default argument to make sure of that).The new error message looks like this:
Before submitting