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

[FEATURE] Improve Backward Compatibility for pickling #4111

Closed
Bibo-Joshi opened this issue Feb 9, 2024 · 3 comments · Fixed by #4134
Closed

[FEATURE] Improve Backward Compatibility for pickling #4111

Bibo-Joshi opened this issue Feb 9, 2024 · 3 comments · Fixed by #4134
Assignees

Comments

@Bibo-Joshi
Copy link
Member

What kind of feature are you missing? Where do you notice a shortcoming of PTB?

When TG deprecates attributes, we usually convert them to properties in our classes and issue a warning on access. For this, we rename Class.attribute to Class.._attribute. This leads to problems on un-pickling data that was pickled with previous versions. While this is okay acccording to our stability policy, it would still be nice to handle this better for the users

Describe the solution you'd like

In TelegramObjejct.__setstate__, the current logic is

for key, val in state.items():
try:
setattr(self, key, val)
except AttributeError:
# catch cases when old attributes are removed from new versions
api_kwargs[key] = val # add it to api_kwargs as fallback
# For api_kwargs we first apply any kwargs that are already attributes of the object
# and then set the rest as MappingProxyType attribute. Converting to MappingProxyType
# is necessary, since __getstate__ converts it to a dict as MPT is not pickable.
self._apply_api_kwargs(api_kwargs)
self.api_kwargs = MappingProxyType(api_kwargs)

I would like to change this to

  1. first try set_attr
  2. check if the attribute name is a property
  3. if yes, try setting _attributename (unless the name already starts with underscore)
  4. if that fails or the attribute is not a property, put the attribute into api_kwargs - unless the attribute has a leading underscore. This last part is for when we finally completely remove the property - pickling with the property in place and unpickling without the property in the new version will then simply remove the attribute.

Moreover, in _apply_api_kwargs

for key in list(api_kwargs.keys()):
if getattr(self, key, True) is None:
setattr(self, key, api_kwargs.pop(key))

we can first check if tha attribute is a property

Describe alternatives you've considered

Do nothing as we didn't promise that this works.

Additional context

https://t.me/pythontelegrambotgroup/723964

@Trifase
Copy link
Contributor

Trifase commented Feb 9, 2024

As a 'victim' of this unfortunate upgrading consequence, I would like to add that - while 'doing nothing' is of course a valid course of action, it would be nice to at least having the un-pickling continue, skipping the deprecated attributes - even with a loss of data - instead of totally crashing and being de-facto locked out of the pickle file.

we can first check if tha attribute is a property

Do you mean like this?

        for key in list(api_kwargs.keys()):
            if getattr(self, key, True) is None:
                try:
                    setattr(self, key, api_kwargs.pop(key))
                except AttributeError:
                    if hasattr(self, key) and not key.startswith("_"):
                        setattr(self, f"_{key}", api_kwargs.pop(key))

@harshil21
Copy link
Member

Agree with approach, I can't tell if there are any downsides to this without actually coding it up, so that's what I'll do this weekend.

@harshil21 harshil21 self-assigned this Feb 21, 2024
@Bibo-Joshi
Copy link
Member Author

@harshil21 For reference: it's also possible to directly check if something is a property: https://stackoverflow.com/a/17735709
Try-except may still be viewed as more elegant, I guess. I'll leave that up to you:)

@github-actions github-actions bot locked and limited conversation to collaborators Mar 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants