-
Notifications
You must be signed in to change notification settings - Fork 53
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
More informative structure mismatch error message in Linear layer #92
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
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.
Also, mind running uv run pyink penzai
to re-format the changed files using pyink so that the checks pass?
Thanks much!
|
if isinstance( | ||
self.weights, | ||
Parameter | ParameterValue, | ||
) and self.weights.label.endswith(".weights"): |
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.
Looks like pytype is raising an error here. I don't think it knows how to refine the type of self.weights
. Would you mind either:
- refactoring this so that you first assign to a local variable
weights = self.weights
and then change the rest of the function to refer toweights
, - or just adding a comment
# pytype: disable=attribute-error
here to tell pytype you know what you're doing?
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.
(You should be able to run the typechecker yourself with uv run pytype --jobs auto penzai
as long as the dev dependencies are installed.)
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.
Hm, for someone reason locally disabling the error doesn't seem to work
if isinstance(
self.weights,
Parameter | ParameterValue,
) and ( # pytype: disable=attribute-error
self.weights.label.endswith(".weights")
):
error_prefix = ( # pytype: disable=attribute-error
f"({self.weights.label[:-8]}) "
)
else:
error_prefix = ""
``
and neither does assigning self.weights to weights. Globally ignoring the attribute error does seem to work though.
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.
Hm that's weird. Does it work if you add
# pytype: disable=attribute-error
on a line on its own before the if block, and
# pytype: enable=attribute-error
on a line after?
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.
That did the trick!
I'm not sure if this is the same thing you're running into, but I've been running into an issue where |
As mentioned in #89. With the following changes,
will throw an error with information about the layer name:
This makes it much easier to debug shape errors in complex models with many layers.