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
Fix TypeError when torch.jit.load is passed a pathlib.Path #45825
Fix TypeError when torch.jit.load is passed a pathlib.Path #45825
Conversation
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.
Could you add a test case for this? Thank you
Codecov Report
@@ Coverage Diff @@
## master #45825 +/- ##
=======================================
Coverage 68.32% 68.32%
=======================================
Files 410 410
Lines 52978 52978
=======================================
+ Hits 36195 36196 +1
+ Misses 16783 16782 -1
Continue to review full report at Codecov.
|
Turns out |
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 good, please fix python lint. Let me know when it is ready, and I will commit for you.
@@ -478,13 +478,13 @@ def code_with_constants(self): | |||
r = self.forward.code_with_constants | |||
return (r[0], ConstMap(r[1])) | |||
|
|||
def save(self, f, _extra_files={}): | |||
def save(self, f, **kwargs): |
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.
OOC, why is this needed?
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.
The lint error was because I was passing a mutable object (i.e. {}
) as a default argument. Alternative solution would be to have None
as the default with
if _extra_files is None:
_extra_files = {}
inside the function. But before I made any changes the function was already using kwargs so I figured I'd just switch back to that.
Think it's ready now. Thanks! |
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.
@gmagogsfm has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@gmagogsfm merged this pull request in 9dc9a55. |
Fixes #45824