Page MenuHomePhabricator
Create Task
Maniphest T209120

Upgrade Parsoid HTML stored in the StructuredDiscussions tables
Open, MediumPublic
Actions

Description

Structured Discussions (erstwhile Flow) has had HTML stored in the database since its inception. Parsoid's HTML has upgraded and changed since then. While Parsoid has had backward compatibility code to support html -> wt for older versions, and has also grown more lenient in what HTML it expects, it would be useful if the HTML is upgraded so that Parsoid code can remove the b/c code without fear of breaking wikitext editing support on these older flow posts.

In the future, once Flow's content has been upgraded, whenever Parsoid updates its HTML versions, we could consider periodically upgrading the stored HTML.

Some notes based on a conversation between @ssastry, @Catrope and @SBisson:

Details

SubjectRepoBranchLines +/-
Add maintenance script to reserialize revision contentmediawiki/extensions/Flowmaster+174 -1
Customize query in gerrit

Related Objects
Search...

Event Timeline

• ssastry triaged this task as Medium priority.Nov 8 2018, 10:16 PM
• ssastry created this task.
Restricted Application added a project: Growth-Team. · View Herald TranscriptNov 8 2018, 10:16 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Catrope moved this task from Inbox to Needs Discussion on the Growth-Team board.Nov 13 2018, 8:30 AM
kostajh subscribed.Nov 13 2018, 6:48 PM
Catrope added a comment.Nov 14 2018, 6:28 PM

@ssastry To aid in our prioritization/triage: roughly when would you like this to be done?

• ssastry added a comment.Nov 14 2018, 6:34 PM
In T209120#4747612, @Catrope wrote:

@ssastry To aid in our prioritization/triage: roughly when would you like this to be done?

Ideally, before we begin our PHP port in earnest. Is end of Jan 2019 feasible?

• ssastry moved this task from Needs Triage to Tech Debt / Big changes on the Parsoid board.Nov 26 2018, 3:04 PM
Catrope added a project: Technical-Debt.Nov 27 2018, 6:58 AM
JTannerWMF moved this task from Needs Discussion to Upcoming Work on the Growth-Team board.Jan 2 2019, 6:05 PM
JTannerWMF moved this task from Upcoming Work to Sprint 0 (Growth Team) on the Growth-Team board.Jan 16 2019, 5:38 PM
JTannerWMF edited projects, added Growth-Team (Sprint 0 (Growth Team)); removed Growth-Team.
JTannerWMF moved this task from Incoming to Ready for Development on the Growth-Team (Sprint 0 (Growth Team)) board.Jan 16 2019, 5:43 PM
Catrope mentioned this in T217766: Flow\Exception\WikitextException: ParseEntityRef: no name.Mar 7 2019, 1:11 AM
gerritbot added a comment.Apr 2 2019, 4:57 PM

Change 500784 had a related patch set uploaded (by Catrope; owner: Catrope):
[mediawiki/extensions/Flow@master] Add maintenance script to reserialize revision content

https://gerrit.wikimedia.org/r/500784

gerritbot added a project: Patch-For-Review.Apr 2 2019, 4:57 PM
SBisson assigned this task to Catrope.Apr 3 2019, 12:54 PM
SBisson moved this task from Ready for Development to In Progress on the Growth-Team (Sprint 0 (Growth Team)) board.
JTannerWMF moved this task from In Progress to Code Review on the Growth-Team (Sprint 0 (Growth Team)) board.Apr 3 2019, 5:45 PM
gerritbot added a comment.Apr 3 2019, 11:48 PM

Change 500784 merged by jenkins-bot:
[mediawiki/extensions/Flow@master] Add maintenance script to reserialize revision content

https://gerrit.wikimedia.org/r/500784

ReleaseTaggerBot added a project: MW-1.33-notes (1.33.0-wmf.25; 2019-04-09).Apr 4 2019, 12:01 AM
SBisson moved this task from Code Review to QA on the Growth-Team (Sprint 0 (Growth Team)) board.Apr 4 2019, 5:12 PM
SBisson moved this task from QA to Code Review on the Growth-Team (Sprint 0 (Growth Team)) board.
JTannerWMF moved this task from Code Review to In Progress on the Growth-Team (Sprint 0 (Growth Team)) board.Apr 10 2019, 4:24 PM
Maintenance_bot removed a project: Patch-For-Review.May 22 2019, 3:47 PM
Catrope mentioned this in T225849: Parsoid incorrectly renders lines beginning with " :" (space colon).Jun 14 2019, 8:51 PM
Catrope edited projects, added Growth-Team; removed Growth-Team (Sprint 0 (Growth Team)).Oct 15 2019, 2:23 PM
Catrope moved this task from Inbox to Q2 2019-20 on the Growth-Team board.
• ssastry mentioned this in T259931: Remove 'missing page parameter' workaround once RESTBase cache & structured discussions content have turned over.Aug 12 2020, 9:40 PM
cscott added a parent task: T259931: Remove 'missing page parameter' workaround once RESTBase cache & structured discussions content have turned over.Aug 13 2020, 3:31 AM
• ssastry added a subscriber: JTannerWMF.Aug 14 2020, 3:34 PM

