Skip to content

[WIP] Check for valid kwargs in default from_dict#67

Open
mkhorton wants to merge 4 commits intomaterialyzeai:masterfrom
mkhorton:patch-2
Open

[WIP] Check for valid kwargs in default from_dict#67
mkhorton wants to merge 4 commits intomaterialyzeai:masterfrom
mkhorton:patch-2

Conversation

@mkhorton
Copy link
Contributor

@mkhorton mkhorton commented Nov 1, 2019

Improves robustness of monty when backwards incompatible changes are made. See materialsproject/custodian#130

@shyuep
Copy link
Contributor

shyuep commented Nov 1, 2019

I am unclear why this is a warning message is needed. Surely if the deserialization fail it is quite clear that the argument is wrong.

@mkhorton
Copy link
Contributor Author

mkhorton commented Nov 1, 2019

The case where this came up:

  1. Class A has argument X and is serialized into a dictionary d.
  2. Code is updated, and argument X is removed. User tries to de-serialize d using default from_dict. De-serialization fails because argument X no longer exists.
  3. De-serialization could still succeed automatically if X was just ignored; however, the user should be warned because perhaps this is not what they want.

This came up because one of the Custodian error handlers dropped an argument, but we have thousands of FireWorks that have still that argument hard-coded, so now even some lpad commands won't work because it's trying and failing to de-serialize these old handler objects. But it can happen (and has happened) elsewhere too when making backwards-incompatible changes.

Should not make changes directly on GitHub . . .
@shyuep
Copy link
Contributor

shyuep commented Nov 1, 2019

It is a very bad idea to deserialize by ignoring arguments. Code should be updated to handle deprecations. If devs want to ignore arguments, they can by all means write a wrapper function to clean the dict before doing deserialization. But it should not be that Monty just ignores bad arguments.

@mkhorton
Copy link
Contributor Author

mkhorton commented Nov 1, 2019

So there is a pull request in Custodian to handle the deprecation in this specific instance -- but in general, the design of FireWorks is such that it becomes very fragile when these kinds of backwards incompatible changes are made. I think a warning is a fair compromise (imo).

For example, currently any historical data that has been serialized via dumpfn becomes completely unreadable if one of these arg changes happens without surgical modification of the file.

I agree that devs should handle this properly in the first place before making such changes but this does not always happen ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments