[Bug] in json_to_data when cloudpickle fails

[ernestum]

🐛 Bug

When cloudpickle fails to deserialize an object, json_to_data prints a warning (fine) but then replaces that object with any other object that just has been parsed before. As you can see here: https://github.com/DLR-RM/stable-baselines3/blob/54bcfa4544315fc920be0944fc380fd75e2f7c4a/stable_baselines3/common/save_util.py#L162-L171 runtime and type errors are caught, warned about but then in line 171 we fill the return data with whatever happened to be in deserialized_object.

To Reproduce

I trained a model in version 1.2 and then tried to load it in 1.4. I am not perfectly sure if this is the cause of the error though. For me the "clip_range" of a PPO model suddenly contained a deque instead of a callable or number, which caused exceptions in other places.

Expected behavior

I think in those cases, this should not be just a warning but the exception should be re-thrown with a meaningful error message.

If not, at least the value should be replaced by a clearly invalid value such as None.

### System Info

Everything installed in a venv.

OS: Linux-5.4.0-92-generic-x86_64-with-glibc2.29 #103-Ubuntu SMP Fri Nov 26 16:13:00 UTC 2021 Python: 3.8.10 Stable-Baselines3: 1.4.0 PyTorch: 1.10.2+cu102 GPU Enabled: False Numpy: 1.22.1 Gym: 0.21.0

Additional context

Found the bug while digging into this issue of the imitation library.

Checklist

• [x] I have checked that there is no similar issue in the repo (required)
• [x] I have read the documentation (required)
• [ ] I have provided a minimal working example to reproduce the bug (required)

[Miffyli]

~Good catch, but I am very confused right now. Looking at the code it should also raise an exception when deserialized_object is not defined (that part of the try block failed) but code tries to assign that variable into return_data :o . I am not sure how a deque could end up there (sounds like something is horribly wrong).~

Edit1: Ah, I just re-read the beginning of message and see the issue. I did not realize Python works this way (I though the variable would not be defined in the next iteration...).

But I agree with the sentiment here: that warning should be replaced with a proper exception informing about this error, and then maybe telling user to use custom_objects to assign that to None if they are uncertain what it should (and pray for the best).

Do you want to build a PR to fix this? :)

[araffin]

I trained a model in version 1.2 and then tried to load it in 1.4. I am not perfectly sure if this is the cause of the error though.

Probably due to a mismatch in python version (cf. #589 )

I think in those cases, this should not be just a warning but the exception should be re-thrown with a meaningful error message.

The idea of warning instead of exceptions is to list all keys that cannot be un-serialized (which is ok when you want to use the policy for inference only), but I do agree with both options (throwing error or replacing with None or even not populating the return_data which would probably be a better solution)