@JTannerWMF It would be helpful for the Growth Team to prioritize this work and set up a process for running upgrades of Parsoid HTML. On the Parsoid end, we are beginning to accumulate a lot of code cruft hanging on to code to support these old versions and we are going to be making more changes in the future. So, it is time for StructuredDiscussions stored content to have a reliable process for upgrading stored content. The Parsing Team will fix any bugs in Parsoid's HTML -> WT conversion that is currently blocking this work. I normally chat with @Catrope directly but I figured it might be useful to actually carve out from space at the team level for this work.

kostajh moved this task from Q2 2019-20 to Needs Discussion on the Growth-Team board.Aug 14 2020, 3:55 PM
kostajh added subscribers: MMiller_WMF, MBinder_WMF.Aug 14 2020, 8:10 PM

cc @MBinder_WMF and @MMiller_WMF

MMiller_WMF added a comment.Aug 17 2020, 5:26 PM

@ssastry -- thanks for bringing this back up. Our team is going to talk about when we can fit this into our work, but I wanted to ask for more specifics around when it is needed for your work. Is there a deadline you can give us from your perspective?

• ssastry added a comment.Aug 17 2020, 5:51 PM

Ideally end-Jan 2021, but probably no later than Q3.

• ssastry added a parent task: T203211: See what backwards compatibility code can be safely removed.Aug 17 2020, 5:52 PM
MMiller_WMF edited projects, added Growth-Team (Sprint 0 (Growth Team)); removed Growth-Team.Oct 5 2020, 6:20 AM
MMiller_WMF moved this task from In Progress to Ready for Development on the Growth-Team (Sprint 0 (Growth Team)) board.
MMiller_WMF added a project: Growth-Team-Leftovers.Oct 7 2020, 6:56 PM
MMiller_WMF removed projects: Growth-Team-Leftovers, Growth-Team (Sprint 0 (Growth Team)).Feb 16 2021, 5:28 PM

@Catrope -- I'm untagging the Growth team on this but leaving you assigned, because I think this task follows you even as you've moved to a new team. Does that sound right?

• marcella reassigned this task from Catrope to kostajh.Mar 2 2021, 3:06 PM
• marcella added a project: Growth-Team.
• marcella subscribed.

Sorry for the miscommunication about this ticket, but for now StructuredDiscussions maintenance remains part of the Growth team responsibilities, and isn't part of the Vue team work. For that reason I'm re-assigning this to @kostajh and adding it back to the Growth team boards. If you've got any questions, please let me know, and thanks a lot for helping to sort this out.

kostajh updated the task description. (Show Details)Mar 4 2021, 1:37 PM
kostajh added a comment.Mar 5 2021, 12:13 PM

Here are some notes and questions. Mostly questions so far 😄

In the future, once Flow's content has been upgraded, whenever Parsoid updates its HTML versions, we could consider periodically upgrading the stored HTML.

@ssastry I'm not sure if I understand the "we could consider periodically upgrading" part of this sentence. This task overall is about migrating older Parsoid HTML to the latest format (side note: is that 2.0.0 currently?). Are you saying that after this initial upgrade is done, conceivably we would need to run the upgrade script again on a periodic basis as the Parsoid HTML version changes from e.g. 2.0.0 to 2.0.1 or 2.1.0? Or is this task about a one-off migration?

SD currently does not store <head> => Parsoid HTML version number isn't known. T209114: Store <head> (including Parsoid version number) for HTML Flow content will address that for the future. But, for now, any upgrade script will have to convert HTML -> wt and reparse that wt -> HTML to upgrade stored HTML.

So, a few months after this task (T209120) was authored, T209114: Store <head> (including Parsoid version number) for HTML Flow content was completed and that should be working in production -- T209114 ensures that all new Parsoid HTML written/read by Flow is in the new format and includes the Parsoid version number.

Confirming then that the idea now is to target Flow content en masse prior to the patch for T209114 being deployed in Feb 2019, and our maintenance script should do essentially the same work that was implemented in T209114 but as a batch process on the server-side.

