Cell metadata and persistent cell deactivation

[lungben]

ToDo:

• the indirectly disabled metadata is not updated directly when cells are activated/deactivated, but only after the next notebook update. This requires re-arranging dependency calculations, see saving / loading disable status of cells #1209
• Comment out(indirectly and directly) deactivated cells
• Load and parse metadata
• Pass metadata to the frontend (not required for cell deactivation, but for other future use cases of metadata)

[github-actions]

Try this Pull Request!

Open Julia and type:

julia> import Pkg
julia> Pkg.activate(temp=true)
julia> Pkg.add(url="https://github.com/lungben/Pluto.jl", rev="metadata")
julia> using Pluto

[fonsp]

frontend tests will give ✅ again if you update from main!

[fonsp]

EDIT: I am wondering the same as Rik in this comment: #1895 (comment)

We should store directly-disabled status in the file, but do we need to store the indirectly-disabled status in the file? We already remove the Pluto-created comment when reading the notebook file, without checking whether it was (indirectly) disabled (👍), so it's not necessary to read the file correctly, right?

Why I think this is important: if we don't store indirectly-disabled status in the file, then:

1. It doesn't need to be in metadata
2. metadata can just have a boolean field: disabled 🎉
3. This means that metadata does not contain "cache values", which I think is a problem (see below) 🎉
4. This means that we don't need cell.running_disabled anymore! No more double state! When clicking the "Disable" button, the frontend will set the cell.metadata.disabled value directly. (It's already synced, right?) 🎉

---

Here is my original comment about reducing double state

Right now, metadata is a pure function of running_disabled and depends_on_disabled_cells, which means that we don't need to store the value. Right now, all the links between metadata and the two boolean fields are manual: whenever anything changes, we manually sync between the dict and the booleans. That's not optimal! And it means that working with metadata or cell disabling will become more difficult (we would have to write comments like: "if you change cell.disabled, remember to also change cell.metadata["disabled"] to a different string!" 🤔)

I think it's a really good idea to have the metadata system be part of the implementation of #1205, but we should really go for it! Instead of using metadata as an "output", let's use metadata as a new, more flexible way to store state!

[lungben]

I removed the explicit cell.running_disabled from frontend and backend and use metadata instead.

[ederag]

Minor comment: now the indirectly disabled cells comment blocks do not have any explicit explanation,
which might surprise some users, or make old commit changes a bit less readable
("where is this block comment coming from ?"),
but I agree, it is a good tradeoff with complexity 👍.

I tried to play with this, with some file editing (removing comments, adding or removing disable lines),
and it looks great so far.
Thanks for listening, the diffs are as lean as possible.
It feels robust,
and I like that you only have to add a # ╠═╡ disabled = true line to start the notebook with the cell disabled
(no need to add block comments by hand).
Thanks !

[fonsp]

Thanks for testing out the file editing @ederag !

[j-fu]

In which respect is this better than #421 (I don't mind at all to have my idea dismissed, but modifiying the notebook format ist something which really needs to be considered carefully, so I just remind it).

[lungben]

This PR essentially implements #421 on cell level, i.e. the metadata for each Pluto cell is its own TOML "file" (written as comment before the cell in the notebook.jl file).

Suggested in #421, but not implemented here are:

1. Metadata on notebook level - I think this would be really helpful, but should be done in a separate PR.
2. Migration of existing cell metadata (UUID, hidden) to the new TOML metadata format. I agree that it would be nicer to have this also in TOML format, but this would be a breaking change in the notebook format - new notebooks would not work with old Pluto versions anymore. This PR is non-breaking, when using an old Pluto version the new metadata is just shown as a comment and ignored.

[j-fu]

Ok I understand - probably this is the way to go in the moment.
Yeah agree for 1.

For 2: I can imagine that quite lot of stuff can become part of cell metadata, so a newer "rich metadata notebook" at certain point will not make much sense for an old pluto version.

I guess the reading code for the current notebook format will stay forever, and so can the writing code, which could be invoked when opening a notebook in "legacy format." if an author wants the ability to run a notebook under old pluto versions.

We had a bit of a similar situation for the Manifest.toml...

[fonsp]

I think this is a really good middle ground! We get the future-proofness of using TOML for metadata (thanks for the suggestion!), which means that we won't have to struggle in the future on how to add new metadata items into the file format, but without a breaking change to the notebook format.

