1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-25 16:22:43 +01:00

When rendering Jupyter notebook diffs, split code inputs into individual blocks

Summary:
Ref T13425. Currently, code inputs and all outputs are grouped into a single block. This is fine for display notebooks but not great for diffing notebooks.

Instead, split source code input into individual lines with one line per block, and each output into its own block.

This allows you to leave actual line-by-line inlines on source, and comment on outputs individually.

Test Plan: {F6888583}

Maniphest Tasks: T13425

Differential Revision: https://secure.phabricator.com/D20840
This commit is contained in:
epriestley 2019-09-25 19:39:45 -07:00
parent 884cd74cc4
commit 2c06815edb
7 changed files with 321 additions and 121 deletions

View file

@ -9,10 +9,10 @@ return array(
'names' => array(
'conpherence.pkg.css' => '3c8a0668',
'conpherence.pkg.js' => '020aebcf',
'core.pkg.css' => '6a8c9533',
'core.pkg.css' => '19ec9519',
'core.pkg.js' => '6e5c894f',
'differential.pkg.css' => 'abd2c0d8',
'differential.pkg.js' => '68fa36fc',
'differential.pkg.css' => '607c84be',
'differential.pkg.js' => 'a0212a0b',
'diffusion.pkg.css' => '42c75c37',
'diffusion.pkg.js' => 'a98c0bf7',
'maniphest.pkg.css' => '35995d6d',
@ -61,7 +61,7 @@ return array(
'rsrc/css/application/dashboard/dashboard.css' => '5a205b9d',
'rsrc/css/application/diff/inline-comment-summary.css' => '81eb368d',
'rsrc/css/application/differential/add-comment.css' => '7e5900d9',
'rsrc/css/application/differential/changeset-view.css' => '5dda5e53',
'rsrc/css/application/differential/changeset-view.css' => '489b6995',
'rsrc/css/application/differential/core.css' => '7300a73e',
'rsrc/css/application/differential/phui-inline-comment.css' => '48acce5b',
'rsrc/css/application/differential/revision-comment.css' => '7dbc8d1d',
@ -113,7 +113,7 @@ return array(
'rsrc/css/application/uiexample/example.css' => 'b4795059',
'rsrc/css/core/core.css' => '1b29ed61',
'rsrc/css/core/remarkup.css' => 'f06cc20e',
'rsrc/css/core/syntax.css' => '4234f572',
'rsrc/css/core/syntax.css' => '220b85f9',
'rsrc/css/core/z-index.css' => '99c0f5eb',
'rsrc/css/diviner/diviner-shared.css' => '4bd263b0',
'rsrc/css/font/font-awesome.css' => '3883938a',
@ -169,7 +169,7 @@ return array(
'rsrc/css/phui/phui-pager.css' => 'd022c7ad',
'rsrc/css/phui/phui-pinboard-view.css' => '1f08f5d8',
'rsrc/css/phui/phui-policy-section-view.css' => '139fdc64',
'rsrc/css/phui/phui-property-list-view.css' => '34180764',
'rsrc/css/phui/phui-property-list-view.css' => 'ad841f1c',
'rsrc/css/phui/phui-remarkup-preview.css' => '91767007',
'rsrc/css/phui/phui-segment-bar-view.css' => '5166b370',
'rsrc/css/phui/phui-spacing.css' => 'b05cadc3',
@ -377,7 +377,7 @@ return array(
'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '1e413dc9',
'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '0116d3e8',
'rsrc/js/application/diff/DiffChangeset.js' => 'a31ffc00',
'rsrc/js/application/diff/DiffChangesetList.js' => '22f6bb51',
'rsrc/js/application/diff/DiffChangesetList.js' => '0f5c016d',
'rsrc/js/application/diff/DiffInline.js' => 'a4a14a94',
'rsrc/js/application/diff/behavior-preview-link.js' => 'f51e9c17',
'rsrc/js/application/differential/behavior-diff-radios.js' => '925fe8cd',
@ -554,7 +554,7 @@ return array(
'conpherence-thread-manager' => 'aec8e38c',
'conpherence-transaction-css' => '3a3f5e7e',
'd3' => '9d068042',
'differential-changeset-view-css' => '5dda5e53',
'differential-changeset-view-css' => '489b6995',
'differential-core-view-css' => '7300a73e',
'differential-revision-add-comment-css' => '7e5900d9',
'differential-revision-comment-css' => '7dbc8d1d',
@ -774,7 +774,7 @@ return array(
'phabricator-darkmessage' => '26cd4b73',
'phabricator-dashboard-css' => '5a205b9d',
'phabricator-diff-changeset' => 'a31ffc00',
'phabricator-diff-changeset-list' => '22f6bb51',
'phabricator-diff-changeset-list' => '0f5c016d',
'phabricator-diff-inline' => 'a4a14a94',
'phabricator-drag-and-drop-file-upload' => '4370900d',
'phabricator-draggable-list' => 'c9ad6f70',
@ -865,7 +865,7 @@ return array(
'phui-pager-css' => 'd022c7ad',
'phui-pinboard-view-css' => '1f08f5d8',
'phui-policy-section-view-css' => '139fdc64',
'phui-property-list-view-css' => '34180764',
'phui-property-list-view-css' => 'ad841f1c',
'phui-remarkup-preview-css' => '91767007',
'phui-segment-bar-view-css' => '5166b370',
'phui-spacing-css' => 'b05cadc3',
@ -900,7 +900,7 @@ return array(
'sprite-login-css' => '18b368a6',
'sprite-tokens-css' => 'f1896dc5',
'syntax-default-css' => '055fc231',
'syntax-highlighting-css' => '4234f572',
'syntax-highlighting-css' => '220b85f9',
'tokens-css' => 'ce5a50bd',
'trigger-rule' => '41b7b4f6',
'trigger-rule-control' => '5faf27b9',
@ -1005,6 +1005,10 @@ return array(
'javelin-workflow',
'phuix-icon-view',
),
'0f5c016d' => array(
'javelin-install',
'phuix-button-view',
),
'111bfd2d' => array(
'javelin-install',
),
@ -1065,6 +1069,9 @@ return array(
'javelin-behavior',
'javelin-request',
),
'220b85f9' => array(
'syntax-default-css',
),
'225bbb98' => array(
'javelin-install',
'javelin-reactor',
@ -1075,10 +1082,6 @@ return array(
'javelin-typeahead-source',
'javelin-util',
),
'22f6bb51' => array(
'javelin-install',
'phuix-button-view',
),
23387297 => array(
'javelin-install',
'javelin-util',
@ -1260,9 +1263,6 @@ return array(
'javelin-behavior',
'javelin-uri',
),
'4234f572' => array(
'syntax-default-css',
),
'4370900d' => array(
'javelin-install',
'javelin-util',
@ -1303,6 +1303,9 @@ return array(
'javelin-dom',
'phabricator-draggable-list',
),
'489b6995' => array(
'phui-inline-comment-view-css',
),
'48fe33d0' => array(
'javelin-behavior',
'javelin-dom',
@ -1457,9 +1460,6 @@ return array(
'javelin-dom',
'phuix-dropdown-menu',
),
'5dda5e53' => array(
'phui-inline-comment-view-css',
),
'5faf27b9' => array(
'phuix-form-control-view',
),

View file

@ -387,6 +387,8 @@ final class DifferentialChangesetTwoUpRenderer
$old_classes[] = 'old-full';
}
$old_classes[] = 'diff-flush';
$old_classes = implode(' ', $old_classes);
} else {
$old_content = null;
@ -404,6 +406,8 @@ final class DifferentialChangesetTwoUpRenderer
$new_classes[] = 'new-full';
}
$new_classes[] = 'diff-flush';
$new_classes = implode(' ', $new_classes);
} else {
$new_content = null;
@ -451,6 +455,7 @@ final class DifferentialChangesetTwoUpRenderer
'td',
array(
'class' => $old_classes,
'data-copy-mode' => 'copy-l',
),
$old_content);
@ -479,6 +484,7 @@ final class DifferentialChangesetTwoUpRenderer
array(
'class' => $new_classes,
'colspan' => '2',
'data-copy-mode' => 'copy-r',
),
$new_content);

View file

@ -57,11 +57,7 @@ final class PhabricatorJupyterDocumentEngine
$viewer = $this->getViewer();
$content = $ref->loadData();
$data = phutil_json_decode($content);
$cells = idx($data, 'cells');
if (!is_array($cells)) {
throw new Exception('Missing "cells".');
}
$cells = $this->newCells($content, true);
$idx = 1;
$blocks = array();
@ -82,8 +78,20 @@ final class PhabricatorJupyterDocumentEngine
),
$notebook_table);
// When the cell is a source code line, we can hash just the raw
// input rather than all the cell metadata.
switch (idx($cell, 'cell_type')) {
case 'code/line':
$hash_input = $cell['raw'];
break;
default:
$hash_input = serialize($cell);
break;
}
$hash = PhabricatorHash::digestWithNamedKey(
serialize($cell),
$hash_input,
'document-engine.content-digest');
$blocks[] = id(new PhabricatorDocumentEngineBlock())
@ -102,50 +110,9 @@ final class PhabricatorJupyterDocumentEngine
$content = $ref->loadData();
try {
$data = phutil_json_decode($content);
} catch (PhutilJSONParserException $ex) {
return $this->newMessage(
pht(
'This is not a valid JSON document and can not be rendered as '.
'a Jupyter notebook: %s.',
$ex->getMessage()));
}
if (!is_array($data)) {
return $this->newMessage(
pht(
'This document does not encode a valid JSON object and can not '.
'be rendered as a Jupyter notebook.'));
}
$nbformat = idx($data, 'nbformat');
if (!strlen($nbformat)) {
return $this->newMessage(
pht(
'This document is missing an "nbformat" field. Jupyter notebooks '.
'must have this field.'));
}
if ($nbformat !== 4) {
return $this->newMessage(
pht(
'This Jupyter notebook uses an unsupported version of the file '.
'format (found version %s, expected version 4).',
$nbformat));
}
$cells = idx($data, 'cells');
if (!is_array($cells)) {
return $this->newMessage(
pht(
'This Jupyter notebook does not specify a list of "cells".'));
}
if (!$cells) {
return $this->newMessage(
pht(
'This Jupyter notebook does not specify any notebook cells.'));
$cells = $this->newCells($content, false);
} catch (Exception $ex) {
return $this->newMessage($ex->getMessage());
}
$rows = array();
@ -170,6 +137,117 @@ final class PhabricatorJupyterDocumentEngine
return $container;
}
private function newCells($content, $for_diff) {
try {
$data = phutil_json_decode($content);
} catch (PhutilJSONParserException $ex) {
throw new Exception(
pht(
'This is not a valid JSON document and can not be rendered as '.
'a Jupyter notebook: %s.',
$ex->getMessage()));
}
if (!is_array($data)) {
throw new Exception(
pht(
'This document does not encode a valid JSON object and can not '.
'be rendered as a Jupyter notebook.'));
}
$nbformat = idx($data, 'nbformat');
if (!strlen($nbformat)) {
throw new Exception(
pht(
'This document is missing an "nbformat" field. Jupyter notebooks '.
'must have this field.'));
}
if ($nbformat !== 4) {
throw new Exception(
pht(
'This Jupyter notebook uses an unsupported version of the file '.
'format (found version %s, expected version 4).',
$nbformat));
}
$cells = idx($data, 'cells');
if (!is_array($cells)) {
throw new Exception(
pht(
'This Jupyter notebook does not specify a list of "cells".'));
}
if (!$cells) {
throw new Exception(
pht(
'This Jupyter notebook does not specify any notebook cells.'));
}
if (!$for_diff) {
return $cells;
}
// If we're extracting cells to build a diff view, split code cells into
// individual lines and individual outputs. We want users to be able to
// add inline comments to each line and each output block.
$results = array();
foreach ($cells as $cell) {
$cell_type = idx($cell, 'cell_type');
if ($cell_type !== 'code') {
$results[] = $cell;
continue;
}
$label = $this->newCellLabel($cell);
$lines = idx($cell, 'source');
if (!is_array($lines)) {
$lines = array();
}
$content = $this->highlightLines($lines);
$count = count($lines);
for ($ii = 0; $ii < $count; $ii++) {
$is_head = ($ii === 0);
$is_last = ($ii === ($count - 1));
if ($is_head) {
$line_label = $label;
} else {
$line_label = null;
}
$results[] = array(
'cell_type' => 'code/line',
'label' => $line_label,
'raw' => $lines[$ii],
'display' => idx($content, $ii),
'head' => $is_head,
'last' => $is_last,
);
}
$outputs = array();
$output_list = idx($cell, 'outputs');
if (is_array($output_list)) {
foreach ($output_list as $output) {
$results[] = array(
'cell_type' => 'code/output',
'output' => $output,
);
}
}
}
return $results;
}
private function renderJupyterCell(
PhabricatorUser $viewer,
array $cell) {
@ -177,13 +255,24 @@ final class PhabricatorJupyterDocumentEngine
list($label, $content) = $this->renderJupyterCellContent($viewer, $cell);
$label_cell = phutil_tag(
'th',
array(),
'td',
array(
'class' => 'jupyter-label',
),
$label);
$classes = null;
switch (idx($cell, 'cell_type')) {
case 'code/line':
$classes = 'jupyter-cell-flush';
break;
}
$content_cell = phutil_tag(
'td',
array(),
array(
'class' => $classes,
),
$content);
return phutil_tag(
@ -204,7 +293,11 @@ final class PhabricatorJupyterDocumentEngine
case 'markdown':
return $this->newMarkdownCell($cell);
case 'code':
return $this->newCodeCell($cell);
return $this->newCodeCell($cell);
case 'code/line':
return $this->newCodeLineCell($cell);
case 'code/output':
return $this->newCodeOutputCell($cell);
}
return $this->newRawCell(id(new PhutilJSON())->encodeFormatted($cell));
@ -243,23 +336,14 @@ final class PhabricatorJupyterDocumentEngine
}
private function newCodeCell(array $cell) {
$execution_count = idx($cell, 'execution_count');
if ($execution_count) {
$label = 'In ['.$execution_count.']:';
} else {
$label = null;
}
$label = $this->newCellLabel($cell);
$content = idx($cell, 'source');
if (!is_array($content)) {
$content = array();
}
$content = implode('', $content);
$content = PhabricatorSyntaxHighlighter::highlightWithLanguage(
'py',
$content);
$content = $this->highlightLines($content);
$outputs = array();
$output_list = idx($cell, 'outputs');
@ -275,7 +359,9 @@ final class PhabricatorJupyterDocumentEngine
phutil_tag(
'div',
array(
'class' => 'jupyter-cell-code PhabricatorMonospaced remarkup-code',
'class' =>
'jupyter-cell-code jupyter-cell-code-block '.
'PhabricatorMonospaced remarkup-code',
),
array(
$content,
@ -285,6 +371,45 @@ final class PhabricatorJupyterDocumentEngine
);
}
private function newCodeLineCell(array $cell) {
$classes = array();
$classes[] = 'PhabricatorMonospaced';
$classes[] = 'remarkup-code';
$classes[] = 'jupyter-cell-code';
$classes[] = 'jupyter-cell-code-line';
if ($cell['head']) {
$classes[] = 'jupyter-cell-code-head';
}
if ($cell['last']) {
$classes[] = 'jupyter-cell-code-last';
}
$classes = implode(' ', $classes);
return array(
$cell['label'],
array(
phutil_tag(
'div',
array(
'class' => $classes,
),
array(
$cell['display'],
)),
),
);
}
private function newCodeOutputCell(array $cell) {
return array(
null,
$this->newOutput($cell['output']),
);
}
private function newOutput(array $output) {
if (!is_array($output)) {
return pht('<Invalid Output>');
@ -371,4 +496,45 @@ final class PhabricatorJupyterDocumentEngine
$content);
}
private function newCellLabel(array $cell) {
$execution_count = idx($cell, 'execution_count');
if ($execution_count) {
$label = 'In ['.$execution_count.']:';
} else {
$label = null;
}
return $label;
}
private function highlightLines(array $lines) {
$head = head($lines);
$matches = null;
if (preg_match('/^%%(.*)$/', $head, $matches)) {
$restore = array_shift($lines);
$lang = $matches[1];
} else {
$restore = null;
$lang = 'py';
}
$content = PhabricatorSyntaxHighlighter::highlightWithLanguage(
$lang,
implode('', $lines));
$content = phutil_split_lines($content);
if ($restore !== null) {
$language_tag = phutil_tag(
'span',
array(
'class' => 'language-tag',
),
$restore);
array_unshift($content, $language_tag);
}
return $content;
}
}

View file

@ -63,6 +63,11 @@
padding: 1px 8px;
}
.differential-diff td.diff-flush {
padding-top: 0;
padding-bottom: 0;
}
.device .differential-diff td {
padding: 1px 4px;
}

View file

@ -6,6 +6,10 @@
color: #aa0066;
}
.remarkup-code .language-tag {
color: {$lightgreytext};
}
.remarkup-code td > span {
display: inline;
word-break: break-all;

View file

@ -298,22 +298,56 @@ div.phui-property-list-stacked .phui-property-list-properties
.jupyter-cell-code {
white-space: pre-wrap;
word-break: break-word;
background: {$lightgreybackground};
padding: 8px;
border: 1px solid {$lightgreyborder};
border-radius: 2px;
border-color: {$lightgreyborder};
border-style: solid;
}
.jupyter-cell-code-block {
padding: 8px;
border-width: 1px;
}
.jupyter-cell-code-line {
padding: 2px 8px;
border-width: 0 1px;
}
.jupyter-cell-code-head {
border-top-width: 1px;
margin-top: 4px;
padding-top: 8px;
}
.jupyter-cell-code-last {
border-bottom-width: 1px;
margin-bottom: 4px;
padding-bottom: 8px;
}
.jupyter-notebook > tbody > tr > th,
.jupyter-notebook > tbody > tr > td {
padding: 8px;
}
.jupyter-notebook > tbody > tr > th {
.jupyter-notebook > tbody > tr > td.jupyter-cell-flush {
padding-top: 0;
padding-bottom: 0;
}
.jupyter-notebook,
.jupyter-notebook > tbody > tr > td {
width: 100%;
}
.jupyter-notebook > tbody > tr > td.jupyter-label {
white-space: nowrap;
text-align: right;
min-width: 48px;
min-width: 56px;
font-weight: bold;
width: auto;
padding: 8px 8px 0;
}
.jupyter-output {

View file

@ -1195,41 +1195,26 @@ JX.install('DiffChangesetList', {
bot = tmp;
}
// Find the leftmost cell that we're going to highlight: this is the next
// <td /> in the row that does not have a "data-n" (line number)
// attribute. In 2up views, it should be directly adjacent. In
// 1up views, we may have to skip over the other line number column.
var l = top;
while (l.nextSibling && l.getAttribute('data-n')) {
l = l.nextSibling;
// Find the leftmost cell that we're going to highlight. This is the
// next sibling with a "data-copy-mode" attribute, which is a marker
// for the cell with actual content in it.
var content_cell = top;
while (content_cell && !content_cell.getAttribute('data-copy-mode')) {
content_cell = content_cell.nextSibling;
}
// Find the rightmost cell that we're going to highlight: this is the
// farthest consecutive, adjacent <td /> in the row that does not have
// a "data-n" (line number) attribute. Sometimes the left and right nodes
// are the same (left side of 2up view); sometimes we're going to
// highlight several nodes (copy + code + coverage).
var r = l;
while (true) {
// No more cells in the row, so we can't keep expanding.
if (!r.nextSibling) {
break;
}
if (r.nextSibling.getAttribute('data-n')) {
break;
}
r = r.nextSibling;
// If we didn't find a cell to highlight, don't highlight anything.
if (!content_cell) {
return;
}
var pos = JX.$V(l)
.add(JX.Vector.getAggregateScrollForNode(l));
var pos = JX.$V(content_cell)
.add(JX.Vector.getAggregateScrollForNode(content_cell));
var dim = JX.$V(r)
.add(JX.Vector.getAggregateScrollForNode(r))
var dim = JX.$V(content_cell)
.add(JX.Vector.getAggregateScrollForNode(content_cell))
.add(-pos.x, -pos.y)
.add(JX.Vector.getDim(r));
.add(JX.Vector.getDim(content_cell));
var bpos = JX.$V(bot)
.add(JX.Vector.getAggregateScrollForNode(bot));