T148258: html2wt for links: Ignore data-parsoid and rel types more aggressively and generate the expected canonical forms is the bug that previous attempts to update HTML tripped on. T148258#3738352 indicates adding https: prefix to hrefs might more or less solve a large part of that problem.

I'm confused about what we need to do here. Is it adding the https:// prefix, or the idea mentioned in the task description of "As for as Flow is concerned, one solution that Flow could adopt would be to strip data-parsoid and rel info entirely from links and Parsoid does the right thing just by analyzing the href." Our maintenance script converts from html -> wikitext -> html, so stripping data-parsoid and rel-info would be done implicitly by that process. Or are you saying that html -> wikitext -> html is not going to satisfy the task description, and instead we need to do (old) html -> (new) html conversion with Parsoid without going through wikitext?

We will need to write a script to perform this upgrade of stored content...

Growth team wrote a script for this in 2018, that takes old Flow Parsoid HTML content, converts to wikitext, and converts back to the latest Parsoid HTML. I don't see anything about https prefixing in that patch, though, so I guess that's still something we need to do?

... (after backing up the content first).

This seems tricky. Has this type of thing been done before? What does the process look like for backing up the content? Who would we coordinate with on that? How do we restore in the event of problems, and how long of a window do we have to detect problems? If we run the script and an hour later there are lots of (we'd need to quantify that) problems, we could restore the previous content, but what about a week or several weeks later?

Test runs of this script will let us identify any other html -> wt -> html bugs that need addressing.

Where should the test runs take place, and who will be responsible for QA on those test runs?

Given that there are about ~800K posts, we will need some mechanism of verifying that the upgrade didn't break anything.

I don't have any thoughts right now on how we would do that. @ssastry do you have suggestions?

Now for the questions @marcella asked me to think about:

what needs to be done?

From what I can tell, the order of operations is roughly:

  1. clarify the answers to the questions I've written in my comments here and update the task description
  2. determine a test and QA process for running the script and verifying its results
  3. determine a backup/restore process
  4. do the migration

what are the risks?

An insufficient test/QA process could result in data loss (missing content, formatting problems, etc) and (I think) potentially make Flow boards utilizing this HTML unusable.

An insufficient backup/restore process could mean the inability to recover from those problems.

Due to general unfamiliarity with our team on the internals of Flow and its Parsoid integration and our team's focus being concentrated in other areas of work, there's a higher risk of errors.

how long will it take?

Hard to say, unfortunately.

If the maintenance script we already have works perfectly, then we still need to figure out the protocols around deployment, as well as backup/restore, which might be complicated.

If the maintenance script test runs reveal problems, then we will need unknown amounts of time to fix issues as they come up.

As an aside, this is a pretty major context shift for Growth-Team, so making time to properly focus on this and understand the problem space will be challenging.

@ssastry does your team have any capacity to help us with this? The Contributors-Team -> Growth-Team maintains Flow for historical reasons, due to personnel who were heavily involved in its development, none of whom are on our team anymore. Specifically, looking at the existing maintenance script written for this task, and testing it out in --dry-run mode would be helpful, but really any assistance your team could provide would be most welcome.

Tgr subscribed.Mar 5 2021, 3:14 PM

Can we rely on the normal DB backups? Especially if we keep good track of what revisions were changed, as long those exist the original value can be recovered.
Would QA involve just looking at HTML diffs produced by the dry-run of the script, or would it happen on the actual converted Flow pages?

• ssastry added a comment.EditedMar 5 2021, 10:57 PM
In T209120#6885970, @kostajh wrote:

Here are some notes and questions. Mostly questions so far 😄

In the future, once Flow's content has been upgraded, whenever Parsoid updates its HTML versions, we could consider periodically upgrading the stored HTML.

@ssastry I'm not sure if I understand the "we could consider periodically upgrading" part of this sentence. This task overall is about migrating older Parsoid HTML to the latest format (side note: is that 2.0.0 currently?). Are you saying that after this initial upgrade is done, conceivably we would need to run the upgrade script again on a periodic basis as the Parsoid HTML version changes from e.g. 2.0.0 to 2.0.1 or 2.1.0? Or is this task about a one-off migration?

Parsoid HTML version will keep updating and ideally, the Flow stored version will keep up so we don't have to have version b/c for ever. So, this task is initially about the first migration, but also about having a process for running regular migrations in the future. My guess is that (a) this first migration will yield you a bunch of scripts and process that can be reused (b) the later migrations will be much simpler if done whenever Parsoid's HTML version changes -- this will greatly reduce problems like what we have now where we have a bigger pile of version changes that have accumulated over time.

SD currently does not store <head> => Parsoid HTML version number isn't known. T209114: Store <head> (including Parsoid version number) for HTML Flow content will address that for the future. But, for now, any upgrade script will have to convert HTML -> wt and reparse that wt -> HTML to upgrade stored HTML.

So, a few months after this task (T209120) was authored, T209114: Store <head> (including Parsoid version number) for HTML Flow content was completed and that should be working in production -- T209114 ensures that all new Parsoid HTML written/read by Flow is in the new format and includes the Parsoid version number.

Confirming then that the idea now is to target Flow content en masse prior to the patch for T209114 being deployed in Feb 2019, and our maintenance script should do essentially the same work that was implemented in T209114 but as a batch process on the server-side.

You should run the update on *all* content. Having the version number makes the job easier in some cases (for your script by knowing what changed) or for Parsoid where it inspects version numbers for specific b/c handling. You implemented T209114 to assist HTML version migration, but it doesn't eliminate the need for it.

T148258: html2wt for links: Ignore data-parsoid and rel types more aggressively and generate the expected canonical forms is the bug that previous attempts to update HTML tripped on. T148258#3738352 indicates adding https: prefix to hrefs might more or less solve a large part of that problem.

I'm confused about what we need to do here. Is it adding the https:// prefix, or the idea mentioned in the task description of "As for as Flow is concerned, one solution that Flow could adopt would be to strip data-parsoid and rel info entirely from links and Parsoid does the right thing just by analyzing the href." Our maintenance script converts from html -> wikitext -> html, so stripping data-parsoid and rel-info would be done implicitly by that process. Or are you saying that html -> wikitext -> html is not going to satisfy the task description, and instead we need to do (old) html -> (new) html conversion with Parsoid without going through wikitext?

So, I am suggesting that HTML -> WT conversion results are improved and will be more reliable if you strip data-parsoid & rel info from links before you post your original HTML to Parsoid for HTML -> WT conversion.

We will need to write a script to perform this upgrade of stored content...

Growth team wrote a script for this in 2018, that takes old Flow Parsoid HTML content, converts to wikitext, and converts back to the latest Parsoid HTML. I don't see anything about https prefixing in that patch, though, so I guess that's still something we need to do?

See above. I'll go back and look at T148258 if https:// addition is still needed.

... (after backing up the content first).

This seems tricky. Has this type of thing been done before? What does the process look like for backing up the content? Who would we coordinate with on that? How do we restore in the event of problems, and how long of a window do we have to detect problems? If we run the script and an hour later there are lots of (we'd need to quantify that) problems, we could restore the previous content, but what about a week or several weeks later?

@Tgr seemed to have some ideas there. We can help you figure out how to do QA on this. When we ported Parsoid from JS to PHP, we had a HTML comparison script to let us identify problems (and which it did) and we could perhaps adapt that for your needs. You may not be able to use the script as is and it probably need adaptation. @Catrope might have some thoughts around that.

Test runs of this script will let us identify any other html -> wt -> html bugs that need addressing.

Where should the test runs take place, and who will be responsible for QA on those test runs?

Perhaps @Catrope or @Tgr have thoughts on where these test runs should take place.

Given that there are about ~800K posts, we will need some mechanism of verifying that the upgrade didn't break anything.

I don't have any thoughts right now on how we would do that. @ssastry do you have suggestions?

See above.

As an aside, this is a pretty major context shift for Growth-Team, so making time to properly focus on this and understand the problem space will be challenging.

@ssastry does your team have any capacity to help us with this? The Contributors-Team -> Growth-Team maintains Flow for historical reasons, due to personnel who were heavily involved in its development, none of whom are on our team anymore. Specifically, looking at the existing maintenance script written for this task, and testing it out in --dry-run mode would be helpful, but really any assistance your team could provide would be most welcome.

We are happy to help but this effort is best driven by you all. @marcella: we might still need to borrow some of @Catrope's time - it may just be that he needs to review the strategy or consult at appropriate points but having him involved in some fashion might make everyone's life a bit easier.

cscott subscribed.Mar 5 2021, 11:50 PM

Some brief additional comments:

@ssastry wrt the "https:// addition" and the "strip data-parsoid and rel info" it seems like this is code that could conceivably be part of parsoid. That is, if we're already running the text through in essentially html2html mode, we can have a special "flow conversion" flag that does some of these hacks for improved html2html output. Since some folks outside the org have also adopted Flow, it seems like these conversion scripts should be 'supported' to some degree through one release cycle at least so that all users of Flow can upgrade their HTML -- and then after that some of the older b/c compatibility hacks can be removed, and instead start using a more release-synchronized b/c policy --- ie, each MW/Flow/Parsoid release supports html2html upgrade from the most recent release (or the most recent LTS release) but b/c past that point can be removed.

WRT to the 'backups': presumably initially the scripts will run in a 'dry run' mode, where instead of writing the results back to the database the instead feed the output to an appropriate HTML comparison script, which reports summary statistics and any problems found. Once this process yields 'clean enough' results, you'd actually do the conversion. As long as the conversion shows up as an edit, it would get a separate revision #, and could be "undone" just by reverting to the previous revision. It should show up in most cases as a 'null edit'.

Of course we'd also have to query or track those reverts, since the b/c code can't be removed until/unless all the conversion edits "stick".

• ssastry added a comment.Mar 6 2021, 12:56 AM
In T209120#6888232, @cscott wrote:

Some brief additional comments:

@ssastry wrt the "https:// addition" and the "strip data-parsoid and rel info" it seems like this is code that could conceivably be part of parsoid. That is, if we're already running the text through in essentially html2html mode, we can have a special "flow conversion" flag that does some of these hacks for improved html2html output.

We'll have to build that endpoint, but doable.

Tgr added a comment.Mar 6 2021, 2:04 PM

Agreed that HTML transformations that facilitate legacy HTML -> wikitext conversion should be part of Parsoid, not Flow.

In T209120#6888000, @ssastry wrote:

When we ported Parsoid from JS to PHP, we had a HTML comparison script to let us identify problems (and which it did) and we could perhaps adapt that for your needs. You may not be able to use the script as is and it probably need adaptation. @Catrope might have some thoughts around that.

The goal with the Parsoid port was no difference in the output, so an automated test could detect problems. Here, changing the output is the goal. A script could generate diffs (the migration script is already able to do that in dry-run mode), but who would determine whether those are the expected diffs, and how?

Perhaps @Catrope or @Tgr have thoughts on where these test runs should take place.

I think the risky part here is the HTML transformation, and that can be easily tested in production, we just wouldn't store the result. We'd want to make a test run or two for the storage manipulation logic, but it doesn't need a whole lot of QA as it is conceptually fairly simple.

In T209120#6888232, @cscott wrote:

As long as the conversion shows up as an edit, it would get a separate revision #, and could be "undone" just by reverting to the previous revision. It should show up in most cases as a 'null edit'.

I think that leaving old revisions with outdated HTML would break undo functionality (Flow seems to do undo actions via three-way merge, so presumably the three affected revisions are converted to wikitext first). So either we remove that, or we have to convert each revision in-place (which is what the current script does).

• ssastry added a comment.Mar 6 2021, 2:45 PM
This comment was removed by • ssastry.
• ssastry added a comment.EditedMar 6 2021, 2:47 PM

I removed that last comment since I wrote that without thinking enough about it. Will revisit and write later.

RhinosF1 subscribed.Mar 6 2021, 2:50 PM
MBinder_WMF added a project: Growth-Team-Filtering.Apr 15 2021, 6:51 PM
MBinder_WMF unsubscribed.Apr 26 2021, 5:37 PM
kostajh added a comment.Jun 2 2021, 9:05 AM
In T209120#6889131, @ssastry wrote:

I removed that last comment since I wrote that without thinking enough about it. Will revisit and write later.

@ssastry I wanted to see if you have other thoughts on this. We (Growth-Team) are going to write out some more notes around scoping for this to see how or if it could fit into upcoming work, but we wanted to know if you have other comments on it first.

• ssastry added a comment.Jun 10 2021, 4:49 PM

@kostajh, sorry this mail got lost in my post-vacation volume. But, please scope this and if necessary, we can schedule a review meeting to see which pieces need which of us to work on and where the components ought to live.

Tgr mentioned this in T252038: <style> tag output for TemplateStyles by Parsoid has three CDATA wrappers.Oct 20 2021, 10:55 PM
kostajh mentioned this in T332022: [Epic] Undeploying StructuredDiscussions (Flow).May 3 2023, 7:44 PM
Arlolra added a parent task: T335843: Purge all old html version code from Parsoid repo once we switch VE from RESTBase to ParserCache.Jun 15 2023, 3:18 PM
KStoller-WMF mentioned this in T346108: [EPIC] IP Masking: StructuredDiscussions (Flow)/LiquidThreads Community discussion.Sep 11 2023, 10:25 PM
kostajh removed kostajh as the assignee of this task.Sep 18 2023, 5:20 PM
ExE-Boss subscribed.Mar 2 2024, 4:58 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL