Separate blocks from items by gpetretto · Pull Request #1936 · datalab-org/datalab

gpetretto · 2026-07-21T14:43:54Z

Implementing the separation of blocks from the items as discussed in #48.

They key principles followed for the implementation are those outlined in #48 (comment) (copy/pasted below for convenience)

Initial planning

No new functionality. The initial PR focuses exclusively on preserving existing functionalities with the blocks separated from the items. Some choices for the document structure may be driven by functionalities that are planned in the near future, but no additions here.
Blocks as separated entities. blocks are similar to File: the new block Model has parents Entry + HasOwner + HasRevisionControl.
Two collections: data_blocks + block_versions. Similar to items/item_versions. An item will always point to the document in the data_blocks collection (i.e. the latest block version). Document in item_versions points to documents in block_versions.
Items collection: the blocks_obj in the items collection can contain either the full block for backward compatibility or a dictionary with {"immutable_id": ObjectId}. The order will still be based on display_order. This is preparatory for the subsequent steps in which the blocks_objanddisplay_orderwill be replaced by{"blocks": [{"immutable_id": ...}] to fit the suggested pattern ({"type": "my-item", "blocks": [{"immutable_id": }], "files": [{"immutable_id": }]}`).
No reference to the items in the block document. Can be added in the future if needed.
Versioning. Like in the current data structure, modifying a block will not automatically trigger the creation of a new block version in block_versions, but will just modify the document in data_blocks. A new version of the block in block_versions will be triggered by an item save, but only if the content of the block has changed from the previous block version stored, to avoid duplicated entries.
Restore. Same mechanism as items. Restoring an old item version writes the old block content back in the current state + a new history entry. At this stage this can happen only when restoring an Item entirely.
No changes to File ownership. Most blocks are associated to a file. Association will remain the same.
No changes to the frontend API. Responses still expose blocks_obj + display_order, reassembled server-side from data_blocks.
Block deletion. As in the current implementation, if a block is deleted it will be removed from the data_blocks collection, but preserved in the block_versions in case it needs to be restored. Block deletion triggered only when removed from an item. If an Item is deleted, the corresponding blocks in data_blocks are deleted (not those in block_versions, mimicking what happens for items). This can revised in the future if a block can be shared.
Collections. Collection is HasBlocks, but if I am not mistaken there is really no place where a block can be assigned to a collection at the moment in the UI, and it has no versioning. We keep the same structure as Items, but there should be no need to handle backward compatibility.

A few additional points to be specified concerning the details of the implementation:

The access to the blocks is currently gated by the access to the item. There is no direct access to the blocks.
The wat the blocks are stored is preserved. A legacy block will never be changed in a new kind of block here.
It is not clear to me if for the collections the backward compatibile version should be supported. And at the moment it seems that there is no way of adding blocks to a collection. For this reason The collection blocks are left untouched. Adding the new blocking version with or without full backward compatibility should be trivial.
Consistently with the current implementation, when a new block is created in the DB it is not versioned until the item to which it belongs to is saved. To handle this properly the version attribute of the block starts at 0 and becomes 1 when the item is saved.
fixed a pre-existing (and apparently known issue): creating and then immediately deleting a block, did not truly remove it. Upon a save the removed block was recreated. Since it was related to the block management it seemed fine to add this fix here.

Note that I used the pydantic2 branch as a base for the implementation, as it seems reasonable as base, given the involvement of the backend. I targeted the PR to that branch, but the idea is to merge this in main after the pydantic2 branch has been merged.

codecov · 2026-07-21T14:46:32Z

Codecov Report

❌ Patch coverage is 91.11842% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.65%. Comparing base (623edac) to head (2e49fd4).

Files with missing lines	Patch %	Lines
pydatalab/src/pydatalab/blocks/store.py	90.17%	11 Missing ⚠️
pydatalab/src/pydatalab/routes/v0_1/items.py	89.02%	9 Missing ⚠️
pydatalab/src/pydatalab/export.py	60.00%	2 Missing ⚠️
pydatalab/src/pydatalab/models/versions.py	92.30%	2 Missing ⚠️
pydatalab/src/pydatalab/routes/v0_1/blocks.py	90.90%	2 Missing ⚠️
pydatalab/src/pydatalab/routes/v0_1/graphs.py	88.88%	1 Missing ⚠️

Additional details and impacted files

@@                         Coverage Diff                          @@
##           ml-evs/bump-pydantic-final-final    #1936      +/-   ##
====================================================================
+ Coverage                             80.24%   80.65%   +0.40%     
====================================================================
  Files                                    83       84       +1     
  Lines                                  7739     8011     +272     
====================================================================
+ Hits                                   6210     6461     +251     
- Misses                                 1529     1550      +21

Files with missing lines	Coverage Δ
pydatalab/src/pydatalab/models/__init__.py	`100.00% <100.00%> (ø)`
pydatalab/src/pydatalab/models/blocks.py	`100.00% <100.00%> (ø)`
pydatalab/src/pydatalab/models/collections.py	`95.45% <100.00%> (+0.21%)`	⬆️
pydatalab/src/pydatalab/models/items.py	`96.66% <100.00%> (+0.11%)`	⬆️
pydatalab/src/pydatalab/models/traits.py	`98.95% <ø> (-0.07%)`	⬇️
pydatalab/src/pydatalab/mongo.py	`87.24% <100.00%> (+0.40%)`	⬆️
pydatalab/src/pydatalab/versioning.py	`93.54% <100.00%> (+2.19%)`	⬆️
pydatalab/src/pydatalab/routes/v0_1/graphs.py	`98.03% <88.88%> (+0.12%)`	⬆️
pydatalab/src/pydatalab/export.py	`71.72% <60.00%> (-0.42%)`	⬇️
pydatalab/src/pydatalab/models/versions.py	`95.06% <92.30%> (-1.31%)`	⬇️
... and 3 more

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

davidwaroquiers

Very good!
I have some small comments on naming of variables + a few comments on some of the logic:

versions logic
HasBlocks outside of traits.py
maybe a few other things

davidwaroquiers · 2026-07-23T07:13:36Z

+# Here to avoid circular import
+class HasBlocks(BaseModel):
+    blocks_obj: dict[str, DataBlockResponse] = Field({})
+    """A mapping from block ID to block data."""
+
+    display_order: list[str] = Field([])
+    """The order in which to display block data in the UI."""


Not sure I like it to have HasBlocks moved here somehow. It could be possible to move DataBlockResponse to another module (e.g. blocks_response.py or something) and keep HasBlocks in traits.py.
Not sure if it's worth. Maybe there is another solution also ? Maybe @ml-evs has an idea/preference or can comment on this.

Indeed moving DataBlockResponse should be an option to avoid the circular import. I think this is a matter of preference.

davidwaroquiers · 2026-07-23T07:20:28Z

+    version: int = 0
+    """The latest committed version number of this block in `block_versions`.


Wouldn't it be better to somehow follow a logic similar to items ? i.e. having a counter in the version_counter collection based on the immutable_id of the block ? What do you think ? I am actually a bit confused about the version that is in the item and the counter that is in the version_counter in the end. It seems they are always the same (version in item and counter in the corresponding document in version_counter). Probably good to clarify this with @ml-evs

After our discussion and regarding the logic of having version = 0 here, I propose we also discuss this with @ml-evs

Somehow we keep exactly the same behavior as it is currently but I have the feeling that we may want to change that somehow (to a better, more consistent behavior).

Behavior:
When we add a block, it already adds the block directly in the item without any save.

What I would expect (to be discussed of course :):
When we add a block in the item, it will add the block only when we save the item.

indeed this is partly to keep exactly the symmetry with what the blocks are doing now.
Other ways of achieving the same would be possible, but maybe a bit more involved on the side of the code. Or maybe it could also be possible to alter the general behaviour of the backend. I did not verify if this would require any change in the routes.

davidwaroquiers · 2026-07-23T07:35:43Z

+    return None
+
+
+def is_block_reference(entry: Any) -> bool:


It's maybe a stupid thing but calling the argument entry here is a bit misleading for me because I initially "expected" to have an Entry object (a subclass of it) and then I was wondering how it could only have immutable_id and version as keys. Maybe better to rename it ? Something like "block_or_reference" ?

I have renamed all the occurrences of entry where they are referring to the block content, replaced with blocks_obj_value. It would be easy to rename it again if the naming does not sound right. At least it should avoid the ambiguity.

davidwaroquiers · 2026-07-23T07:42:49Z

+    (``{"immutable_id": ...}``) rather than a legacy embedded block.
+    """
+    return (
+        isinstance(entry, dict) and "immutable_id" in entry and set(entry.keys()) <= _REFERENCE_KEYS


Can it happen that "entry" is not a dict ? And if we assume it can, then this "non dict" object is then considered as a legacy embedded block. What would/could happen then ?

in general it should not. Since this is used in different places it is a protection against what comes in and does risk giving internal errors. I can remove the check or put a warning. Any preference?

davidwaroquiers · 2026-07-23T07:46:29Z

+    )
+
+
+def authorize_and_get_form(item_id: str | None, block_id: str) -> dict | None:


Suggested change

def authorize_and_get_form(item_id: str | None, block_id: str) -> dict | None:

def authorize_and_get_block_or_reference(item_id: str | None, block_id: str) -> dict | None:

Changing the name to make it clearer ?

I renamed it to authorize_and_get_blocks_data, so that it does not commit to the explicit content of the blocks_obj or its for in case of future further refactoring.

davidwaroquiers · 2026-07-23T14:50:22Z

+    stored_entry = (doc_before.get("blocks_obj") or {}).get(block_id) if doc_before else None
+
+    block_was_present = stored_entry is not None or (
+        doc_before is not None and block_id in (doc_before.get("display_order") or [])


Can this happen that the block was not in the list of blocks of the items but it was in the display order ? That seems like a case where the block actually does not exist and cannot be deleted, just that there was some kind of bug or weird thing happening to the display order at some point.

This would keep the equivalence with the original procedure. That one did find_one and the query was addressing the display_order. So in the case where the block_id was missing, but still in the display_order the document would have resulted as modified, returning a 200. Without this condition, instead the new version would return a 400. Here I switched to find_one_and_update to save one action on the DB, but then still tried to reproduce exactly the same logic. It can be questioned if the logic is correct (and properly preserved also...)

davidwaroquiers · 2026-07-23T15:27:10Z

+    # Embedded blocks are captured verbatim in the item snapshot.
+    # This must happen before the de-duplication guard below so that a
+    # block content change is seen as an item change.
+    for block_id, entry in list((item.get("blocks_obj") or {}).items()):


I would change the name of "entry" here, it confuses me :) (again :D)
Maybe "block_doc" or "block_obj" (I guess it would be good to have something "similar" in all places where such a thing occur)

done. see above.

davidwaroquiers · 2026-07-23T21:53:41Z

+        validation_data = restored_data
+        if any(
+            block_store.is_block_reference(entry)
+            for entry in (restored_data.get("blocks_obj") or {}).values()
+        ):
+            validation_data = {
+                **restored_data,
+                "blocks_obj": block_store.resolve_snapshot_blocks_obj(restored_data),


This check is already done in resolve_snapshot_block_obj. Is there any reason not to directly do this:

Suggested change

validation_data = restored_data

if any(

block_store.is_block_reference(entry)

for entry in (restored_data.get("blocks_obj") or {}).values()

):

validation_data = {

**restored_data,

"blocks_obj": block_store.resolve_snapshot_blocks_obj(restored_data),

validation_data = {

**restored_data,

"blocks_obj": block_store.resolve_snapshot_blocks_obj(restored_data),

right, done.

davidwaroquiers · 2026-07-24T06:37:51Z

+    # the payload from the `block_versions` and becomes the new current `blocks`
+    # state, plus a new RESTORED `block_versions` entry.
+    restored_block_pins: dict[str, dict] = {}
+    if "blocks_obj" in restored_data:


Not sure the if is needed here as the loop anyway uses "or {}"

Removing it would have the impact on the second loop, that instead works on current_item. Again, this is a case that in general should not happen, but it is a cheap check. Can be considered for removal.

davidwaroquiers · 2026-07-24T06:43:53Z

+            if new_block_version is None:
+                # No block_versions entry to restore from (should not happen):
+                # drop the entry rather than leaving a dangling reference.
+                LOGGER.error(
+                    "Dropping unrestorable block reference %s (%s) while restoring %s",
+                    block_id,
+                    entry,
+                    refcode,
+                )
+                if isinstance(restored_data.get("display_order"), list):
+                    restored_data["display_order"] = [
+                        b for b in restored_data["display_order"] if b != block_id
+                    ]
+                continue


As this should not happen. I'm wondering if it should not be a BadRequest or something. It seems that if it happens it is a bug (from somewhere else) no ? Then if we "just" log it and drop, we may not discover it while using. What do you think ?

Simply raising here in the middle of the loop would risk leaving the db in an inconsistent state. One would probably need to make a full check before and raise if one block is missing, and if not proceed with the update. Do you think it would be better?

davidwaroquiers reviewed Jul 24, 2026

View reviewed changes

ml-evs added this to the v0.8.x milestone Jul 27, 2026

ml-evs force-pushed the ml-evs/bump-pydantic-final-final branch 3 times, most recently from 0969519 to 6d809c7 Compare August 9, 2026 13:10

ml-evs force-pushed the ml-evs/bump-pydantic-final-final branch 2 times, most recently from f112cf0 to 765d3b1 Compare August 13, 2026 23:12

ml-evs force-pushed the ml-evs/bump-pydantic-final-final branch 2 times, most recently from 623e1da to c075b7a Compare August 20, 2026 16:17

gpetretto force-pushed the gp/blocks branch from 8adfb0d to 57bb7f8 Compare August 22, 2026 14:09

gpetretto added 2 commits August 23, 2026 15:10

separate blocks from items

b472313

address comments

2e49fd4

gpetretto force-pushed the gp/blocks branch from 57bb7f8 to 2e49fd4 Compare August 23, 2026 13:16

		version: int = 0
		"""The latest committed version number of this block in `block_versions`.

		)


		def authorize_and_get_form(item_id: str \| None, block_id: str) -> dict \| None:

	def authorize_and_get_form(item_id: str \| None, block_id: str) -> dict \| None:
	def authorize_and_get_block_or_reference(item_id: str \| None, block_id: str) -> dict \| None:

Uh oh!

Conversation

gpetretto commented Jul 21, 2026

Uh oh!

codecov Bot commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

davidwaroquiers left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Jul 21, 2026 •

edited

Loading