The comparison with the Manifest.toml change is a good one! I think they handled it well by adding future-proof compatibility code in 1.6, and backwards-proof compat code in 1.7 and later.

But! I think we might be able to avoid a breaking file format change, because we have more flexibility than the original Manifest.toml did (which was the reason for the new format): for example, we can put anything next to the macro bind definition, everything above the first cell is ignored by old versions of Pluto. Another path is to "add cells at the bottom" with a special UUID, just like we did for #844 .

[fonsp]

Just called with @pankgeorg , he will make some final changes to also generalize the default value to future metadata, which currently needs this special code:

https://github.com/fonsp/Pluto.jl/pull/1895/files#diff-003dd60ac4c81ca9113f164d77c246eb00a15b75dc1d03fb78ad96083fdd22c7R63

Should be ready and merged before the next community call!

[dralletje]

Is it okay if I remove the frontend changes (and remove disabled_cell as it was before), feel like it would make the PR a lot cleaner 😁

[lungben]

The frontend changes currently do 2 things:

1. make metadata available in the frontend and synchronize it with the backend
2. remove the explicit "cell_running_disabled" boolean variable and use the corresponding metadata instead.

While 1. is not required to store the cell disabling status, it is the basis for any future enhancements in the frontend using the new metadata system. Therefor I suggest to keep it in this PR.

[dralletje]

The frontend changes currently do 2 things:

1. make metadata available in the frontend and synchronize it with the backend
2. remove the explicit "cell_running_disabled" boolean variable and use the corresponding metadata instead.

While 1. is not required to store the cell disabling status, it is the basis for any future enhancements in the frontend using the new metadata system. Therefor I suggest to keep it in this PR.

I want to keep 1. for sure! Just would like to also expose cell_running_disabled to the frontend as it was before (I'm afraid of frontend bugs :P)

[Pangoraw]

Some motivations for 2. in #1895 (comment). Basically, if there is an API to get/set metadata from Julia/JS then there is going to be less logic if everything already refers to the metadata. Maybe we can namespace (under metadata.pluto.disabled) the disabled property in the metadata?

As a follow up it would be great to persists logs hiding for example.

[dralletje]

Some motivations for 2. in #1895 (comment). Basically, if there is an API to get/set metadata from Julia/JS then there is going to be less logic if everything already refers to the metadata. Maybe we can namespace (under metadata.pluto.disabled) the disabled property in the metadata?

As a follow up it would be great to persists logs hiding for example.

I only wanted to keep the frontend changes to a minimum (because they span a lot of different files), but if you are confident it works now, then having it migrated now is very nice :)

[ederag]

@dralletje Although the file format was the main focus of #1895 (comment), the user interface seemed fine as well
(as usual, quality work @lungben !)
Anyway, if you could precise what you mean by "frontend", then I'd be ready to perform more tests this week-end.

[pankgeorg]

Michiel's concern is real, pretty much this comment: https://github.com/fonsp/Pluto.jl/pull/1895/files#diff-6f965d00041ae73bc1bc31832951173264ef6f72375983989b13d99069f5aaf6R66
I'm for keeping the metadata as @lungben does, that along with the cell_memo will operate correctly for now. When we add more metadata, we'll need to add them to the CellMemo dependencies.

A quick word for my (very small!) updates:
-> on the backend, two things

1. metadata now follows the DEFAULT_METADATA "schema": cells will intialize to this dict and get updates accordingly
2. cells will deserialize removing the DEFAULT_METADATA pairs from the TOML writer input dict. So "disabled": false and every other metadatum we add will be removed there if it matched the process of [1]

-> On the front-end I fixed the possible invalidation issue and adjusted the code to expect the metadata.default to exist and be false (and then true)

finally, we fix a small saving issue: I noticed that a cell needed to run after deactivating something, to get the right stuff saved. That was because we saved very early; we need to save after the last field that gets serialized changes. Before that was cell_order, but now it is depends_on_disabled_cells. If more stuff get into the serialization of the notebook, that save (now on line 87) will need to be moved further down.

@lungben this is amazing work!
@lungben @fonsp @dralletje can you do a quick check so we get this into #main?

[lungben]

Thanks!
Your changes look fine for me.