-
Notifications
You must be signed in to change notification settings - Fork 100
🤖 Store execution outputs in Outputs
#1903
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: fccf681 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
cec055e
to
6870f57
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments as I am looking now.
Thoughts on a testing plan for this:
|
const outputsNode = select('outputs', block) as GenericNode | undefined; | ||
if (outputsNode !== undefined && !outputsNode.identifier) { | ||
// Label outputs node | ||
outputsNode.identifier = `${block.identifier}-output`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that these ID changes may need to be taken into account in the upgrade/downgrade script for thebe to continue working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep an eye on that
d8a8684
to
6f0d05b
Compare
@@ -270,12 +273,14 @@ describe('propagateBlockDataToCode', () => { | |||
result = mdast.children[0].children[0].visibility; | |||
break; | |||
case 'output': | |||
if (!has_output && target == 'output') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
target
is known to be output
here.
2060521
to
5857844
Compare
Outputs
Outputs
826b957
to
455e0d8
Compare
Outputs
Outputs
Outputs
Outputs
const output = { | ||
type: 'output', | ||
const outputs = { | ||
type: 'outputs', | ||
id: nanoid(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@agoose77 adding this immediately enables compute in figures & embeds in many of the test cases. If there is a place in myst-execute
where this should be added I suggest we do. If you could please do that, or if it's trivial, point me to the line to change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, not quite following - are you saying that it's broken currently?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need this change here for in-browser compute.
On this PR jupyter-book/myst-theme#571 you mentioned that myst-execute
does not add the id, so I'm just flagging that we need one. Adding the id here seems to be enough, but if there is a further change needed in myst-execute
, in line with your comment that should also be changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm in a conference atm. I need to check the status quo — that comment is about output.id
not outputs.id
. I think the intention is that the id
remainsper-outputs, so
myst-executeshould still work (it preserves the fields of the
outputsnode, and the
id` is pre-populated).
IIRC myst-theme in that PR now accesses the outputs.id
via context, so that the output
renderer has access to it.
Great - @agoose77 I think this is good to go pending test fixes, and is working against the theme PR jupyter-book/myst-theme#571 but I also note the testing points from @rowanc1 above:
The second will work after th content there is updated to the new form as the PR above confirms external emdbedding and referencing works as before. |
We don't need to delete nodes without children: they will simply vanish when lifted
76c5b6b
to
fccf681
Compare
Outputs
Outputs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@agoose77 verified the hard copy output, which was the last ask on this PR. I think this is good to go.
Before merging, we should ensure that the date in the |
This PR closes #1674 by changing our internal representation of cell outputs so that they are children of the
Outputs
node.Note
Part of initiative #1026
Notable Changes
${block.identifier}-outputs-${N}
outputs
node with identifier${block.identifier}-outputs
Tasks
.docx
exports still export matplotlib figuresJATS
exports still export matplotlib figures