ENH Dealing with skops persistence protocol updates #322

BenjaminBossan · 2023-03-16T17:15:15Z

~~This PR is a POC that is intended as a basis for discussing how we can deal with updating the skops persistence protocol.~~

This is a working implementation that shows one way to deal with updates in the skops persistence protocol in a backwards compatible manner that should, hopefully, scale.

As an example, the update to how functions are persisted is rolled into this PR to show that it works. Therefore, this PR supersedes #320.

Problem description

The problem in general occurs if we make a change to the state that is stored with an object. For instance, we could store an additional field. When we try to load old state without that key, there would be an error. Therefore, we have to somehow adjust our loading code to deal with this possibility.

Rejected solution

We want to avoid having a bunch of conditionals in our loading code that checks what protocol is used and then does this or that thing. This approach would become messy and error prone quite quickly, because we intend to support old protocols for a very long time.

Proposed solution

This proposal here would allow to write a clean implementation of the new deserialization code without regard for backwards compatibility. The old code, however, would not be deleted but instead moved to a different module and registered with its old protocol number. Then, while loading, we would check the protocol stored in the schema and use the old code if there is a match for it. If there is no match, we assume we can safely use the current protocol instead. For this to work, the key used for mapping the state to the loader is changed to be the tuple (loader, protocol), instead of just the loader, as has been the case so far. This is a hopefully transparent way to "dispatch" on both the loader and the protocol.

This is just a draft and I haven't tested it on a real example. If we agree that this is the way forward, I would expand on it and test it properly. But first we should agree that this is the way to go and check if there are no problems with this approach.

Description of the mechanism for changing the protocol

Below are the instructions with how to deal with a change in protocol:

Every time that a backwards incompatible change to the skops format is made for the first time within a release, the protocol should be bumped to the next higher number. The old version of the Node, which knows how to deal with the old state, should be preserved and registered. Let's give an example:

There is a BC breaking change in FunctionNode.
Since it's the first BC breaking change in the skops format in this release, bump skops.io._protocol.PROTOCOL from version X to Y.
Move the old FunctionNode code into skops/io/old/_general_vX.py, where 'X' is the old protocol.
Register the _general_vX.FunctionNode in NODE_TYPE_MAPPING inside of _persist.py (on top of registering the current version).
Write a test in test_persist_old.py that shows that the old state can still be loaded.

Now, if a user loads a FunctionNode state with version X using skops with version Y, the old code will be used instead of the new one. For all other node types, if there is no loader for version X, skops will automatically use version Y instead.

@skops-dev/maintainers RFC

This PR is a POC that is intended as a basis for discussing how we can deal with updating the skops persistence protocol. The problem in general occurs if we make a change to the state that is stored with an object. For instance, we could store an additional field. When we try to load old state without that key, there would be an error. Therefore, we have to somehow adjust our loading code to deal with this possibility. First of all, we want to avoid having a bunch of conditionals in our loading code that checks what protocol is used and then does this or that thing. This approach would become messy and error prone quite quickly, because we intend to support old protocols for a very long time. This proposal here would allow to write a clean implementation of the new deserialization code without regard for backwards compatibility. The old code, however, would not be deleted but instead moved to a different module and registered with its old protocol number. Then, while loading, we would check the protocol stored in the schema and use the old code if there is a match for it. If there is no match, we assume we can safely use the current protocol instead. This is just a draft and I haven't tested it on a real example. If we agree that this is the way forward, I would expand on it and test it properly. But first we should agree that this is the way to go and check if there are no problems with this approach. Below are the instructions with how to deal with a change in protocol: Every time that a backwards incompatible change to the skops format is made for the first time within a release, the protocol should be bumped to the next higher number. The old version of the Node, which knows how to deal with the old state, should be preserved and registered. Let's give an example: - There is a BC breaking change in FunctionNode. - Since it's the first BC breaking change in the skops format in this release, bump skops.io._protocol.PROTOCOL (this file) from version X to Y. - Move the old FunctionNode code into 'skops/io/old/_general_vX.py', where 'X' is the old protocol. - Register the _general_vX.FunctionNode in NODE_TYPE_MAPPING inside of _persist.py. Now, if a user loads a FunctionNode state with version X using skops with version Y, the old code will be used instead of the new one. For all other node types, if there is no loader for version X, skops will automatically use version Y instead.

Not sure why pre-commit didn't catch that...

adrinjalali

Other than adding some docs in the _general_v0.py and some tests to check for old protocol, I think this looks perfect.

BenjaminBossan · 2023-03-21T14:37:29Z

Other than adding some docs in the _general_v0.py and some tests to check for old protocol, I think this looks perfect.

One way to go about that would be that I wrap #320 into this PR and have the old + new FunctionNode code as a test case. Otherwise, I would have to come up with a fake example. WDYT?

adrinjalali · 2023-03-21T15:33:25Z

adding #320 here sounds good to me.

Taking the changes from skops-dev#320 to how functions are persisted and adding them here. Therefore, this PR supersedes skops-dev#320. That change is added here because it is the perfect test case for the update route of the skops protocol.

BenjaminBossan · 2023-03-21T18:10:27Z

@skops-dev/maintainers Despite the best efforts of codecov and rtd, I managed to get CI green. Ready for review.

