Skip to content
This repository was archived by the owner on Apr 8, 2025. It is now read-only.

Fix loading of saved models with class weights#431

Merged
Timoeller merged 5 commits intodeepset-ai:masterfrom
PhilipMay:fix_load_model
Jun 29, 2020
Merged

Fix loading of saved models with class weights#431
Timoeller merged 5 commits intodeepset-ai:masterfrom
PhilipMay:fix_load_model

Conversation

@PhilipMay
Copy link
Copy Markdown
Contributor

@PhilipMay PhilipMay commented Jun 27, 2020 •
edited
Loading

This is a PR regarding #428 and #422

TODO

  • implement test
  • fix bug
  • review potential sideeffects

@PhilipMay PhilipMay changed the title Fix for loading of saved models Fix loading of saved models with class weights Jun 27, 2020
@PhilipMay PhilipMay mentioned this pull request Jun 27, 2020
Closed
@PhilipMay
Copy link
Copy Markdown
Contributor Author

PhilipMay commented Jun 27, 2020 •
edited
Loading

An easy fix (hack) would be to pass strict=False at this location:

FARM/farm/modeling/adaptive_model.py

Line 323 in 451bc0b

head = PredictionHead.load(config_file, strict=strict)

This would avoid the exception but I am not sure if it fixees the underlying problem.

@PhilipMay
Copy link
Copy Markdown
Contributor Author

PhilipMay commented Jun 27, 2020

The bug also happens when PyTorch 1.4.0 is used. I tested it. This is very strange. What did FARM do to introduce this bug? I am pretty sure that the early stopping example was ok 3 weeks ago...

@PhilipMay
Copy link
Copy Markdown
Contributor Author

PhilipMay commented Jun 27, 2020 •
edited
Loading

Weight is not correctly set at loading. See screenshot of the new test in debugger after loading.

Screenshot from 2020-06-27 12-39-44

@PhilipMay PhilipMay force-pushed the fix_load_model branch 3 times, most recently from 925ea19 to 7c671d8 Compare June 27, 2020 12:45
@PhilipMay
Copy link
Copy Markdown
Contributor Author

PhilipMay commented Jun 27, 2020

Well, long debugging, quick fix. :-)
IMO it can be reviewed and merged when ok.

@PhilipMay PhilipMay mentioned this pull request Jun 27, 2020
Closed
@PhilipMay
Add check for class_weights dimensions
7a9547c 
- throw ValueError
- add test
@PhilipMay
Copy link
Copy Markdown
Contributor Author

PhilipMay commented Jun 27, 2020

Because we convert the class_weights to a list I added a check that the numpy array has exactly one dimension.

@tholor tholor self-requested a review June 29, 2020 07:51
@Timoeller Timoeller requested review from Timoeller and removed request for tholor June 29, 2020 10:10
Timoeller
Timoeller approved these changes Jun 29, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@Timoeller Timoeller left a comment

Choose a reason for hiding this comment

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

Thanks @PhilipMay , well spotted. This fixes our saving + loading issues.

Calling json.dump on numpy class weights should result in TypeError: Object of type 'ndarray' is not JSON serializable
Weird that this error happens silently and class weights are simply omitted when saving the Prediction Head config. I might look into this another time.

Thanks for the changes and for adding a dedicated test case.

@Timoeller Timoeller merged commit a5bda34 into deepset-ai:master Jun 29, 2020
@PhilipMay
Copy link
Copy Markdown
Contributor Author

PhilipMay commented Jun 29, 2020 •
edited
Loading

Calling json.dump on numpy class weights should result in TypeError: Object of type 'ndarray' is not JSON serializable
Weird that this error happens silently and class weights are simply omitted when saving the Prediction Head config. I might look into this another time.

It is because you checked if it can be converted to json:

https://github.com/deepset-ai/FARM/blob/master/farm/modeling/prediction_head.py#L91

@tholor tholor mentioned this pull request Jul 1, 2020
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@Timoeller Timoeller Timoeller approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@PhilipMay @Timoeller