Upgrade to yrs 0.16

[stefanw]

This PR upgrades yrs to 0.16 and attempts to deal with the consequences while keeping the Python API the same as discussed in #106. This is a draft PR to get feedback.

The approach here is the one proposed by @Waidhoferj in #106: The YDoc (through YDocInner) keeps a Weak reference to a YTransaction and the YTypes get an optional reference to the document in order to start or get hold of a running transaction behind the scences (.e.g in __len__).

I've implemented this for YArray and stubbed/commented out the other types for now and the tests mostly pass for tests/test_y_array.py.

The tests that don't pass (8 of 46) are due to the following problem that I haven't been able to solve yet:

• XML Treewalker needs to be implemented
• Map items, keys and values Views are currently faked by returning a list with the corresponding items with no connection to the original. This needs to be fixed.
• Integrating types into each other still seems to be buggy.
• Write an XMLFragment Python API

Any feedback is appreciated! Please be aware that I'm relatively new to Rust. I'm especially unsure about the unsafe code I used to make TransactionMut<'doc> lifetimes 'static (because #[pymethods] don't allow lifetime annotations).

[stefanw]

I've come up with a solution for the above mentioned problem: another level of indirection in the form of YTransactionWrapper. It can be passed out to Python and keeps a Rc<RefCell<YTransaction>>.

The tests in tests/test_y_array.py pass – except for one where the bug likely is in yrs..

The test for test_inserts_nested passes but still seems to produce an error:

tests/test_y_array.py::test_inserts_nested Err(PyErr {
  type: <class 'y_py.MultipleIntegrationError'>,
  value: MultipleIntegrationError("Cannot integrate a nested Ypy object because is already integrated into a YDoc: ['world']"),
  traceback: None
})
Err(PyErr {
  type: <class 'y_py.MultipleIntegrationError'>,
  value: MultipleIntegrationError("Cannot integrate a nested Ypy object because is already integrated into a YDoc: ['world']"),
  traceback: None
})

I had to introduce some extra methods on YArray to turn YTransactionWrappers back into a ReadTxn type (Deref/DerefMut convenience doesn't work due to RefCell).

I think this PR still needs more cleaning up (e.g. regarding naming). But if this is roughly the right direction, I could look into upgrading the other YTypes.

[zswaff]

If this will add support for XMLFragments, I would love to figure out how to get it merged! Happy to help! Where do things stand?

[zswaff]

@Horusiath hello! I think that Stefan and I are both pretty motivated and excited to get this going! I can also get some other engineers on my team involved. However none of us is really a Rust expert so we might need some tips and pointers or potentially even more support.

Can you help us get this moving, or if not, do you know who can? Excited to help!

[davidbrochart]

@stefanw Do you still plan to work on this PR?

[stefanw]

I rebased on main and converted YText to use the same new transaction style as in YArray and tests for YText pass now. 26 out of 46 tests pass in total.

I will work on the other types, but I would still appreciate feedback on the new transaction style and especially on the unsafe blocks.

[davidbrochart]

That's great @stefanw, thanks for working on this.
I don't have the knowledge to review this PR, but on the Python API, would it be possible to keep YTransaction instead of YTransactionWrapper, even though it's not the same object anymore?

[stefanw]

YTransaction instead of YTransactionWrapper, even though it's not the same object anymore?
Yes, I agree it makes sense to keep the publicly visible name.

I updated the PR description with latest progress and remaining TODOs that I can see.

[zswaff]

I just wrote up some related thoughts in this discussion. XML Treewalker may also be the solution that I am looking for!

[rolandmas]

As a Debian developer trying to package ypy, I'm happy to see progress on that PR. Let me know if I can do anything to help (if only check that I can build ypy on current Debian unstable).

[stefanw]

All tests pass now (skipped one that is buggy in yrs). I added more PyResults around re-using transactions and modifying the doc store during transactions.

Possible TODOs:

• Someone should check the use of unsafe! I used lots of raw pointers to get out of lifetime and Python/Rust interaction issues. Not sure what the deal with GC is.
• Maybe deduping the XML code like in yrs via traits
• Add more to the YXmlFragment API?
• Clippy cleanup

[davidbrochart]

Thanks a lot for pushing on this @stefanw!
I just tested your PR with JupyterLab, and played a bit with a shared notebook, and I get the following error:

thread '<unnamed>' panicked at 'called `Option::unwrap()` on a `None` value', src/y_map.rs:59:26

Which is here:

ypy/src/y_map.rs

Lines 57 to 61 in cf7383b

	impl WithTransaction for YMap {
	fn get_doc(&self) -> Rc<RefCell<YDocInner>> {
	self.doc.clone().unwrap()
	}
	}

Do you know what's the issue?

[stefanw]

Could you provide the Python code that causes this? The ypy types need to keep an optional reference to the ydoc but prelims don’t have one. Ideally, I should make better use of the Rust type system to avoid ‘unwrap’s.

[davidbrochart]

Since I was editing a notebook, it's probably in ynotebook.py.

[stefanw]

@davidbrochart Thanks, I found the bug, fixed it and added a test case. I also tried a jupyter notebook with real time collaboration and it seems to work!

Summary of other changes:

• refactored the whole approach of attaching a document to YTypes via the new TypeWithDoc which is more ergonomic as it makes integrated types without a doc unrepresentable.
• Aligned YXmlFragment API with YXmlElement API.
• Realised that deduping XML code via traits is a no-go because pymethods is not working with traits.

This is now getting closer to being finished. Do you want me to clean up history? (it's not too bad, but not really linear either)

[davidbrochart]

I checked on my side as well, everything seems to work fine. This is really awesome, thanks again for your work!
I don't have any issue merging as is, if you think this is ready to go. Maybe @Horusiath or @dmonad want to look at it?

[davidbrochart]

I'll merge today if there are no objections.

[Horusiath]

It's really fantastic effort. Since all mayor issues seems to be taken into account, I'd suggest merging it now. We can polish the implementation over time. As far as I looked there are not serious issues - sure there may be some things regarding style and API design, but these should not be stopping us from merge. We can discuss them later.

[davidbrochart]

I released v0.7.0a1.

[zswaff]

Thanks everyone!! Very excited to check it out!