Python learns to detect conflicts and rebase
#420
Conversation
| @@ -49,7 +49,7 @@ pub enum ConflictResolution { | |||
| } | |||
|
|
|||
| #[async_trait] | |||
| pub trait ConflictSolver { | |||
| pub trait ConflictSolver: Send + Sync { | |||
There was a problem hiding this comment.
Required for use in async python
There was a problem hiding this comment.
Hm this smells bad to me. Can we demand Send + Sync in the usage point instead?
| @@ -16,6 +16,7 @@ use crate::{ | |||
|
|
|||
| use super::{Conflict, ConflictResolution, ConflictSolver}; | |||
|
|
|||
| #[derive(Clone, Debug)] | |||
There was a problem hiding this comment.
Python is annoying to implement without this (accepting a union as argument to method)
There was a problem hiding this comment.
ups, I forgot to add those
| @@ -350,6 +350,30 @@ async def async_merge(self, changes: bytes) -> None: | |||
| """ | |||
| return await self._store.async_merge(changes) | |||
|
|
|||
| def rebase(self, solver: ConflictDetector | BasicConflictSolver) -> None: | |||
There was a problem hiding this comment.
I wonder if we should write a few paragraphs of design document on this. Two topics would be:
- how much do we want to expose of the solvers? This methods for example requires adding here when different strategies are created, and doesn't allow for users to extend the mechanisms (which I'd love some day)
- how do we expose the results of the conflict detector. This is going to be important, I expect most production jobs to at least report to logs the conflicts when they find them.
There was a problem hiding this comment.
I think we should. For 1: I think we can used subclassing/inheritance to solve this in Python probably.
I wanted to start this PR as a jumping off point, cuz I agree there is a lot to figure out here in terms of the API
| @@ -275,6 +279,56 @@ class StoreConfig: | |||
| """ | |||
| ... | |||
|
|
|||
| class VersionSelection: | |||
There was a problem hiding this comment.
Is this better than an enum?
There was a problem hiding this comment.
This is how Rust enums map unfortunately (i think, I'll confirm)
| Basic(PyBasicConflictSolver), | ||
| } | ||
|
|
||
| impl AsRef<dyn ConflictSolver + 'static> for PyConflictSolver { |
There was a problem hiding this comment.
this looks weird. I wonder why you need it. If you do, I think this probably should have a generic timeline instead of static. Something like
impl<'a> AsRef<dyn ConflictSolver + 'a> for .....
| on_user_attributes_conflict=icechunk.VersionSelection.use_ours(), | ||
| ) | ||
|
|
||
| store_b.rebase(solver) |
There was a problem hiding this comment.
Rebase fails in the this test as unsolveable 🤷
Closes #374