Skip to content
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

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

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