skops/hub_utils/_hf_hub.py

skops/io/tests/_utils.py

adrinjalali · 2023-03-22T12:26:52Z

skops/io/old/_general_v0.py

+PROTOCOL = 0
+
+
+class FunctionNode(Node):


we should actually this this one, shouldn't we? we're not at the moment.

this message is a bit cryptic :P

lol, I meant to write, "we should actually test this one, and we're not at the moment", as in, test this particular upgrade for this particular node type.

Ah okay. I added a test, basically the same as for the old version, minus the downgrading. Strictly speaking, this was already tested indirectly through other tests, which is why there was no test to begin with, but it was easy enough to add.

Also, add "# pragma: no cover" where it makes sense.

BenjaminBossan

I replied to your comments, please check again.

skops/hub_utils/_hf_hub.py

BenjaminBossan · 2023-03-22T13:55:07Z

skops/io/old/_general_v0.py

+PROTOCOL = 0
+
+
+class FunctionNode(Node):


this message is a bit cryptic :P

skops/io/tests/_utils.py

Test for old and new protocol version are basically identical.

adrinjalali · 2023-03-22T18:01:20Z

I'm feeling much better about our persistence now :)

BenjaminBossan added 2 commits March 16, 2023 18:04

Fixing black

35280e7

Not sure why pre-commit didn't catch that...

BenjaminBossan changed the title ~~POC: Proposal for how to deal with protocol update~~ RFC: Proposal for how to deal with skops persistence protocol updates Mar 16, 2023

BenjaminBossan added 2 commits March 16, 2023 18:20

Check in missing module file

b535d9a

Check in even more missing stuff

b58f169

adrinjalali reviewed Mar 21, 2023

View reviewed changes

BenjaminBossan added 3 commits March 21, 2023 17:53

Upgrade protocol to use new FunctionNode

8852480

Taking the changes from skops-dev#320 to how functions are persisted and adding them here. Therefore, this PR supersedes skops-dev#320. That change is added here because it is the perfect test case for the update route of the skops protocol.

Merge branch 'main' into POC-updating-skops-persistence-protocol

e1e891a

Fix annotation issue, add missing file

b0f83fa

adrinjalali added this to the v0.6 milestone Mar 21, 2023

BenjaminBossan marked this pull request as ready for review March 21, 2023 18:09

BenjaminBossan changed the title ~~RFC: Proposal for how to deal with skops persistence protocol updates~~ Dealing with skops persistence protocol updates Mar 22, 2023

BenjaminBossan added the persistence Secure persistence feature label Mar 22, 2023

adrinjalali reviewed Mar 22, 2023

View reviewed changes

Give an example for downgrade_state

1e9415e

Also, add "# pragma: no cover" where it makes sense.

BenjaminBossan commented Mar 22, 2023

View reviewed changes

Add test for perstisting functions

333f9d1

Test for old and new protocol version are basically identical.

adrinjalali approved these changes Mar 22, 2023

View reviewed changes

adrinjalali changed the title ~~Dealing with skops persistence protocol updates~~ ENH Dealing with skops persistence protocol updates Mar 22, 2023

adrinjalali merged commit c7f8992 into skops-dev:main Mar 22, 2023

BenjaminBossan deleted the POC-updating-skops-persistence-protocol branch March 23, 2023 09:15

This was referenced Mar 23, 2023

MNT: Suggestion how to simplify serialization of funcs #320

Closed

FIX LoadContext missing arg & wrong function name #325

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH Dealing with skops persistence protocol updates #322

ENH Dealing with skops persistence protocol updates #322

BenjaminBossan commented Mar 16, 2023 •

edited

Loading

adrinjalali left a comment

BenjaminBossan commented Mar 21, 2023

adrinjalali commented Mar 21, 2023

BenjaminBossan commented Mar 21, 2023

adrinjalali Mar 22, 2023

BenjaminBossan Mar 22, 2023

adrinjalali Mar 22, 2023

BenjaminBossan Mar 22, 2023

BenjaminBossan left a comment

BenjaminBossan Mar 22, 2023

adrinjalali commented Mar 22, 2023

		PROTOCOL = 0


		class FunctionNode(Node):

		PROTOCOL = 0


		class FunctionNode(Node):

ENH Dealing with skops persistence protocol updates #322

ENH Dealing with skops persistence protocol updates #322

Conversation

BenjaminBossan commented Mar 16, 2023 • edited Loading

Problem description

Rejected solution

Proposed solution

Description of the mechanism for changing the protocol

adrinjalali left a comment

Choose a reason for hiding this comment

BenjaminBossan commented Mar 21, 2023

adrinjalali commented Mar 21, 2023

BenjaminBossan commented Mar 21, 2023

adrinjalali Mar 22, 2023

Choose a reason for hiding this comment

BenjaminBossan Mar 22, 2023

Choose a reason for hiding this comment

adrinjalali Mar 22, 2023

Choose a reason for hiding this comment

BenjaminBossan Mar 22, 2023

Choose a reason for hiding this comment

BenjaminBossan left a comment

Choose a reason for hiding this comment

BenjaminBossan Mar 22, 2023

Choose a reason for hiding this comment

adrinjalali commented Mar 22, 2023

BenjaminBossan commented Mar 16, 2023 •

edited

Loading